FS#6812 - pacman -Syi ignores the -y

Attached to Project: Pacman
Opened by Thomas Bächler (brain0) - Wednesday, 04 April 2007, 19:50 GMT
Last edited by Dan McGee (toofishes) - Tuesday, 06 November 2007, 05:49 GMT
Task Type Bug Report
Category Backend/Core
Status Closed
Assigned To Aaron Griffin (phrakture)
Architecture All
Severity Medium
Priority Normal
Reported Version 3.0.0
Due in Version 3.1.0
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

With pacman 2, when I ran pacman -Syi, it would first refresh the database and the perform the -i. With pacman 3, it only does the -i, ignoring the -y. You should also check the other -S operations, all of them should be performed after the -y if it is on the commandline.
This task depends upon

This task blocks these from closing
 FS#8109 - Pacman 3.1 Release Roadmap 
Closed by  Dan McGee (toofishes)
Tuesday, 06 November 2007, 05:49 GMT
Reason for closing:  Fixed
Additional comments about closing:  Tested and confirmed working in current pacman-git devel release.
Comment by Dan McGee (toofishes) - Tuesday, 17 April 2007, 01:45 GMT
Permissions checking in the codebase is currently not the cleanest. This fix would be quite extreme for pacman 3.0.1 wrt testing any regressions, so I don't want to make it there. We really need an overhaul of permissions stuff before we can fix this.
Comment by Xavier (shining) - Sunday, 29 July 2007, 18:05 GMT
What's the status of this?
Permissions checking isn't easy in general, but in relation to the current bug report, is there really a big problem ?
If -y is specified, then root access is needed, and that check should be enough.
And then maybe just moving the -y part at the top of the pacman_sync function.
Comment by Aaron Griffin (phrakture) - Friday, 28 September 2007, 02:47 GMT
  • Field changed: Status (Assigned → Requires Testing)
Committed changes in my working branch to all -y to function with -i, -g, -l, and -s options.
e.g. -Syl -Sys, etc etc
Comment by Xavier (shining) - Friday, 28 September 2007, 13:20 GMT
Hmm, I see, it's more complicated than I thought.
I'm not really satisfied by this solution of calling needs_transaction() twice.
But I've yet to come up with a better solution.

Also, note that there are 2 problems with the cleanup part now :
706 cleanup:
707 |_if(data) {
708 |_|_alpm_list_free(data);
709 |_}
710 |_if(alpm_trans_release() == -1) {
711 |_|_fprintf(stderr, _("error: failed to release transaction (%s)\n"),
712 |_|_ alpm_strerrorlast());
713 |_|_retval = 1;
714 |_}

For the data check to work correctly, I had to move its declaration at top of pacman_sync function, and initialize it to NULL.
Otherwise, pacman segfaults on _alpm_list_free(data);.
Just try running : sudo pacman -Sys foo

After that, there is still a problem with the second part, when doing for example : pacman -Ss foo
In this case, pacman doesn't initialize a transaction (just like before), but it'll now try to release it, and fail.
Comment by Aaron Griffin (phrakture) - Friday, 28 September 2007, 15:49 GMT
needs_transaction() isn't an expensive function. In fact, it's a handful of bitwise operations and probably faster than function call overhead.

Calling it twice isn't a bad thing at all, and that was actually why i broke it out, because in cases like this the _only_ thing we can do is check this, or keep track of it in a variable.

There isn't much else we can do, we need to have knowledge of whether we need a transaction or not with the current state of things - in the long run, transactions shouldn't be managed on the front-end, but that's a LONG time from now.
Comment by Xavier (shining) - Friday, 28 September 2007, 15:53 GMT
Another problem with this code :
418 if(needs_transaction() &&
419 alpm_trans_init(PM_TRANS_TYPE_SYNC, config->flags, cb_trans_evt,

If pacman is run in a chroot (eg like pactest does), being root is not needed, it can be run as user.
So needs_transaction return false (even though a transaction is in fact needed), and the transaction is not initialized.

Just look at the number of pactests failing in make check.
Comment by Aaron Griffin (phrakture) - Friday, 28 September 2007, 15:55 GMT
But needs_transaction doesn't check the uid at all.
Comment by Xavier (shining) - Friday, 28 September 2007, 17:01 GMT
But it does this :
55 || (strcmp(alpm_option_get_root(), "/") != 0)) {
Comment by Aaron Griffin (phrakture) - Friday, 28 September 2007, 17:07 GMT
Ah, ok, that's not a check for the root _user_, that's a check for the root _path_.

And I see what you mean. I will fix that tonight
Comment by Xavier (shining) - Saturday, 29 September 2007, 10:57 GMT
For this last problem, I just moved the root path check out of needs_transaction, and put it directly in pacman.c . I think this part is alright.

For the other problems, I thought about doing the transaction first, in a new sync trans function, which will init and release a transaction.
And then doing the commands like -Ss / -Sl / -Sg / -Si.

The problem is that for commands like -Sys / -Syl / etc, only the refresh part of the transaction should be done.
So I had to introduce an ugly sync_only hack.

Anyway, maybe this patch can give you some ideas.. But if you need more time to understand the crap I did than doing what you had in mind, just ignore it :)
Comment by Aaron Griffin (phrakture) - Tuesday, 02 October 2007, 18:59 GMT
Whoops, sorry, let me tag this one to do tonight.... /me runs in circles
Comment by Aaron Griffin (phrakture) - Thursday, 04 October 2007, 02:11 GMT
Fixed the borken-ness, courtesy of the patch above.

http://code.phraktured.net/?p=pacman.git;a=shortlog;h=working

Loading...