[pacman-dev] Version bounds for IgnorePkg
Hi, A common situation for me is to ignore a specific known-to-be-buggy version of a package with a fix being available in the next upcoming version. I think it would be a good idea to allow ignoring a specific package version. The following patch implements this functionality. Since this is my first contribution to pacman / alpm, I'm not sure if I followed the coding style. A few notes on the implementation: - 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. - 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. - 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. Comments and suggestions are of course appreciated. Best regards, Daniel
This patch adds support for ignoring specific version of packages in
IgnorePkg. This is useful for ignoring a version that is known to be
buggy where a fix is going to be included in the next version. Only
equality comparisons (e.g. "foo=1.0-1" or "bar=1.0") are supported.
Signed-off-by: Daniel Schoepe
On 30/06/15 01:35, Daniel Schoepe wrote:
This patch adds support for ignoring specific version of packages in IgnorePkg. This is useful for ignoring a version that is known to be buggy where a fix is going to be included in the next version. Only equality comparisons (e.g. "foo=1.0-1" or "bar=1.0") are supported.
Signed-off-by: Daniel Schoepe
--- NEWS | 2 + doc/pacman.conf.5.txt | 4 +- lib/libalpm/alpm.h | 4 +- lib/libalpm/handle.c | 97 +++++++++++++++++++++++++----------------- lib/libalpm/package.c | 27 ++++++++++-- src/pacman/conf.c | 62 ++++++++++++++++++++------- test/pacman/tests/TESTS | 5 +++ test/pacman/tests/ignore009.py | 14 ++++++ test/pacman/tests/ignore010.py | 13 ++++++ test/pacman/tests/ignore011.py | 13 ++++++ test/pacman/tests/ignore012.py | 13 ++++++ test/pacman/tests/ignore013.py | 13 ++++++ 12 files changed, 205 insertions(+), 62 deletions(-) create mode 100644 test/pacman/tests/ignore009.py create mode 100644 test/pacman/tests/ignore010.py create mode 100644 test/pacman/tests/ignore011.py create mode 100644 test/pacman/tests/ignore012.py create mode 100644 test/pacman/tests/ignore013.py
NEW updates, documentation and testsuite additions! This is awesome already. Note: warning: 1 line adds whitespace errors.
diff --git a/NEWS b/NEWS index a4d3dba..f42a185 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ VERSION DESCRIPTION 5.0.0 - pacman can check the validity of the local and sync databases (-Dk and -Dkk respectively). This replaces the 'testdb' software + - pacman supports specific version bounds on packages in + IgnorePkg.
That sounds like you can used inequalities - how about: - pacman supports adding a specific version of a package in IgnorePkg The coding and style looks fine. I did not review further based on my comments to your patch cover letter. Allan
On 30/06/15 01:35, Daniel Schoepe wrote:
Hi,
A common situation for me is to ignore a specific known-to-be-buggy version of a package with a fix being available in the next upcoming version. I think it would be a good idea to allow ignoring a specific package version. The following patch implements this functionality.
Sounds great! Always good to see a first time contributor here.
Since this is my first contribution to pacman / alpm, I'm not sure if I followed the coding style. A few notes on the implementation:
- 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.
- 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.
Comments and suggestions are of course appreciated.
Best regards, Daniel
A
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.
- 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). Best regards, Daniel
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.
- 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! Allan
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.
- 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. apg
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
On 07/06/15 at 06:17pm, Allan McRae wrote:
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?
I think checking the incoming package is actually the odd workaround here. The reason adding a version constraint to IgnorePkg isn't straightforward is that we're trying to have IgnorePkg do two separate things. Its intended purpose, at least according to the documentation, is to pin an installed package to its current version, in which case IgnorePkg should really refer to the name of the installed package, not the incoming one. This patch is trying to turn it into a blacklist to prevent certain packages from being installed. There's obviously a lot of overlap between those two functions, but version constraints don't really make sense in the former, hence the confusion. I'm fine with having IgnorePkg do both if we can do it sanely, but I think the desired effect would be much more straightforward to achieve with a separate blacklist option. apg
On Mon, 06 Jul 2015 17:07 +0200, Andrew Gregory wrote:
[..] I'm fine with having IgnorePkg do both if we can do it sanely, but I think the desired effect would be much more straightforward to achieve with a separate blacklist option.
I think the equality constraint is somewhat consistent with the previous intention of IgnorePkg if you see it as "pacman operations should not affect the state of this package". Without a version constraint, this means that if the package is installed, it won't be upgraded; if it's not installed, it won't get installed either. Similarly, for a specific version, then this version won't get removed or installed. (Note that a side effect of the current patch is that if the IgnorePkg-ed version is currently installed, it will never be updated.) Under that interpretation, it also makes sense that "foo < 5" leads to foo never getting updated if some foo < 5 is already installed. That being said, I'm not against adding a separate option for blacklisting, but I guess it would lead to some additional complexity and/or duplication in the code (since it does something very similar to IgnorePkg). Best regards, Daniel
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Daniel Schoepe