[pacman-dev] [PATCH] Fix bug 9395 by allowing pacman to remove unresolvable packages from a transaction

[Bryan Ischo]

Nagy Gabor wrote:
> Hi!
>
> Some comments/suggestions. (Note: First wait for Dan's response, whether
> he can support your concept (behavior change) or not. I would be sad, if
> you did useless work because of me.)
>
> I must repeat some of my earlier suggestions.
>
> We prefer shorter patches. I can fully support in your patch: dropping
> PM_TRANS_CONV_INSTALL_IGNOREPKG, I don't know why we haven't done this
> earlier. But: I think we should give a warning where we asked the user
> earlier. For example, this part could be factorized into a seperate
> patch.
>
>   
OK, I can separate that into a different patch if that is required to 
get the change in.  One reason that I thought it made sense to make 
these changes in one patch is that the new prompt I have added to some 
degree obsoletes the old prompt: whereas a user used to be asked if they 
want to un-ignore packages which have updates, the new prompt doesn't 
ask them this but does as them if they want to remove all packages from 
the transaction which cannot be updated due to packages being ignored 
(although it doesn't explicitly state which packages were ignored - 
something I'd like to do, but in a future checkin to improve the 
prompt).  In both cases, they become aware that some packages are not 
going to be updated due to being in the ignore list, but in the prior 
code, they were asked whether to let the transaction proceed by 
un-ignoring these packages, in the new code they are asked whether to 
let the transaction proceed by removing all dependencies on ignored 
packages from the transaction.  Because both prompts deal with ignore 
packages, but in different ways, I thought of the new prompt as a bit of 
a "replacement" for the old prompt, and thus include the removal of the 
old prompt in this change, rather than doing it in a later change, which 
would, in the interim code between the changes, have a "double-prompt".

In terms of a warning, pacman already prints out lines like this:

warning: kernel26: ignoring package upgrade (2.6.27.8-1 => 2.6.27.10-1)
warning: pacman: ignoring package upgrade (3.2.1FIXBUG9395-1 => 3.2.1-2)
warning: pango: ignoring package upgrade (1.22.3-2 => 1.22.4-1)

