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 daniel@schoepe.org --- 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
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. 4.2.1 - Remove warnings about incorrect directory ownership until packaging files with dynamic users/groups is improved - Do not require a specific automake version when building from diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index 383e072..2e29ad0 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -92,7 +92,9 @@ Options
*IgnorePkg =* package ...:: Instructs pacman to ignore any upgrades for this package when performing - a '--sysupgrade'. Shell-style glob patterns are allowed. + a '--sysupgrade'. Shell-style glob patterns are allowed. Specific + package versions can be ignored; for example 'foo=1.0' ignores only + updates to version 1.0 of foo.
*IgnoreGroup =* group ...:: Instructs pacman to ignore any upgrades for all packages in this diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 06e080b..34753b0 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -821,9 +821,9 @@ int alpm_option_match_noextract(alpm_handle_t *handle, const char *path); * @{ */ alpm_list_t *alpm_option_get_ignorepkgs(alpm_handle_t *handle); -int alpm_option_add_ignorepkg(alpm_handle_t *handle, const char *pkg); +int alpm_option_add_ignorepkg(alpm_handle_t *handle, const alpm_depend_t *pkg); int alpm_option_set_ignorepkgs(alpm_handle_t *handle, alpm_list_t *ignorepkgs); -int alpm_option_remove_ignorepkg(alpm_handle_t *handle, const char *pkg); +int alpm_option_remove_ignorepkg(alpm_handle_t *handle, const alpm_depend_t *pkg); /** @} */
/** @name Accessors to the list of ignored groups. diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 4915d0b..9980201 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -88,7 +88,8 @@ void _alpm_handle_free(alpm_handle_t *handle) FREE(handle->gpgdir); FREELIST(handle->noupgrade); FREELIST(handle->noextract); - FREELIST(handle->ignorepkg); + alpm_list_free_inner(handle->ignorepkg, (alpm_list_fn_free)alpm_dep_free); + alpm_list_free(handle->ignorepkg); FREELIST(handle->ignoregroup);
alpm_list_free_inner(handle->assumeinstalled, (alpm_list_fn_free)alpm_dep_free); @@ -512,6 +513,54 @@ static int _alpm_option_strlist_rem(alpm_handle_t *handle, alpm_list_t **list, c return 0; }
+/* Note that, at the moment, the following functions don't copy their + arguments like the _strlist equivalents do. */ +static int _alpm_option_deplist_add(alpm_handle_t *handle, alpm_list_t **list, const alpm_depend_t *dep) +{ + CHECK_HANDLE(handle, return -1); + *list = alpm_list_add(*list, (void *)dep); + return 0; +} + +static int _alpm_option_deplist_set(alpm_handle_t *handle, alpm_list_t **list, alpm_list_t *newlist) +{ + CHECK_HANDLE(handle, return -1); + if(list) { + alpm_list_free_inner(*list, (alpm_list_fn_free)alpm_dep_free); + alpm_list_free(*list); + } + *list = newlist; + return 0; +} + +static int depend_cmp(const void *d1, const void *d2) +{ + const alpm_depend_t *dep1 = d1; + const alpm_depend_t *dep2 = d2; + + if(strcmp(dep1->name, dep2->name) == 0 && + strcmp(dep1->version, dep2->version) == 0 && + dep1->mod == dep2->mod) { + return 0; + } + + return -1; +} + +static int _alpm_option_deplist_rem(alpm_handle_t *handle, alpm_list_t **list, const alpm_depend_t *dep) +{ + alpm_depend_t *vdata = NULL; + CHECK_HANDLE(handle, return -1); + + *list = alpm_list_remove(handle->assumeinstalled, dep, &depend_cmp, (void **)&vdata); + if(vdata != NULL) { + alpm_dep_free(vdata); + return 1; + } + + return 0; +} + int SYMEXPORT alpm_option_add_noupgrade(alpm_handle_t *handle, const char *pkg) { return _alpm_option_strlist_add(handle, &(handle->noupgrade), pkg); @@ -552,19 +601,19 @@ int SYMEXPORT alpm_option_match_noextract(alpm_handle_t *handle, const char *pat return _alpm_fnmatch_patterns(handle->noextract, path); }
-int SYMEXPORT alpm_option_add_ignorepkg(alpm_handle_t *handle, const char *pkg) +int SYMEXPORT alpm_option_add_ignorepkg(alpm_handle_t *handle, const alpm_depend_t *pkg) { - return _alpm_option_strlist_add(handle, &(handle->ignorepkg), pkg); + return _alpm_option_deplist_add(handle, &(handle->ignorepkg), pkg); }
int SYMEXPORT alpm_option_set_ignorepkgs(alpm_handle_t *handle, alpm_list_t *ignorepkgs) { - return _alpm_option_strlist_set(handle, &(handle->ignorepkg), ignorepkgs); + return _alpm_option_deplist_set(handle, &(handle->ignorepkg), ignorepkgs); }
-int SYMEXPORT alpm_option_remove_ignorepkg(alpm_handle_t *handle, const char *pkg) +int SYMEXPORT alpm_option_remove_ignorepkg(alpm_handle_t *handle, const alpm_depend_t *pkg) { - return _alpm_option_strlist_rem(handle, &(handle->ignorepkg), pkg); + return _alpm_option_deplist_rem(handle, &(handle->ignorepkg), pkg); }
int SYMEXPORT alpm_option_add_ignoregroup(alpm_handle_t *handle, const char *grp) @@ -584,47 +633,17 @@ int SYMEXPORT alpm_option_remove_ignoregroup(alpm_handle_t *handle, const char *
int SYMEXPORT alpm_option_add_assumeinstalled(alpm_handle_t *handle, const alpm_depend_t *dep) { - CHECK_HANDLE(handle, return -1); - - handle->assumeinstalled = alpm_list_add(handle->assumeinstalled, (void *)dep); - return 0; + return _alpm_option_deplist_add(handle, &(handle->assumeinstalled), dep); }
int SYMEXPORT alpm_option_set_assumeinstalled(alpm_handle_t *handle, alpm_list_t *deps) { - CHECK_HANDLE(handle, return -1); - if(handle->assumeinstalled) { - alpm_list_free_inner(handle->assumeinstalled, (alpm_list_fn_free)alpm_dep_free); - alpm_list_free(handle->assumeinstalled); - } - handle->assumeinstalled = deps; - return 0; -} - -static int assumeinstalled_cmp(const void *d1, const void *d2) -{ - const alpm_depend_t *dep1 = d1; - const alpm_depend_t *dep2 = d2; - - if(strcmp(dep1->name, dep2->name) == 0 && strcmp(dep1->version, dep2->version) == 0) { - return 0; - } - - return -1; + return _alpm_option_deplist_set(handle, &(handle->assumeinstalled), deps); }
int SYMEXPORT alpm_option_remove_assumeinstalled(alpm_handle_t *handle, const alpm_depend_t *dep) { - alpm_depend_t *vdata = NULL; - CHECK_HANDLE(handle, return -1); - - handle->assumeinstalled = alpm_list_remove(handle->assumeinstalled, dep, &assumeinstalled_cmp, (void **)&vdata); - if(vdata != NULL) { - alpm_dep_free(vdata); - return 1; - } - - return 0; + return _alpm_option_deplist_rem(handle, &(handle->assumeinstalled), dep); }
int SYMEXPORT alpm_option_set_arch(alpm_handle_t *handle, const char *arch) diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 4b5120b..ac27cab 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -768,10 +768,29 @@ alpm_pkg_t SYMEXPORT *alpm_pkg_find(alpm_list_t *haystack, const char *needle) int SYMEXPORT alpm_pkg_should_ignore(alpm_handle_t *handle, alpm_pkg_t *pkg) { alpm_list_t *groups = NULL; - - /* first see if the package is ignored */ - if(alpm_list_find(handle->ignorepkg, pkg->name, _alpm_fnmatch)) { - return 1; + alpm_list_t *pkgs = NULL; + + /* first see if package is matched by IgnorePkg */ + for(pkgs = handle->ignorepkg; pkgs; pkgs = pkgs->next) { + alpm_depend_t *ignored = pkgs->data; + char *oldname = NULL; + /* check names using fnmatch first: */ + if(strcmp(ignored->name, pkg->name) != 0 && + _alpm_fnmatch(ignored->name, pkg->name) == 0) { + oldname = ignored->name; + /* If the name matched a glob expression, replace the pattern by + the concrete name for the version comparison to work. */ + ignored->name = strdup(pkg->name); + ignored->name_hash = _alpm_hash_sdbm(pkg->name); + } + int result = _alpm_depcmp_literal(pkg, ignored); + if(oldname != NULL) { + /* Restore the original glob expression. */ + ignored->name = oldname; + } + if(result) { + return 1; + } }
/* next see if the package is in a group that is ignored */ diff --git a/src/pacman/conf.c b/src/pacman/conf.c index ccf8183..177ff86 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -677,6 +677,42 @@ static int register_repo(config_repo_t *repo) return 0; }
+typedef int (*add_fn)(alpm_handle_t *, const alpm_depend_t *); +static int _add_dep_list(alpm_handle_t *handle, alpm_list_t *list, add_fn add, + const char *name) +{ + int ret; + for(; list; list = list->next) { + char *entry = list->data; + alpm_depend_t *dep = alpm_dep_from_string(entry); + if(!dep) { + return 1; + } + pm_printf(ALPM_LOG_DEBUG, "parsed %s: %s %s\n", name, dep->name, + dep->version); + + ret = (*add)(handle, dep); + if(ret != 0) { + pm_printf(ALPM_LOG_ERROR, _("Failed to pass %s entry to libalpm\n"), + name); + alpm_dep_free(dep); + return ret; + } + } + return 0; +} +/* This is basically alpm_option_add_ignorepkg with an extra sanity + * check. */ +static int _add_ignorepkg(alpm_handle_t *handle, const alpm_depend_t *dep) +{ + if (dep->mod != ALPM_DEP_MOD_ANY && dep->mod != ALPM_DEP_MOD_EQ) { + pm_printf(ALPM_LOG_ERROR, "Invalid version constraint in IgnorePkg: %s", + dep->name); + return 1; + } + return alpm_option_add_ignorepkg(handle, dep); +} + /** Sets up libalpm global stuff in one go. Called after the command line * and initial config file parsing. Once this is complete, we can see if any * paths were defined. If a rootdir was defined and nothing else, we want all @@ -783,26 +819,20 @@ static int setup_libalpm(void) alpm_option_set_usesyslog(handle, config->usesyslog); alpm_option_set_deltaratio(handle, config->deltaratio);
- alpm_option_set_ignorepkgs(handle, config->ignorepkg); alpm_option_set_ignoregroups(handle, config->ignoregrp); alpm_option_set_noupgrades(handle, config->noupgrade); alpm_option_set_noextracts(handle, config->noextract);
- for(i = config->assumeinstalled; i; i = i->next) { - char *entry = i->data; - alpm_depend_t *dep = alpm_dep_from_string(entry); - if(!dep) { - return 1; - } - pm_printf(ALPM_LOG_DEBUG, "parsed assume installed: %s %s\n", dep->name, dep->version); - - ret = alpm_option_add_assumeinstalled(handle, dep); - if(ret) { - pm_printf(ALPM_LOG_ERROR, _("Failed to pass %s entry to libalpm"), "assume-installed"); - alpm_dep_free(dep); - return ret; - } - } + ret = _add_dep_list(handle, config->assumeinstalled, + &alpm_option_add_assumeinstalled, "assume-installed"); + if(ret != 0) { + return ret; + } + ret = _add_dep_list(handle, config->ignorepkg, &_add_ignorepkg, + "ignorepkg"); + if(ret != 0) { + return ret; + }
return 0; } diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index 3ff0b0c..fd9999c 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -57,6 +57,11 @@ TESTS += test/pacman/tests/ignore005.py TESTS += test/pacman/tests/ignore006.py TESTS += test/pacman/tests/ignore007.py TESTS += test/pacman/tests/ignore008.py +TESTS += test/pacman/tests/ignore009.py +TESTS += test/pacman/tests/ignore010.py +TESTS += test/pacman/tests/ignore011.py +TESTS += test/pacman/tests/ignore012.py +TESTS += test/pacman/tests/ignore013.py TESTS += test/pacman/tests/ldconfig001.py TESTS += test/pacman/tests/ldconfig002.py TESTS += test/pacman/tests/ldconfig003.py diff --git a/test/pacman/tests/ignore009.py b/test/pacman/tests/ignore009.py new file mode 100644 index 0000000..52125c8 --- /dev/null +++ b/test/pacman/tests/ignore009.py @@ -0,0 +1,14 @@ +self.description = "Sync with specific ignored package version" + +package1 = pmpkg("package1", "1.0-1") +self.addpkg2db("local", package1) + +package1up = pmpkg("package1", "1.1-1") +self.addpkg2db("sync", package1up) + +self.option["IgnorePkg"] = ["package1=1.1-1"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.0-1") + diff --git a/test/pacman/tests/ignore010.py b/test/pacman/tests/ignore010.py new file mode 100644 index 0000000..c96c9de --- /dev/null +++ b/test/pacman/tests/ignore010.py @@ -0,0 +1,13 @@ +self.description = "Sync with invalid bound on IgnorePkg" + +package1 = pmpkg("package1", "1.0-1") +self.addpkg2db("local", package1) + +package1up = pmpkg("package1", "1.1-1") +self.addpkg2db("sync", package1up) + +self.option["IgnorePkg"] = ["package1<1.1-1"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.expectfailure = True diff --git a/test/pacman/tests/ignore011.py b/test/pacman/tests/ignore011.py new file mode 100644 index 0000000..4fae819 --- /dev/null +++ b/test/pacman/tests/ignore011.py @@ -0,0 +1,13 @@ +self.description = "Sync with specific ignored, non-matching package version" + +package1 = pmpkg("package1", "1.0-1") +self.addpkg2db("local", package1) + +package1up = pmpkg("package1", "1.2-1") +self.addpkg2db("sync", package1up) + +self.option["IgnorePkg"] = ["package1=1.1-1"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.2-1") diff --git a/test/pacman/tests/ignore012.py b/test/pacman/tests/ignore012.py new file mode 100644 index 0000000..ffbcc5e --- /dev/null +++ b/test/pacman/tests/ignore012.py @@ -0,0 +1,13 @@ +self.description = "Sync with ignored package version and fnmatch" + +package1 = pmpkg("package1", "1.0-1") +self.addpkg2db("local", package1) + +package1up = pmpkg("package1", "1.1-1") +self.addpkg2db("sync", package1up) + +self.option["IgnorePkg"] = ["packag*=1.1-1"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.0-1") diff --git a/test/pacman/tests/ignore013.py b/test/pacman/tests/ignore013.py new file mode 100644 index 0000000..9a8edfc --- /dev/null +++ b/test/pacman/tests/ignore013.py @@ -0,0 +1,13 @@ +self.description = "Sync with ignored package version without pkgrel" + +package1 = pmpkg("package1", "1.0-1") +self.addpkg2db("local", package1) + +package1up = pmpkg("package1", "1.1-1") +self.addpkg2db("sync", package1up) + +self.option["IgnorePkg"] = ["package1=1.1"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.0-1")
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 daniel@schoepe.org
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 inIgnorePkg.
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
pacman-dev@lists.archlinux.org