[pacman-dev] Version bounds for IgnorePkg

[Allan McRae]

On 06/07/15 12:30, Andrew Gregory wrote:
> On 07/06/15 at 11:35am, Allan McRae wrote:
>> On 05/07/15 22:44, Daniel Schoepe wrote:
>>> On Sun, 05 Jul 2015 14:01 +0200, Allan McRae wrote:
>>>>> - As discussed when I brought this up a while back, only equality constraints
>>>>>   are supported, since the other types of bounds would have unclear semantics.
>>>>
>>>> Can you clarify what is unclear there?  At first glance I would think
>>>> that "IgnorePkg foo < 5.0" would be useful.
>>>
>>> You're right, this sounds like a useful constraint as well. Not sure if
>>> something like "foo > 5.0" would also have some use cases, but allowing
>>> it won't hurt, I suppose.
>>>
>>> On the implementation side, this makes things a bit more tricky, since
>>> the code in sync.c also checks if the locally installed version is
>>> matched by IgnorePkg / IgnoreGroup. For example, when having foo-1.0
>>> installed and setting IgnorePkg foo<1.1, an update to foo-1.2 will be
>>> ignored, since the locally installed version matches foo<1.1.
>>>
>>> I'm not sure what the motivation is for taking the locally installed
>>> version into account. It might be sufficient to only check the package
>>> that's about to be installed.
>>
>> I have no idea why the locally installed package is taken into account.
>>  That is unexpected...   I agree that only checking the package that is
>> about to be installed is the way to go.
> 
> Both the installed and incoming packages need to be checked.
> Otherwise replacers would be able to "upgrade" ignored packages
> because their names would not likely match the IgnorePkg setting.

Ohhh...

That seems an odd workaround.  Can't it be done when looking at the
replacement?

Anyway, this has gone beyond the scope of the original patch.  Stick to
the equality in the operator.

>>>>> - This is checked for by pacman and libalpm assumes that only such IgnorePkg
>>>>>   entries are added. I'm not sure if this check should rather be handled by
>>>>>   libalpm instead.
>>>>
>>>> I think this needs to be in the backend.  The all frontends benefit.
>>>>
>>>>> - alpm_handle_t.ignorepkg is now a list of alpm_depend_t structures, not
>>>>>   strings. This required some more widespread changes to avoid duplicating the
>>>>>   code that handles assumeinstalled entries.
>>>>
>>>> I'd see a string being passed to the backend which converts it to
>>>> alpm_depend_t.  And having it is that type will need a comment in the
>>>> handle field that we are reusing that struct to have versioning on ignoring.
>>>
>>> At the moment, alpm_depend_t is also used for assumeinstalled, with the
>>> conversion being done by pacman. For example,
>>> alpm_option_add_assumeinstalled takes a depend_t structure as an
>>> argument and assumes that the conversion is done before interacting with
>>> alpm. I guess we should be consistent and either handle both in alpm or
>>> both in pacman (or other frontends).
>>
>> I'd like as much work to be done in the backend as possible.  That means
>> that frontends are easier to implement.  So both in alpm is my
>> preference.  Another patch if you are willing!
> 
> assumeinstalled takes a list of depend_t structs rather than strings
> for consistency.  Taking strings would require that ether the getter
> and setter have different types or the getter convert the depend_t
> structs back into strings which the front-end would then have to free.
> None of our other option getters return alloc'd memory.
> 
> I don't see either --ignore or --assumeinstalled as options that
> should see particularly frequent use, so perhaps we could get away
> with storing them as strings and parsing them into short-lived
> depend_t structs as they're needed.

OK - Sticking to depend_t structures for now is fine.

Perhaps if other frontends want, we could add a helper function
alpm_option_add_ignorpkg_string() that takes the string and does the
conversion for us.


So that means that both my comments on the orginal patch are now moot!
 I'll go back and review it properly soon.

Allan