Is that not sufficient?
>> +static int is_needed(alpm_list_t *infolist, pkginfo_t *info, int marker)
>> +{
>> +	if (info->unresolvable) {
>> +		/* Obviously if it's already been marked unresolvable, it is not
>> +		   needed */
>> +		return(0);
>> +	} else if (!info->pulled) {
>> +		/* If it's top-level (not pulled), then it's needed */
>> +		return(1);
>> +	} else {
>> +		/* Now, if all of the top-level packages which depend on it are
>> +		   unresolvable, then it is unneeded */
>> +		alpm_list_t *i;
>> +		for (i = info->dependents; i; i = i->next) {
>> +			pmpkg_t *deppkg = (pmpkg_t *) i->data;
>> +			if (info->marker == marker) {
>> +				/* This means that a dependency loop has been detected; we've
>> +				   already marked this package meaning that it's already been
>> +				   seen this time through.  So ignore this dependency. */
>> +				continue;
>> +			}
>> +
>> +			info->marker = marker;
>> +
>> +			if (!is_needed(infolist, findinfo(infolist, deppkg), marker)) {
>> +				return(0);
>> +			}
>>     
>
> ???
>
>   
>> +		}
>> +		
>> +		return(1);
>>     
>
> ???
> I don't understand this part. (I checked, your earlier patch also had
> this part.) Probably want to do this: If any dependent is needed, then 
> this package is also needed, otherwise not.
>   

It's more complicated than that.  This function might not be very well 
named.  It's purpose is to determine which packages are to be removed 
from the transaction because they either were unresolvable, or were 
resolvable but are only in the transaction because they were pulled by a 
package that was unresolvable.

This function is run only after the user has been asked about the 
top-level packages which were unresolvable (i.e. the packages they 
explicitly requested be updated, but which had dependencies which could 
not be resolved).  If they say that they want these packages removed 
from the transaction, then the code has to remove not just those 
packages, but also any dependencies which were pulled only to satisfy 
dependencies of the packages being removed from the transaction (and so 
on recursively).

It determines which packages to be removed using these tests:

- If the package was marked unresolvable, then it must be removed
- Else, if the package was top-level (i.e. not pulled, i.e. explicitly 
requested by the user), then it is NOT removed (since it was already 
just determined to be resolvable)
- Else, it is a pulled package which was not resolvable.  But maybe it 
was pulled only to satisfy dependencies of packages which later on 
turned out to be unresolvable themselves.  Just because the pulled 
package has one or more unresolvable "immediate" dependents, does not 
mean that it needs to be removed (because some other dependents may be 
resolvable and retained in the transaction, thus this package should be 
too).  And just because the pulled package has all resolvable 
"immediate" dependents, does not mean that it should be retained 
(because maybe all dependents of its dependents are going to be removed 
from the transaction, in which case this package should be too).  The 
simplest question to ask about such a package is, "is there any path 
back through the dependents chain from this package to a top-level 
package that will be retained in this transaction".  In other words, are 
any top-level dependent "ancestors" of this package needed?  If so, then 
this package is needed too.  Otherwise, this package is not needed; we 
can visualize this as the entire set of subtrees rooted at the top-level 
packages and which include the package that we are testing, as being 
unresolvable and thus needing to be removed from the transaction.  So 
the test we finally do is recursively ask  each dependent if it is 
needed (which will only recursively stop at the top-level packages), and 
if any are, then this package is needed too.

HOWEVER, in writing this very last line, I realize that my final test 
should be:

if (is_needed(infolist, findinfo(infolist, deppkg), marker)) {
    return(1);
}

This is because we want to consider a package needed if any of its dependent packages are needed.  The code as it was was mistakenly considering a package needed only if all of its dependent packages are needed.

Does that make sense?


>> +	}
>> +}
>>     
>
>
>
>   
>> +			pm_errno = PM_ERR_UNSATISFIED_DEPS;
>> +			_alpm_log(PM_LOG_ERROR, _("cannot resolve dependencies for:\n"));
>> +			for (i = unresolvable; i; i = i->next) {
>> +				_alpm_log(PM_LOG_ERROR, _("\t%s\n"), alpm_pkg_get_name((pmpkg_t *) i->data));
>>     
>
> 2. The result of this:
> error: cannot resolve dependencies for:
> error:	foo
> error:	bar
> ...
>
> This is ugly, we use data list for this purpose, the front-end should
> build this message.
>   
Please tell me exactly the format you want to see this error message in, 
and I will make it so.  I don't know exactly what you are expecting so I 
am not sure how to fix the problem you are pointing out.
>   
>> +				if (data) {
>> +					alpm_list_t *targ = alpm_list_add(NULL, i->data);
>> +					deps = alpm_checkdeps(_alpm_db_get_pkgcache(local), 0, remove, targ);
>>     
>
> This is not OK. (If this part existed in your previous patch, sorry for
> missing it.) In the target list we may have a satisfier.
>   
I don't understand what you are saying here.   A few points:

1. The code already looked like this; I haven't changed how the 
dependencies are checked in my patch.  So what you are objecting to is, 
I think, already the way that this code worked, not a change that I have 
made.
2. I think you already talked about this in a previous email, when you said:

 > 4. It is not easy to determine which package is unresolvable. If a
 > pulled dependency satisfier of foo is unresolvable we may could find an
 > other (resolvable) satisfier. But this is not handled in the current
 > code neither, so your _alpm_mark_unresolvable() is OK.

3. alpm_checkdeps just returns the list of dependencies for the 
package.  The very first thing that the code then does with each 
dependency is look for it in the target list.  Then if it's not found, 
it uses _alpm_resolvedep to try to find a satisfier.  Here is the code 
in question:

Thanks for your comments.  I hope that we're getting closer to getting 
my changes in :)

Bryan