[pacman-dev] sodeps (yes, again)
So some months ago Xavier posted a link to his patches for pacman (-Sdd). Back then Allan didn't have enough time to take a look. Later I asked him directly and he told me to let Dan review them. Dan told me he would like to hear Allan's opinion first. I'll try again and please someone just review them. I've cherry-picked them to one central location [1] and I'll also post them in this thread for easier quoting. In case you don't want to include sodeps support in pacman I'll accept that, but *please* tell me so I don't waste my time trying to get answers. [1] http://git.server-speed.net/users/flo/pacman/log/?h=sodeps -- Florian Pritz -- {flo,bluewind}@server-speed.net
From: Xavier Chantry <chantry.xavier@gmail.com> Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com> --- test/pacman/tests/sync-nodepversion01.py | 18 ++++++++++++++++++ test/pacman/tests/sync-nodepversion02.py | 15 +++++++++++++++ test/pacman/tests/sync-nodepversion03.py | 15 +++++++++++++++ test/pacman/tests/sync-nodepversion04.py | 17 +++++++++++++++++ test/pacman/tests/sync-nodepversion05.py | 19 +++++++++++++++++++ test/pacman/tests/sync-nodepversion06.py | 19 +++++++++++++++++++ 6 files changed, 103 insertions(+), 0 deletions(-) create mode 100644 test/pacman/tests/sync-nodepversion01.py create mode 100644 test/pacman/tests/sync-nodepversion02.py create mode 100644 test/pacman/tests/sync-nodepversion03.py create mode 100644 test/pacman/tests/sync-nodepversion04.py create mode 100644 test/pacman/tests/sync-nodepversion05.py create mode 100644 test/pacman/tests/sync-nodepversion06.py diff --git a/test/pacman/tests/sync-nodepversion01.py b/test/pacman/tests/sync-nodepversion01.py new file mode 100644 index 0000000..3db445f --- /dev/null +++ b/test/pacman/tests/sync-nodepversion01.py @@ -0,0 +1,18 @@ +self.description = "nodepversion: -Sdd works" + +p1 = pmpkg("pkg1", "1.0-2") +p1.depends = ["provision>1.0-1"] +self.addpkg2db("sync", p1) + +p2 = pmpkg("pkg2", "1.0-2") +p2.provides = ["provision=1.0-1"] +self.addpkg2db("sync", p2) + +self.args = "-Sdd %s" % p1.name + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_EXIST=pkg1") +self.addrule("PKG_EXIST=pkg2") +self.addrule("PKG_DEPENDS=pkg1|provision>1.0-1") + +self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion02.py b/test/pacman/tests/sync-nodepversion02.py new file mode 100644 index 0000000..6ebbdb7 --- /dev/null +++ b/test/pacman/tests/sync-nodepversion02.py @@ -0,0 +1,15 @@ +self.description = "nodepversion: -S fails" + +p1 = pmpkg("pkg1", "1.0-2") +p1.depends = ["provision>=1.0-2"] +self.addpkg2db("sync", p1) + +p2 = pmpkg("pkg2", "1.0-2") +p2.provides = ["provision=1.0-1"] +self.addpkg2db("sync", p2) + +self.args = "-S %s" % p1.name + +self.addrule("PACMAN_RETCODE=1") +self.addrule("!PKG_EXIST=pkg1") +self.addrule("!PKG_EXIST=pkg2") diff --git a/test/pacman/tests/sync-nodepversion03.py b/test/pacman/tests/sync-nodepversion03.py new file mode 100644 index 0000000..8ebb1c8 --- /dev/null +++ b/test/pacman/tests/sync-nodepversion03.py @@ -0,0 +1,15 @@ +self.description = "nodepversion: -Sd works but no deps" + +p1 = pmpkg("pkg1", "1.0-2") +p1.depends = ["provision>=1.0-2"] +self.addpkg2db("sync", p1) + +p2 = pmpkg("pkg2", "1.0-2") +p2.provides = ["provision=1.0-1"] +self.addpkg2db("sync", p2) + +self.args = "-Sd %s" % p1.name + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_EXIST=pkg1") +self.addrule("!PKG_EXIST=pkg2") diff --git a/test/pacman/tests/sync-nodepversion04.py b/test/pacman/tests/sync-nodepversion04.py new file mode 100644 index 0000000..f5a091e --- /dev/null +++ b/test/pacman/tests/sync-nodepversion04.py @@ -0,0 +1,17 @@ +self.description = "nodepversion: provision does not exist" + +p1 = pmpkg("pkg1", "1.0-2") +p1.depends = ["invalid>=1.0-2"] +self.addpkg2db("sync", p1) + +p2 = pmpkg("pkg2", "1.0-2") +p2.provides = ["provision=1.0-1"] +self.addpkg2db("sync", p2) + +self.args = "-Sdd %s" % p1.name + +self.addrule("PACMAN_RETCODE=1") +self.addrule("!PKG_EXIST=pkg1") +self.addrule("!PKG_EXIST=pkg2") + +self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion05.py b/test/pacman/tests/sync-nodepversion05.py new file mode 100644 index 0000000..f2a45f0 --- /dev/null +++ b/test/pacman/tests/sync-nodepversion05.py @@ -0,0 +1,19 @@ +self.description = "nodepversion: -Sudd works" + +p1 = pmpkg("pkg1", "1.0-1") +p1.depends = ["provision=1.0"] +self.addpkg2db("local", p1) + +p2 = pmpkg("pkg2", "1.0-1") +p2.provides = ["provision=1.0"] +self.addpkg2db("local", p2) + +sp2 = pmpkg("pkg2", "1.1-1") +sp2.provides = ["provision=1.1"] +self.addpkg2db("sync", sp2) + +self.args = "-Sudd" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=pkg1|1.0-1") +self.addrule("PKG_VERSION=pkg2|1.1-1") diff --git a/test/pacman/tests/sync-nodepversion06.py b/test/pacman/tests/sync-nodepversion06.py new file mode 100644 index 0000000..139b87a --- /dev/null +++ b/test/pacman/tests/sync-nodepversion06.py @@ -0,0 +1,19 @@ +self.description = "nodepversion: -Su fails" + +p1 = pmpkg("pkg1", "1.0-1") +p1.depends = ["provision=1.0"] +self.addpkg2db("local", p1) + +p2 = pmpkg("pkg2", "1.0-1") +p2.provides = ["provision=1.0"] +self.addpkg2db("local", p2) + +sp2 = pmpkg("pkg2", "1.1-1") +sp2.provides = ["provision=1.1"] +self.addpkg2db("sync", sp2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=pkg1|1.0-1") +self.addrule("PKG_VERSION=pkg2|1.0-1") -- 1.7.3.5
From: Xavier Chantry <chantry.xavier@gmail.com> This flag allows to disable version checking in dependency resolving code. depcmp_tolerant respects the NODEPVERSION flag but we still keep the original strict depcmp. The idea is to reduce the impact of the NODEPVERSION flag by using it in fewer places. I replaced almost all depcmp calls by depcmp_tolerant in deps.c (except in the public find_satisfier used by deptest / pacman -T), but I kept depcmp in sync.c and conflict.c Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com> --- lib/libalpm/alpm.h | 2 +- lib/libalpm/deps.c | 43 +++++++++++++++++++++++++++++++++++-------- lib/libalpm/deps.h | 1 + 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 7c4cd48..66258f8 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -275,7 +275,7 @@ typedef enum _pmtransflag_t { PM_TRANS_FLAG_NODEPS = 1, PM_TRANS_FLAG_FORCE = (1 << 1), PM_TRANS_FLAG_NOSAVE = (1 << 2), - /* (1 << 3) flag can go here */ + PM_TRANS_FLAG_NODEPVERSION = (1 << 3), PM_TRANS_FLAG_CASCADE = (1 << 4), PM_TRANS_FLAG_RECURSE = (1 << 5), PM_TRANS_FLAG_DBONLY = (1 << 6), diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 3d4b1df..3c057a6 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -195,7 +195,7 @@ pmpkg_t *_alpm_find_dep_satisfier(alpm_list_t *pkgs, pmdepend_t *dep) for(i = pkgs; i; i = alpm_list_next(i)) { pmpkg_t *pkg = i->data; - if(_alpm_depcmp(pkg, dep)) { + if(_alpm_depcmp_tolerant(pkg, dep)) { return(pkg); } } @@ -319,12 +319,20 @@ static int dep_vercmp(const char *version1, pmdepmod_t mod, return(equal); } -int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) +/* nodepversion: skip version checking */ +static int _depcmp(pmpkg_t *pkg, pmdepend_t *dep, int nodepversion) { alpm_list_t *i; const char *pkgname = pkg->name; const char *pkgversion = pkg->version; int satisfy = 0; + int depmod; + + if(nodepversion) { + depmod = PM_DEP_MOD_ANY; + } else { + depmod = dep->mod; + } /* check (pkg->name, pkg->version) */ if(pkg->name_hash && dep->name_hash @@ -332,7 +340,7 @@ int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) /* skip more expensive checks */ } else { satisfy = (strcmp(pkgname, dep->name) == 0 - && dep_vercmp(pkgversion, dep->mod, dep->version)); + && dep_vercmp(pkgversion, depmod, dep->version)); if(satisfy) { return(satisfy); } @@ -344,7 +352,7 @@ int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) const char *provver = strchr(provision, '='); if(provver == NULL) { /* no provision version */ - satisfy = (dep->mod == PM_DEP_MOD_ANY + satisfy = (depmod == PM_DEP_MOD_ANY && strcmp(provision, dep->name) == 0); } else { /* This is a bit tricker than the old code for performance reasons. To @@ -356,13 +364,32 @@ int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) provver += 1; satisfy = (strlen(dep->name) == namelen && strncmp(provision, dep->name, namelen) == 0 - && dep_vercmp(provver, dep->mod, dep->version)); + && dep_vercmp(provver, depmod, dep->version)); } } return(satisfy); } +/* tolerant : respects NODEPVERSION flag */ +int _alpm_depcmp_tolerant(pmpkg_t *pkg, pmdepend_t *dep) +{ + int nodepversion = 0; + int flags = alpm_trans_get_flags(); + + if (flags != -1) { + nodepversion = flags & PM_TRANS_FLAG_NODEPVERSION; + } + + return(_depcmp(pkg, dep, nodepversion)); +} + +/* strict : ignores NODEPVERSION flag */ +int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) +{ + return(_depcmp(pkg, dep, 0)); +} + pmdepend_t *_alpm_splitdep(const char *depstring) { pmdepend_t *depend; @@ -527,7 +554,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, /* 1. literals */ for(i = dbs; i; i = i->next) { pmpkg_t *pkg = _alpm_db_get_pkgfromcache(i->data, dep->name); - if(pkg && _alpm_depcmp(pkg, dep) && !_alpm_pkg_find(excluding, pkg->name)) { + if(pkg && _alpm_depcmp_tolerant(pkg, dep) && !_alpm_pkg_find(excluding, pkg->name)) { if(_alpm_pkg_should_ignore(pkg)) { int install = 0; if (prompt) { @@ -548,7 +575,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, for(i = dbs; i; i = i->next) { for(j = _alpm_db_get_pkgcache(i->data); j; j = j->next) { pmpkg_t *pkg = j->data; - if(_alpm_depcmp(pkg, dep) && strcmp(pkg->name, dep->name) != 0 && + if(_alpm_depcmp_tolerant(pkg, dep) && strcmp(pkg->name, dep->name) != 0 && !_alpm_pkg_find(excluding, pkg->name)) { if(_alpm_pkg_should_ignore(pkg)) { int install = 0; @@ -674,7 +701,7 @@ int _alpm_dep_edge(pmpkg_t *pkg1, pmpkg_t *pkg2) { alpm_list_t *i; for(i = alpm_pkg_get_depends(pkg1); i; i = i->next) { - if(_alpm_depcmp(pkg2, i->data)) { + if(_alpm_depcmp_tolerant(pkg2, i->data)) { return(1); } } diff --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h index bd5e9a4..86070ab 100644 --- a/lib/libalpm/deps.h +++ b/lib/libalpm/deps.h @@ -57,6 +57,7 @@ int _alpm_dep_edge(pmpkg_t *pkg1, pmpkg_t *pkg2); pmdepend_t *_alpm_splitdep(const char *depstring); pmpkg_t *_alpm_find_dep_satisfier(alpm_list_t *pkgs, pmdepend_t *dep); int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep); +int _alpm_depcmp_tolerant(pmpkg_t *pkg, pmdepend_t *dep); #endif /* _ALPM_DEPS_H */ -- 1.7.3.5
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
From: Xavier Chantry <chantry.xavier@gmail.com>
This flag allows to disable version checking in dependency resolving code.
depcmp_tolerant respects the NODEPVERSION flag but we still keep the original strict depcmp. The idea is to reduce the impact of the NODEPVERSION flag by using it in fewer places.
I replaced almost all depcmp calls by depcmp_tolerant in deps.c (except in the public find_satisfier used by deptest / pacman -T), but I kept depcmp in sync.c and conflict.c
Seems mostly OK, but without digging in I don't really understand why some get changed and some do not.
Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com> --- lib/libalpm/alpm.h | 2 +- lib/libalpm/deps.c | 43 +++++++++++++++++++++++++++++++++++-------- lib/libalpm/deps.h | 1 + 3 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 7c4cd48..66258f8 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -275,7 +275,7 @@ typedef enum _pmtransflag_t { PM_TRANS_FLAG_NODEPS = 1, PM_TRANS_FLAG_FORCE = (1 << 1), PM_TRANS_FLAG_NOSAVE = (1 << 2), - /* (1 << 3) flag can go here */ + PM_TRANS_FLAG_NODEPVERSION = (1 << 3), PM_TRANS_FLAG_CASCADE = (1 << 4), PM_TRANS_FLAG_RECURSE = (1 << 5), PM_TRANS_FLAG_DBONLY = (1 << 6), diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 3d4b1df..3c057a6 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -195,7 +195,7 @@ pmpkg_t *_alpm_find_dep_satisfier(alpm_list_t *pkgs, pmdepend_t *dep)
for(i = pkgs; i; i = alpm_list_next(i)) { pmpkg_t *pkg = i->data; - if(_alpm_depcmp(pkg, dep)) { + if(_alpm_depcmp_tolerant(pkg, dep)) { return(pkg); } } @@ -319,12 +319,20 @@ static int dep_vercmp(const char *version1, pmdepmod_t mod, return(equal); }
-int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) +/* nodepversion: skip version checking */ +static int _depcmp(pmpkg_t *pkg, pmdepend_t *dep, int nodepversion) { alpm_list_t *i; const char *pkgname = pkg->name; const char *pkgversion = pkg->version; int satisfy = 0; + int depmod; + + if(nodepversion) { + depmod = PM_DEP_MOD_ANY; + } else { + depmod = dep->mod; + }
/* check (pkg->name, pkg->version) */ if(pkg->name_hash && dep->name_hash @@ -332,7 +340,7 @@ int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) /* skip more expensive checks */ } else { satisfy = (strcmp(pkgname, dep->name) == 0 - && dep_vercmp(pkgversion, dep->mod, dep->version)); + && dep_vercmp(pkgversion, depmod, dep->version)); if(satisfy) { return(satisfy); } @@ -344,7 +352,7 @@ int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) const char *provver = strchr(provision, '=');
if(provver == NULL) { /* no provision version */ - satisfy = (dep->mod == PM_DEP_MOD_ANY + satisfy = (depmod == PM_DEP_MOD_ANY && strcmp(provision, dep->name) == 0); } else { /* This is a bit tricker than the old code for performance reasons. To @@ -356,13 +364,32 @@ int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) provver += 1; satisfy = (strlen(dep->name) == namelen && strncmp(provision, dep->name, namelen) == 0 - && dep_vercmp(provver, dep->mod, dep->version)); + && dep_vercmp(provver, depmod, dep->version)); } }
return(satisfy); }
+/* tolerant : respects NODEPVERSION flag */ +int _alpm_depcmp_tolerant(pmpkg_t *pkg, pmdepend_t *dep) +{ + int nodepversion = 0; + int flags = alpm_trans_get_flags(); + + if (flags != -1) { + nodepversion = flags & PM_TRANS_FLAG_NODEPVERSION; + } + + return(_depcmp(pkg, dep, nodepversion)); +} + +/* strict : ignores NODEPVERSION flag */ +int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep) +{ + return(_depcmp(pkg, dep, 0)); +} + pmdepend_t *_alpm_splitdep(const char *depstring) { pmdepend_t *depend; @@ -527,7 +554,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, /* 1. literals */ for(i = dbs; i; i = i->next) { pmpkg_t *pkg = _alpm_db_get_pkgfromcache(i->data, dep->name); - if(pkg && _alpm_depcmp(pkg, dep) && !_alpm_pkg_find(excluding, pkg->name)) { + if(pkg && _alpm_depcmp_tolerant(pkg, dep) && !_alpm_pkg_find(excluding, pkg->name)) { if(_alpm_pkg_should_ignore(pkg)) { int install = 0; if (prompt) { @@ -548,7 +575,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, for(i = dbs; i; i = i->next) { for(j = _alpm_db_get_pkgcache(i->data); j; j = j->next) { pmpkg_t *pkg = j->data; - if(_alpm_depcmp(pkg, dep) && strcmp(pkg->name, dep->name) != 0 && + if(_alpm_depcmp_tolerant(pkg, dep) && strcmp(pkg->name, dep->name) != 0 && !_alpm_pkg_find(excluding, pkg->name)) { if(_alpm_pkg_should_ignore(pkg)) { int install = 0; @@ -674,7 +701,7 @@ int _alpm_dep_edge(pmpkg_t *pkg1, pmpkg_t *pkg2) { alpm_list_t *i; for(i = alpm_pkg_get_depends(pkg1); i; i = i->next) { - if(_alpm_depcmp(pkg2, i->data)) { + if(_alpm_depcmp_tolerant(pkg2, i->data)) { return(1); } } diff --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h index bd5e9a4..86070ab 100644 --- a/lib/libalpm/deps.h +++ b/lib/libalpm/deps.h @@ -57,6 +57,7 @@ int _alpm_dep_edge(pmpkg_t *pkg1, pmpkg_t *pkg2); pmdepend_t *_alpm_splitdep(const char *depstring); pmpkg_t *_alpm_find_dep_satisfier(alpm_list_t *pkgs, pmdepend_t *dep); int _alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep); +int _alpm_depcmp_tolerant(pmpkg_t *pkg, pmdepend_t *dep);
#endif /* _ALPM_DEPS_H */
-- 1.7.3.5
On Wed, Jan 19, 2011 at 7:57 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
From: Xavier Chantry <chantry.xavier@gmail.com>
This flag allows to disable version checking in dependency resolving code.
depcmp_tolerant respects the NODEPVERSION flag but we still keep the original strict depcmp. The idea is to reduce the impact of the NODEPVERSION flag by using it in fewer places.
I replaced almost all depcmp calls by depcmp_tolerant in deps.c (except in the public find_satisfier used by deptest / pacman -T), but I kept depcmp in sync.c and conflict.c
Seems mostly OK, but without digging in I don't really understand why some get changed and some do not.
Reading this again, the choice in find_satisfier probably does not matter, since I doubt pacman -T specifies NODEPVERSION. And if it could, well why not. So that one could be changed. Otherwise, the problem is that we use depcmp both for dep checking and conflict checking. We (Nagy, me) thought that nodepversion should only appy to versioned dep, and not versioned conflict, which are separate. But to be honest, I am not sure what's best. IIRC it's Nagy who first mentioned he did not like that nodepversion had an effect on all depcmp usage in the first version of the patch.
-dd ignores only the version of a dependency being checked, but not the package itself. Signed-off-by: Florian Pritz <bluewind@xssn.at> Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com> --- doc/pacman.8.txt | 1 + src/pacman/pacman.c | 11 ++++++++++- test/pacman/tests/sync-nodepversion01.py | 2 -- test/pacman/tests/sync-nodepversion04.py | 2 -- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 2b47a88..735bff9 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -155,6 +155,7 @@ Transaction Options (apply to '-S', '-R' and '-U') Skips all dependency checks. Normally, pacman will always check a package's dependency fields to ensure that all dependencies are installed and there are no package conflicts in the system. + Specify this option twice to skip the version checking only. *-k, \--dbonly*:: Adds/Removes the database entry only, leaves all files in place. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..4074b84 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -552,8 +552,17 @@ static int parsearg_query(int opt) /* options common to -S -R -U */ static int parsearg_trans(int opt) { + static int nodeps = 0; switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + nodeps++; + if(nodeps == 1) { + config->flags |= PM_TRANS_FLAG_NODEPS; + } else if(nodeps == 2) { + config->flags ^= PM_TRANS_FLAG_NODEPS; + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; diff --git a/test/pacman/tests/sync-nodepversion01.py b/test/pacman/tests/sync-nodepversion01.py index 3db445f..734ac98 100644 --- a/test/pacman/tests/sync-nodepversion01.py +++ b/test/pacman/tests/sync-nodepversion01.py @@ -14,5 +14,3 @@ self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") self.addrule("PKG_DEPENDS=pkg1|provision>1.0-1") - -self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion04.py b/test/pacman/tests/sync-nodepversion04.py index f5a091e..fdd3481 100644 --- a/test/pacman/tests/sync-nodepversion04.py +++ b/test/pacman/tests/sync-nodepversion04.py @@ -13,5 +13,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True -- 1.7.3.5
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
-dd ignores only the version of a dependency being checked, but not the package itself.
Signed-off-by: Florian Pritz <bluewind@xssn.at> Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com>
I don't mind this, but it just seems...backwards. Specifying more flags should make it less-restrictive, not more restrictive, but I understand the desire to keep backward compatibility. With that said, does it matter? People that use -d very often are usually screwing their system or know what they are doing- should we make -d just skip versions and -dd skip everything? -Dan
--- doc/pacman.8.txt | 1 + src/pacman/pacman.c | 11 ++++++++++- test/pacman/tests/sync-nodepversion01.py | 2 -- test/pacman/tests/sync-nodepversion04.py | 2 -- 4 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 2b47a88..735bff9 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -155,6 +155,7 @@ Transaction Options (apply to '-S', '-R' and '-U') Skips all dependency checks. Normally, pacman will always check a package's dependency fields to ensure that all dependencies are installed and there are no package conflicts in the system. + Specify this option twice to skip the version checking only.
*-k, \--dbonly*:: Adds/Removes the database entry only, leaves all files in place. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..4074b84 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -552,8 +552,17 @@ static int parsearg_query(int opt) /* options common to -S -R -U */ static int parsearg_trans(int opt) { + static int nodeps = 0; switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + nodeps++; + if(nodeps == 1) { + config->flags |= PM_TRANS_FLAG_NODEPS; + } else if(nodeps == 2) { + config->flags ^= PM_TRANS_FLAG_NODEPS; + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; You can do this without a static local; look at "case 's'" in parsearg_remove().
case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; diff --git a/test/pacman/tests/sync-nodepversion01.py b/test/pacman/tests/sync-nodepversion01.py index 3db445f..734ac98 100644 --- a/test/pacman/tests/sync-nodepversion01.py +++ b/test/pacman/tests/sync-nodepversion01.py @@ -14,5 +14,3 @@ self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") self.addrule("PKG_DEPENDS=pkg1|provision>1.0-1") - -self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion04.py b/test/pacman/tests/sync-nodepversion04.py index f5a091e..fdd3481 100644 --- a/test/pacman/tests/sync-nodepversion04.py +++ b/test/pacman/tests/sync-nodepversion04.py @@ -13,5 +13,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True -- 1.7.3.5
On 19.01.2011 19:29, Dan McGee wrote:
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
-dd ignores only the version of a dependency being checked, but not the package itself.
I don't mind this, but it just seems...backwards. Specifying more flags should make it less-restrictive, not more restrictive, but I understand the desire to keep backward compatibility. With that said, does it matter? People that use -d very often are usually screwing their system or know what they are doing- should we make -d just skip versions and -dd skip everything?
static int parsearg_trans(int opt) { + static int nodeps = 0; switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + nodeps++; + if(nodeps == 1) { + config->flags |= PM_TRANS_FLAG_NODEPS; + } else if(nodeps == 2) { + config->flags ^= PM_TRANS_FLAG_NODEPS; + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; You can do this without a static local; look at "case 's'" in parsearg_remove().
I'll wait for a decision about -d and -dd before fixing that. -- Florian Pritz -- {flo,bluewind}@server-speed.net
On Thu, Jan 20, 2011 at 12:20 PM, Florian Pritz <bluewind@server-speed.net> wrote:
On 19.01.2011 19:29, Dan McGee wrote:
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
-dd ignores only the version of a dependency being checked, but not the package itself.
I don't mind this, but it just seems...backwards. Specifying more flags should make it less-restrictive, not more restrictive, but I understand the desire to keep backward compatibility. With that said, does it matter? People that use -d very often are usually screwing their system or know what they are doing- should we make -d just skip versions and -dd skip everything?
static int parsearg_trans(int opt) { + static int nodeps = 0; switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + nodeps++; + if(nodeps == 1) { + config->flags |= PM_TRANS_FLAG_NODEPS; + } else if(nodeps == 2) { + config->flags ^= PM_TRANS_FLAG_NODEPS; + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; You can do this without a static local; look at "case 's'" in parsearg_remove().
I'll wait for a decision about -d and -dd before fixing that.
Anyone else want to offer an opinion on this? I'd like to reverse the two options to match what we do elsewhere- -d will now just ignore versions, -dd everything. -Dan
On 25/01/11 23:03, Dan McGee wrote:
On Thu, Jan 20, 2011 at 12:20 PM, Florian Pritz <bluewind@server-speed.net> wrote:
On 19.01.2011 19:29, Dan McGee wrote:
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz<bluewind@xssn.at> wrote:
-dd ignores only the version of a dependency being checked, but not the package itself.
I don't mind this, but it just seems...backwards. Specifying more flags should make it less-restrictive, not more restrictive, but I understand the desire to keep backward compatibility. With that said, does it matter? People that use -d very often are usually screwing their system or know what they are doing- should we make -d just skip versions and -dd skip everything?
static int parsearg_trans(int opt) { + static int nodeps = 0; switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + nodeps++; + if(nodeps == 1) { + config->flags |= PM_TRANS_FLAG_NODEPS; + } else if(nodeps == 2) { + config->flags ^= PM_TRANS_FLAG_NODEPS; + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; You can do this without a static local; look at "case 's'" in parsearg_remove().
I'll wait for a decision about -d and -dd before fixing that.
Anyone else want to offer an opinion on this? I'd like to reverse the two options to match what we do elsewhere- -d will now just ignore versions, -dd everything.
That seems reasonable. With -d getting stricter in terms of dependency checks, there should not be any issues caused by this change going unnoticed by people who currently using -d. Allan
From: Florian Pritz <bluewind@xssn.at> -d skips checking the version of a dependency. -dd skips the whole dependency check Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com> Signed-off-by: Florian Pritz <bluewind@server-speed.net> --- doc/pacman.8.txt | 7 ++++--- src/pacman/pacman.c | 9 ++++++++- test/pacman/tests/sync-nodepversion01.py | 6 ++---- test/pacman/tests/sync-nodepversion03.py | 4 ++-- test/pacman/tests/sync-nodepversion04.py | 4 +--- test/pacman/tests/sync-nodepversion05.py | 4 ++-- test/pacman/tests/sync045.py | 4 ++-- test/pacman/tests/upgrade072.py | 2 +- 8 files changed, 22 insertions(+), 18 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 2b47a88..4ce8fa2 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -152,9 +152,10 @@ Options Transaction Options (apply to '-S', '-R' and '-U') -------------------------------------------------- *-d, \--nodeps*:: - Skips all dependency checks. Normally, pacman will always check a - package's dependency fields to ensure that all dependencies are - installed and there are no package conflicts in the system. + Skips dependency version checks. Package names are still checked Normally, + pacman will always check a package's dependency fields to ensure that all + dependencies are installed and there are no package conflicts in the + system. Specify this option twice to skip all dependency checks. *-k, \--dbonly*:: Adds/Removes the database entry only, leaves all files in place. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..363b167 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -553,7 +553,14 @@ static int parsearg_query(int opt) static int parsearg_trans(int opt) { switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + if(config->flags & PM_TRANS_FLAG_NODEPVERSION) { + config->flags ^= PM_TRANS_FLAG_NODEPVERSION; + config->flags |= PM_TRANS_FLAG_NODEPS; + } else { + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; diff --git a/test/pacman/tests/sync-nodepversion01.py b/test/pacman/tests/sync-nodepversion01.py index 3db445f..5829b9e 100644 --- a/test/pacman/tests/sync-nodepversion01.py +++ b/test/pacman/tests/sync-nodepversion01.py @@ -1,4 +1,4 @@ -self.description = "nodepversion: -Sdd works" +self.description = "nodepversion: -Sd works" p1 = pmpkg("pkg1", "1.0-2") p1.depends = ["provision>1.0-1"] @@ -8,11 +8,9 @@ p2.provides = ["provision=1.0-1"] self.addpkg2db("sync", p2) -self.args = "-Sdd %s" % p1.name +self.args = "-Sd %s" % p1.name self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") self.addrule("PKG_DEPENDS=pkg1|provision>1.0-1") - -self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion03.py b/test/pacman/tests/sync-nodepversion03.py index 8ebb1c8..378ff76 100644 --- a/test/pacman/tests/sync-nodepversion03.py +++ b/test/pacman/tests/sync-nodepversion03.py @@ -1,4 +1,4 @@ -self.description = "nodepversion: -Sd works but no deps" +self.description = "nodepversion: -Sdd works but no deps" p1 = pmpkg("pkg1", "1.0-2") p1.depends = ["provision>=1.0-2"] @@ -8,7 +8,7 @@ p2.provides = ["provision=1.0-1"] self.addpkg2db("sync", p2) -self.args = "-Sd %s" % p1.name +self.args = "-Sdd %s" % p1.name self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") diff --git a/test/pacman/tests/sync-nodepversion04.py b/test/pacman/tests/sync-nodepversion04.py index f5a091e..2bf83bb 100644 --- a/test/pacman/tests/sync-nodepversion04.py +++ b/test/pacman/tests/sync-nodepversion04.py @@ -8,10 +8,8 @@ p2.provides = ["provision=1.0-1"] self.addpkg2db("sync", p2) -self.args = "-Sdd %s" % p1.name +self.args = "-Sd %s" % p1.name self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion05.py b/test/pacman/tests/sync-nodepversion05.py index f2a45f0..bc048db 100644 --- a/test/pacman/tests/sync-nodepversion05.py +++ b/test/pacman/tests/sync-nodepversion05.py @@ -1,4 +1,4 @@ -self.description = "nodepversion: -Sudd works" +self.description = "nodepversion: -Sud works" p1 = pmpkg("pkg1", "1.0-1") p1.depends = ["provision=1.0"] @@ -12,7 +12,7 @@ sp2.provides = ["provision=1.1"] self.addpkg2db("sync", sp2) -self.args = "-Sudd" +self.args = "-Sud" self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=pkg1|1.0-1") diff --git a/test/pacman/tests/sync045.py b/test/pacman/tests/sync045.py index 574c0a5..6c31983 100644 --- a/test/pacman/tests/sync045.py +++ b/test/pacman/tests/sync045.py @@ -1,4 +1,4 @@ -self.description = "Install a sync package conflicting with two local ones (-d)" +self.description = "Install a sync package conflicting with two local ones (-dd)" sp = pmpkg("pkg1") sp.conflicts = ["pkg2", "pkg3"] @@ -10,7 +10,7 @@ lp2 = pmpkg("pkg3") self.addpkg2db("local", lp2); -self.args = "-Sd %s --ask=4" % sp.name +self.args = "-Sdd %s --ask=4" % sp.name self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") diff --git a/test/pacman/tests/upgrade072.py b/test/pacman/tests/upgrade072.py index f88e150..116103b 100644 --- a/test/pacman/tests/upgrade072.py +++ b/test/pacman/tests/upgrade072.py @@ -6,7 +6,7 @@ p.depends = ["dep1"] self.addpkg(p) -self.args = "-Ud %s" % p.filename() +self.args = "-Udd %s" % p.filename() self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=dummy") -- 1.7.3.5
On Fri, Jan 28, 2011 at 2:11 AM, Florian Pritz <bluewind@server-speed.net> wrote:
From: Florian Pritz <bluewind@xssn.at>
-d skips checking the version of a dependency.
-dd skips the whole dependency check Periods are awesome, please use one. :)
I assume this follows the actual implementation of this in the backend, even though it has a patch 1/2 subject? Maybe resubmitting just the two -d/-dd patches as a single set would be good to make sure I know what to apply here.
Signed-off-by: Xavier Chantry <chantry.xavier@gmail.com> Signed-off-by: Florian Pritz <bluewind@server-speed.net> --- doc/pacman.8.txt | 7 ++++--- src/pacman/pacman.c | 9 ++++++++- test/pacman/tests/sync-nodepversion01.py | 6 ++---- test/pacman/tests/sync-nodepversion03.py | 4 ++-- test/pacman/tests/sync-nodepversion04.py | 4 +--- test/pacman/tests/sync-nodepversion05.py | 4 ++-- test/pacman/tests/sync045.py | 4 ++-- test/pacman/tests/upgrade072.py | 2 +- 8 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 2b47a88..4ce8fa2 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -152,9 +152,10 @@ Options Transaction Options (apply to '-S', '-R' and '-U') -------------------------------------------------- *-d, \--nodeps*:: - Skips all dependency checks. Normally, pacman will always check a - package's dependency fields to ensure that all dependencies are - installed and there are no package conflicts in the system. + Skips dependency version checks. Package names are still checked Normally,
Missing period.
+ pacman will always check a package's dependency fields to ensure that all + dependencies are installed and there are no package conflicts in the + system. Specify this option twice to skip all dependency checks. No double-space.
*-k, \--dbonly*:: Adds/Removes the database entry only, leaves all files in place. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..363b167 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -553,7 +553,14 @@ static int parsearg_query(int opt) static int parsearg_trans(int opt) { switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + if(config->flags & PM_TRANS_FLAG_NODEPVERSION) { + config->flags ^= PM_TRANS_FLAG_NODEPVERSION; + config->flags |= PM_TRANS_FLAG_NODEPS;
Why do we need to back the depversion flag out at all? Given that nodepversion is less strong than nodeps, it doesn't make intuitive sense to have to do this.
+ } else { + config->flags |= PM_TRANS_FLAG_NODEPVERSION; + } + break; case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break; case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; diff --git a/test/pacman/tests/sync-nodepversion01.py b/test/pacman/tests/sync-nodepversion01.py index 3db445f..5829b9e 100644 --- a/test/pacman/tests/sync-nodepversion01.py +++ b/test/pacman/tests/sync-nodepversion01.py @@ -1,4 +1,4 @@ -self.description = "nodepversion: -Sdd works" +self.description = "nodepversion: -Sd works"
p1 = pmpkg("pkg1", "1.0-2") p1.depends = ["provision>1.0-1"] @@ -8,11 +8,9 @@ p2.provides = ["provision=1.0-1"] self.addpkg2db("sync", p2)
-self.args = "-Sdd %s" % p1.name +self.args = "-Sd %s" % p1.name
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") self.addrule("PKG_DEPENDS=pkg1|provision>1.0-1") - -self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion03.py b/test/pacman/tests/sync-nodepversion03.py index 8ebb1c8..378ff76 100644 --- a/test/pacman/tests/sync-nodepversion03.py +++ b/test/pacman/tests/sync-nodepversion03.py @@ -1,4 +1,4 @@ -self.description = "nodepversion: -Sd works but no deps" +self.description = "nodepversion: -Sdd works but no deps"
p1 = pmpkg("pkg1", "1.0-2") p1.depends = ["provision>=1.0-2"] @@ -8,7 +8,7 @@ p2.provides = ["provision=1.0-1"] self.addpkg2db("sync", p2)
-self.args = "-Sd %s" % p1.name +self.args = "-Sdd %s" % p1.name
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") diff --git a/test/pacman/tests/sync-nodepversion04.py b/test/pacman/tests/sync-nodepversion04.py index f5a091e..2bf83bb 100644 --- a/test/pacman/tests/sync-nodepversion04.py +++ b/test/pacman/tests/sync-nodepversion04.py @@ -8,10 +8,8 @@ p2.provides = ["provision=1.0-1"] self.addpkg2db("sync", p2)
-self.args = "-Sdd %s" % p1.name +self.args = "-Sd %s" % p1.name
self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True diff --git a/test/pacman/tests/sync-nodepversion05.py b/test/pacman/tests/sync-nodepversion05.py index f2a45f0..bc048db 100644 --- a/test/pacman/tests/sync-nodepversion05.py +++ b/test/pacman/tests/sync-nodepversion05.py @@ -1,4 +1,4 @@ -self.description = "nodepversion: -Sudd works" +self.description = "nodepversion: -Sud works"
p1 = pmpkg("pkg1", "1.0-1") p1.depends = ["provision=1.0"] @@ -12,7 +12,7 @@ sp2.provides = ["provision=1.1"] self.addpkg2db("sync", sp2)
-self.args = "-Sudd" +self.args = "-Sud"
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=pkg1|1.0-1") diff --git a/test/pacman/tests/sync045.py b/test/pacman/tests/sync045.py index 574c0a5..6c31983 100644 --- a/test/pacman/tests/sync045.py +++ b/test/pacman/tests/sync045.py @@ -1,4 +1,4 @@ -self.description = "Install a sync package conflicting with two local ones (-d)" +self.description = "Install a sync package conflicting with two local ones (-dd)"
sp = pmpkg("pkg1") sp.conflicts = ["pkg2", "pkg3"] @@ -10,7 +10,7 @@ lp2 = pmpkg("pkg3") self.addpkg2db("local", lp2);
-self.args = "-Sd %s --ask=4" % sp.name +self.args = "-Sdd %s --ask=4" % sp.name
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") diff --git a/test/pacman/tests/upgrade072.py b/test/pacman/tests/upgrade072.py index f88e150..116103b 100644 --- a/test/pacman/tests/upgrade072.py +++ b/test/pacman/tests/upgrade072.py @@ -6,7 +6,7 @@ p.depends = ["dep1"] self.addpkg(p)
-self.args = "-Ud %s" % p.filename() +self.args = "-Udd %s" % p.filename()
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=dummy") -- 1.7.3.5
On 28.01.2011 20:48, Dan McGee wrote:
On Fri, Jan 28, 2011 at 2:11 AM, Florian Pritz <bluewind@server-speed.net> wrote:
From: Florian Pritz <bluewind@xssn.at>
-d skips checking the version of a dependency.
-dd skips the whole dependency check Periods are awesome, please use one. :)
Fixed.
I assume this follows the actual implementation of this in the backend, even though it has a patch 1/2 subject? Maybe resubmitting just the two -d/-dd patches as a single set would be good to make sure I know what to apply here.
Oh sorry I broke that when using format-patch. :( I didn't want to resubmit all 5 patches so I just exported 3 and 4 (old numbers). This one is the fixed 3 and "[PATCH 2/2] makepkg: add soprovides support" is the fixed 4.
- Skips all dependency checks. Normally, pacman will always check a - package's dependency fields to ensure that all dependencies are - installed and there are no package conflicts in the system. + Skips dependency version checks. Package names are still checked Normally, Missing period.
+ pacman will always check a package's dependency fields to ensure that all + dependencies are installed and there are no package conflicts in the + system. Specify this option twice to skip all dependency checks. No double-space.
Both fixed
*-k, \--dbonly*:: Adds/Removes the database entry only, leaves all files in place. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..363b167 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -553,7 +553,14 @@ static int parsearg_query(int opt) static int parsearg_trans(int opt) { switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + if(config->flags & PM_TRANS_FLAG_NODEPVERSION) { + config->flags ^= PM_TRANS_FLAG_NODEPVERSION; + config->flags |= PM_TRANS_FLAG_NODEPS; Why do we need to back the depversion flag out at all? Given that nodepversion is less strong than nodeps, it doesn't make intuitive sense to have to do this.
I haven't checked that yet, but if it's not needed I'll take it out. -- Florian Pritz -- {flo,bluewind}@server-speed.net
On Fri, Jan 28, 2011 at 1:59 PM, Florian Pritz <bluewind@server-speed.net> wrote:
On 28.01.2011 20:48, Dan McGee wrote:
On Fri, Jan 28, 2011 at 2:11 AM, Florian Pritz <bluewind@server-speed.net> wrote:
From: Florian Pritz <bluewind@xssn.at>
-d skips checking the version of a dependency.
-dd skips the whole dependency check Periods are awesome, please use one. :)
Fixed.
I assume this follows the actual implementation of this in the backend, even though it has a patch 1/2 subject? Maybe resubmitting just the two -d/-dd patches as a single set would be good to make sure I know what to apply here.
Oh sorry I broke that when using format-patch. :(
I didn't want to resubmit all 5 patches so I just exported 3 and 4 (old numbers). This one is the fixed 3 and "[PATCH 2/2] makepkg: add soprovides support" is the fixed 4.
- Skips all dependency checks. Normally, pacman will always check a - package's dependency fields to ensure that all dependencies are - installed and there are no package conflicts in the system. + Skips dependency version checks. Package names are still checked Normally, Missing period.
+ pacman will always check a package's dependency fields to ensure that all + dependencies are installed and there are no package conflicts in the + system. Specify this option twice to skip all dependency checks. No double-space.
Both fixed
*-k, \--dbonly*:: Adds/Removes the database entry only, leaves all files in place. diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..363b167 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -553,7 +553,14 @@ static int parsearg_query(int opt) static int parsearg_trans(int opt) { switch(opt) { - case 'd': config->flags |= PM_TRANS_FLAG_NODEPS; break; + case 'd': + if(config->flags & PM_TRANS_FLAG_NODEPVERSION) { + config->flags ^= PM_TRANS_FLAG_NODEPVERSION; + config->flags |= PM_TRANS_FLAG_NODEPS; Why do we need to back the depversion flag out at all? Given that nodepversion is less strong than nodeps, it doesn't make intuitive sense to have to do this.
I haven't checked that yet, but if it's not needed I'll take it out.
Test this yet? -Dan
Support-by: brain0 <thomas@archlinux.org> Support-by: GNU\caustic <Christoph.Schied@uni-ulm.de> Signed-off-by: Florian Pritz <bluewind@xssn.at> --- scripts/makepkg.sh.in | 38 +++++++++++++++++++++++++++++++++++++- 1 files changed, 37 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 6ebfac0..cf7dbb9 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -917,6 +917,28 @@ tidy_install() { fi } +find_soprovides() { + local soprovides + find $pkgdir -type f -name \*.so\* | while read filename + do + if readelf -h "$filename" 2>/dev/null | grep -q '.*Type:.*DYN (Shared object file).*'; then + soarch="$(objdump -a "$filename" 2>/dev/null | \ + sed -rn 's/.* file format elf[0-9]+-(.*)$/\1/p' | tr - _)_$(uname -s)" + sofile=$(readelf -d "$filename" 2>/dev/null | sed -nr 's/.*Library soname: \[(.*)\].*/\1/p') + [ -z "$sofile" ] && sofile="$(basename "$filename")" + + soname=$(sed -rn 's/(.*)\.so.*/\1.so/p' <<< "$sofile") + soversion=$(sed -rn 's/.*\.so\.(.*)/\1/p' <<< "$sofile") + if in_array "${soname}" ${provides[@]}; then + if ! in_array "${soname}=${soversion}-${soarch}" ${soprovides[@]}; then + echo "${soname}=${soversion}-${soarch}" + soprovides=(${soprovides[@]} "${soname}=${soversion}-${soarch}") + fi + fi + fi + done +} + write_pkginfo() { local builddate=$(date -u "+%s") if [[ -n $PACKAGER ]]; then @@ -950,10 +972,24 @@ write_pkginfo() { [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}" [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}" - [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" local it + + soprovides=$(find_soprovides) + provides=("${provides[@]}" ${soprovides}) + + for it in "${provides[@]}"; do + if grep -q ".*\.so$" <<< "$it"; then + if ! grep -q "\(^\|\s\)${it}=.*" <<< $soprovides; then + error "$(gettext "Can't find library listed in \$provides: %s")" "$it" + return 1 + fi + else + echo "provides = $it" + fi + done + for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" if [[ $ret != "?" ]]; then -- 1.7.3.5
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
<Descriptive commit message usually goes here>
What does this do? Why? What is the generated format? This stuff needs
to be here in permanent history so someone patching bugs 2 years from
now can figure out the what and why.
> Support-by: brain0 <thomas@archlinux.org>
> Support-by: GNU\caustic <Christoph.Schied@uni-ulm.de>
Since the real names of these guys are so obvious, I'd prefer those
are there rather than nicks.
>
> Signed-off-by: Florian Pritz <bluewind@xssn.at>
> ---
> scripts/makepkg.sh.in | 38 +++++++++++++++++++++++++++++++++++++-
> 1 files changed, 37 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 6ebfac0..cf7dbb9 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -917,6 +917,28 @@ tidy_install() {
> fi
> }
>
> +find_soprovides() {
> + local soprovides
> + find $pkgdir -type f -name \*.so\* | while read filename
$pkgdir needs quotes, just like everywhere else it is used.
> + do
> + if readelf -h "$filename" 2>/dev/null | grep -q '.*Type:.*DYN (Shared object file).*'; then
> + soarch="$(objdump -a "$filename" 2>/dev/null | \
> + sed -rn 's/.* file format elf[0-9]+-(.*)$/\1/p' | tr - _)_$(uname -s)"
> + sofile=$(readelf -d "$filename" 2>/dev/null | sed -nr 's/.*Library soname: \[(.*)\].*/\1/p')
> + [ -z "$sofile" ] && sofile="$(basename "$filename")"
> +
> + soname=$(sed -rn 's/(.*)\.so.*/\1.so/p' <<< "$sofile")
> + soversion=$(sed -rn 's/.*\.so\.(.*)/\1/p' <<< "$sofile")
Please give examples in a resubmit of what is coming out of here- this
is not super easy to follow. I'm also a bit concerned about:
* the uses of sed (is -r portable? We use -n already, but not -r)
* the inconsistencies between `sed -rn` and `sed -nr`
* running this in any non-C, non-en locale
* introducing dependencies on readelf and objdump. At least elsewhere
in the strip code, we use `file` to locate shared libraries. And you
can get the SONAME bit out of objdump -p, which would at least limit
this to one tool.
> + if in_array "${soname}" ${provides[@]}; then
> + if ! in_array "${soname}=${soversion}-${soarch}" ${soprovides[@]}; then
> + echo "${soname}=${soversion}-${soarch}"
> + soprovides=(${soprovides[@]} "${soname}=${soversion}-${soarch}")
What's the reasoning behind the ${soarch} append? I suppose it might
help with multilib, but something about this just doesn't seem right.
It is most definitely not a valid pkgver (dash) or pkgrel (not a
number).
> + fi
> + fi
> + fi
> + done
> +}
> +
> write_pkginfo() {
> local builddate=$(date -u "+%s")
> if [[ -n $PACKAGER ]]; then
> @@ -950,10 +972,24 @@ write_pkginfo() {
> [[ $depends ]] && printf "depend = %s\n" "${depends[@]}"
> [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}"
> [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}"
> - [[ $provides ]] && printf "provides = %s\n" "${provides[@]}"
> [[ $backup ]] && printf "backup = %s\n" "${backup[@]}"
>
> local it
> +
> + soprovides=$(find_soprovides)
> + provides=("${provides[@]}" ${soprovides})
> +
> + for it in "${provides[@]}"; do
> + if grep -q ".*\.so$" <<< "$it"; then
> + if ! grep -q "\(^\|\s\)${it}=.*" <<< $soprovides; then
> + error "$(gettext "Can't find library listed in \$provides: %s")" "$it"
Do we really need to add 18 more forks to makepkg and use grep here?
Bash has regexes, so it would prevent the cryptic use of <<<.
I don't even know why we are doing this check, can you elaborate?
> + return 1
> + fi
> + else
> + echo "provides = $it"
> + fi
> + done
> +
> for it in "${packaging_options[@]}"; do
> local ret="$(check_option $it)"
> if [[ $ret != "?" ]]; then
> --
> 1.7.3.5
On 19.01.2011 19:54, Dan McGee wrote:
On Wed, Jan 19, 2011 at 11:13 AM, Florian Pritz <bluewind@xssn.at> wrote:
<Descriptive commit message usually goes here>
What does this do? Why? What is the generated format? This stuff needs to be here in permanent history so someone patching bugs 2 years from now can figure out the what and why.
Support-by: brain0 <thomas@archlinux.org> Support-by: GNU\caustic <Christoph.Schied@uni-ulm.de> Since the real names of these guys are so obvious, I'd prefer those are there rather than nicks.
Fixed locally.
+find_soprovides() { + local soprovides + find $pkgdir -type f -name \*.so\* | while read filename $pkgdir needs quotes, just like everywhere else it is used.
+ do + if readelf -h "$filename" 2>/dev/null | grep -q '.*Type:.*DYN (Shared object file).*'; then + soarch="$(objdump -a "$filename" 2>/dev/null | \ + sed -rn 's/.* file format elf[0-9]+-(.*)$/\1/p' | tr - _)_$(uname -s)" + sofile=$(readelf -d "$filename" 2>/dev/null | sed -nr 's/.*Library soname: \[(.*)\].*/\1/p') + [ -z "$sofile" ] && sofile="$(basename "$filename")" + + soname=$(sed -rn 's/(.*)\.so.*/\1.so/p' <<< "$sofile") + soversion=$(sed -rn 's/.*\.so\.(.*)/\1/p' <<< "$sofile") Please give examples in a resubmit of what is coming out of here- this is not super easy to follow.
Do you want comments in the code, one big comment above the function or just an explanation in the commit message (will do that one anyway)?
I'm also a bit concerned about: * the uses of sed (is -r portable? We use -n already, but not -r) * the inconsistencies between `sed -rn` and `sed -nr`
I'll just remove the -r then
* running this in any non-C, non-en locale
Fixed locally.
* introducing dependencies on readelf and objdump. At least elsewhere in the strip code, we use `file` to locate shared libraries. And you can get the SONAME bit out of objdump -p, which would at least limit this to one tool.
readelf and objdump are both in binutils, but I've noticed that objdump doesn't support the file format generated by tcc although readelf does and it's a valid ELF file afaict. eu-objdump (elfutils) can't output that much, but it understands tcc's format and outputs pretty nice architecture information. Sadly I have no idea if that's portable. Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's fine since both are in core and you only need them for makepkg anyway.
+ if in_array "${soname}" ${provides[@]}; then + if ! in_array "${soname}=${soversion}-${soarch}" ${soprovides[@]}; then + echo "${soname}=${soversion}-${soarch}" + soprovides=(${soprovides[@]} "${soname}=${soversion}-${soarch}") What's the reasoning behind the ${soarch} append? I suppose it might help with multilib, but something about this just doesn't seem right.
Yeah without that sodeps become useless for packages like wine (32+64bit in multilib)
It is most definitely not a valid pkgver (dash) or pkgrel (not a number).
The dash here just seperates pkgver from pkgrel. Did a quick test with libc.so=6-x86_64_Linux as dependency and a package called libc.so with that pkgver and pkgrel and it worked just fine.
+ fi + fi + fi + done +} + write_pkginfo() { local builddate=$(date -u "+%s") if [[ -n $PACKAGER ]]; then @@ -950,10 +972,24 @@ write_pkginfo() { [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}" [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}" - [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" [[ $backup ]] && printf "backup = %s\n" "${backup[@]}"
local it + + soprovides=$(find_soprovides) + provides=("${provides[@]}" ${soprovides}) + + for it in "${provides[@]}"; do + if grep -q ".*\.so$" <<< "$it"; then + if ! grep -q "\(^\|\s\)${it}=.*" <<< $soprovides; then + error "$(gettext "Can't find library listed in \$provides: %s")" "$it" Do we really need to add 18 more forks to makepkg and use grep here? Bash has regexes, so it would prevent the cryptic use of <<<.
Fixed locally.
I don't even know why we are doing this check, can you elaborate?
First I filter the entries from the PKGBUILD where you use something like this: provides=(libc.so) Then I check if any of those removed entries is missing in the sodeps array. In that case you have a sodep in your PKGBUILD that is not needed and you should remove it.
+ return 1 + fi + else + echo "provides = $it" + fi + done + for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" if [[ $ret != "?" ]]; then -- 1.7.3.5
-- Florian Pritz -- {flo,bluewind}@server-speed.net
On Fri, Jan 21, 2011 at 8:25 AM, Florian Pritz <bluewind@server-speed.net> wrote:
On 19.01.2011 19:54, Dan McGee wrote:
It is most definitely not a valid pkgver (dash) or pkgrel (not a number).
The dash here just seperates pkgver from pkgrel.
Did a quick test with libc.so=6-x86_64_Linux as dependency and a package called libc.so with that pkgver and pkgrel and it worked just fine.
"Works" was not the standard to meet here. The standard is what the prevailing values tend to be (which is integers or decimals), what we may enforce in the future. Perhaps more importantly, this is still wrong (I can't run your i686 binary on my i386 system as it seems to indicate) and if we do keep it, it has *nothing* to do with a version in the normal ordering sense- it would belong as part of the provision name. -Dan
On 21.01.2011 15:49, Dan McGee wrote:
On Fri, Jan 21, 2011 at 8:25 AM, Florian Pritz <bluewind@server-speed.net> wrote:
On 19.01.2011 19:54, Dan McGee wrote:
It is most definitely not a valid pkgver (dash) or pkgrel (not a number).
The dash here just seperates pkgver from pkgrel.
Did a quick test with libc.so=6-x86_64_Linux as dependency and a package called libc.so with that pkgver and pkgrel and it worked just fine.
Perhaps more importantly, this is still wrong (I can't run your i686 binary on my i386 system as it seems to indicate)
http://mailman.archlinux.org/pipermail/pacman-dev/2010-February/010410.html
and if we do keep it, it has *nothing* to do with a version in the normal ordering sense- it would belong as part of the provision name.
I had that before and Allan didn't like it. http://mailman.archlinux.org/pipermail/pacman-dev/2010-February/010420.html -- Florian Pritz -- {flo,bluewind}@server-speed.net
On 22/01/11 00:57, Florian Pritz wrote:
On 21.01.2011 15:49, Dan McGee wrote:
On Fri, Jan 21, 2011 at 8:25 AM, Florian Pritz <bluewind@server-speed.net> wrote:
On 19.01.2011 19:54, Dan McGee wrote:
It is most definitely not a valid pkgver (dash) or pkgrel (not a number).
The dash here just seperates pkgver from pkgrel.
Did a quick test with libc.so=6-x86_64_Linux as dependency and a package called libc.so with that pkgver and pkgrel and it worked just fine.
Perhaps more importantly, this is still wrong (I can't run your i686 binary on my i386 system as it seems to indicate)
http://mailman.archlinux.org/pipermail/pacman-dev/2010-February/010410.html
That reply is just wrong... i686 is not a restricted flavour of i386. It is the other way around. I can not run i686 optimised software on an i386 system. Just ask all the Via C3 owners who do not have that "nopl" instruction and the joys they had with glibc-2.12. So there is a real problem that you can not get the correct value out of the library on i686 systems. We could use CARCH, but that does not work for multilib stuff, which was the entire point of including it in the first place... This needs left out unless the correct value can be given.
and if we do keep it, it has *nothing* to do with a version in the normal ordering sense- it would belong as part of the provision name.
I had that before and Allan didn't like it.
http://mailman.archlinux.org/pipermail/pacman-dev/2010-February/010420.html
I did not like provides=(libfoo.so) magically turning into "libfoo.so-i686" in the .PKGINFO file. The details of the package should reflect what is in the PKGBUILD. Allan
Am 21.01.2011 17:01, schrieb Allan McRae:
That reply is just wrong... i686 is not a restricted flavour of i386. It is the other way around. I can not run i686 optimised software on an i386 system. Just ask all the Via C3 owners who do not have that "nopl" instruction and the joys they had with glibc-2.12.
That is not the point. We should reflect what the dynamic linker tries to do whenever we want to load a binary: 1) If we install a i686 version of libfoo.so.2 onto a i386 system, the linker will still try to load that version (and fail with SIGILL). 2) If we install a x86_64 version and a i686 version of libfoo.so.2 into the system, and launch a 32 bit binary, the linker will determine that it cannot load the the x86_64 version, but the i686 version. So, in order to "do it right", we only need to reflect the distinction the linker does in 2). Not even the linker makes the distinction in 1), so why should we? And now think about use cases: The package architecture is already in PKGINFO. So the only distinction we need to make is between 32 bit and 64 bit binaries - the only case where you are able to run more than one architecture in the same system is when executing 32 bit binaries on a 64 bit platform with a backward compatibility mode for 32 bit. (AFAIK, this is not only the case for x86, but for some other platforms, too). The only info we should include into the soprovides/depends is whether we have a 32 bit or a 64 bit binary. There shouldn't be an explicit statement about architecture.
So there is a real problem that you can not get the correct value out of the library on i686 systems. We could use CARCH, but that does not work for multilib stuff, which was the entire point of including it in the first place...
I think my previous comments should make this irrelevant, unless I am completely wrong.
and if we do keep it, it has *nothing* to do with a version in the normal ordering sense- it would belong as part of the provision name.
I had that before and Allan didn't like it.
http://mailman.archlinux.org/pipermail/pacman-dev/2010-February/010420.html
I did not like provides=(libfoo.so) magically turning into "libfoo.so-i686" in the .PKGINFO file. The details of the package should reflect what is in the PKGBUILD.
So, to summarize: 1) Dan does not want a new PKGINFO field to be introduced for this. 2) Allan does not want any auto-generated meta-info in the provides and depends in PKGINFO. 3) Dan does not want non-versioning information in the version field in provides/depends in PKGINFO. Where does that leave us? Essentially, we cannot get this feature implemented, as all those restrictions combined leave us with no options. Clearly, you don't see any options either, as you only provide destructive criticism, no suggestions on how you think it should be done.
Wow, this degenerated into a flamewar quickly. Can we all calm down just a tad? If we were truly at an impasse, Allan and I wouldn't even be looking at these patches. The biggest reason I'm putting up a stop sign and red flags is the lack of explanation at the questions I've posed, and stating I've only offered destructive criticism is not the truth- I just don't understand this whole thing. On Fri, Jan 21, 2011 at 10:54 AM, Thomas Bächler <thomas@archlinux.org> wrote:
Am 21.01.2011 17:01, schrieb Allan McRae:
That reply is just wrong... i686 is not a restricted flavour of i386. It is the other way around. I can not run i686 optimised software on an i386 system. Just ask all the Via C3 owners who do not have that "nopl" instruction and the joys they had with glibc-2.12.
That is not the point. We should reflect what the dynamic linker tries to do whenever we want to load a binary:
1) If we install a i686 version of libfoo.so.2 onto a i386 system, the linker will still try to load that version (and fail with SIGILL). 2) If we install a x86_64 version and a i686 version of libfoo.so.2 into the system, and launch a 32 bit binary, the linker will determine that it cannot load the the x86_64 version, but the i686 version.
So, in order to "do it right", we only need to reflect the distinction the linker does in 2). Not even the linker makes the distinction in 1), so why should we?
And now think about use cases: The package architecture is already in PKGINFO. So the only distinction we need to make is between 32 bit and 64 bit binaries - the only case where you are able to run more than one architecture in the same system is when executing 32 bit binaries on a 64 bit platform with a backward compatibility mode for 32 bit. (AFAIK, this is not only the case for x86, but for some other platforms, too).
The only info we should include into the soprovides/depends is whether we have a 32 bit or a 64 bit binary. There shouldn't be an explicit statement about architecture.
Taking a step back here, if we want to not lock ourself into ELF-only (OSX does not use it), it sounds like using objdump due to it's use of the BFD backend which understands multiple file formats. From objdump -a, although the output is rather arcane, we should always be able to get the file format of the library, which I believe is the most important to matching what the linker does. Sidenote: I saw this when running objdump --info on my x86_64 box: plugin (header little endian, data little endian) BFD: BFD (GNU Binutils) 2.21.0.20101217 assertion fail /build/src/binutils/bfd/plugin.c:453 objdump: plugin: Bad value
So there is a real problem that you can not get the correct value out of the library on i686 systems. We could use CARCH, but that does not work for multilib stuff, which was the entire point of including it in the first place...
I think my previous comments should make this irrelevant, unless I am completely wrong.
and if we do keep it, it has *nothing* to do with a version in the normal ordering sense- it would belong as part of the provision name.
I had that before and Allan didn't like it.
http://mailman.archlinux.org/pipermail/pacman-dev/2010-February/010420.html
I did not like provides=(libfoo.so) magically turning into "libfoo.so-i686" in the .PKGINFO file. The details of the package should reflect what is in the PKGBUILD.
See, this is what I still don't understand. Is this libprovides, or is this "well we'll hijack your provides array sometimes depending on what you put there". It *still* hasn't been explained how this thing works in either a commit message or docs. Do I include soprovides=() in my PKGBUILD? Do I need to insert something in provides=() to get a library to show up in the final package? Is it all completely automatic- all libraries in the package get an automatic provide entry generated? This is why I am so damn frustrated with these patches.
So, to summarize:
1) Dan does not want a new PKGINFO field to be introduced for this.
Where did I say that- if I did, I'm not remembering.
2) Allan does not want any auto-generated meta-info in the provides and depends in PKGINFO. No, he stated he doesn't want *magic changes* to data the user specified in the PKGBUILD. I think he and I would be more comfortable if this was less magic.
3) Dan does not want non-versioning information in the version field in provides/depends in PKGINFO. Sure.
Where does that leave us? Essentially, we cannot get this feature implemented, as all those restrictions combined leave us with no options. Clearly, you don't see any options either, as you only provide destructive criticism, no suggestions on how you think it should be done.
On 21.01.2011 18:35, Dan McGee wrote:
It *still* hasn't been explained how this thing works in either a commit message or docs. Do I include soprovides=() in my PKGBUILD? Do I need to insert something in provides=() to get a library to show up in the final package? Is it all completely automatic- all libraries in the package get an automatic provide entry generated? This is why I am so damn frustrated with these patches.
We had all 3 versions and the current patch works as follows: You add something like "provides=(foo bar libfoo.so)" to the PKGBUILD. The function searches all provides in the whole pkgdir and filters that list using your provides array. It then removes your entries (versionless) and adds those it found (with version). -- Florian Pritz -- {flo,bluewind}@server-speed.net
Am 21.01.2011 18:35, schrieb Dan McGee:
Wow, this degenerated into a flamewar quickly.
I deleted all flames before submitting this post. That said, I was a bit pissed about that last part of Allan's post.
and stating I've only offered destructive criticism is not the truth- I just don't understand this whole thing.
That statement was directed only at the part of Allan's reply that I quoted right above the statement. It was not in any way meant for you, or to any other part of Allan's post.
On Fri, Jan 21, 2011 at 10:54 AM, Thomas Bächler <thomas@archlinux.org> wrote:
Am 21.01.2011 17:01, schrieb Allan McRae:
That reply is just wrong... i686 is not a restricted flavour of i386. It is the other way around. I can not run i686 optimised software on an i386 system. Just ask all the Via C3 owners who do not have that "nopl" instruction and the joys they had with glibc-2.12.
That is not the point. We should reflect what the dynamic linker tries to do whenever we want to load a binary:
1) If we install a i686 version of libfoo.so.2 onto a i386 system, the linker will still try to load that version (and fail with SIGILL). 2) If we install a x86_64 version and a i686 version of libfoo.so.2 into the system, and launch a 32 bit binary, the linker will determine that it cannot load the the x86_64 version, but the i686 version.
So, in order to "do it right", we only need to reflect the distinction the linker does in 2). Not even the linker makes the distinction in 1), so why should we?
And now think about use cases: The package architecture is already in PKGINFO. So the only distinction we need to make is between 32 bit and 64 bit binaries - the only case where you are able to run more than one architecture in the same system is when executing 32 bit binaries on a 64 bit platform with a backward compatibility mode for 32 bit. (AFAIK, this is not only the case for x86, but for some other platforms, too).
The only info we should include into the soprovides/depends is whether we have a 32 bit or a 64 bit binary. There shouldn't be an explicit statement about architecture.
Taking a step back here, if we want to not lock ourself into ELF-only (OSX does not use it), it sounds like using objdump due to it's use of the BFD backend which understands multiple file formats. From objdump -a, although the output is rather arcane, we should always be able to get the file format of the library, which I believe is the most important to matching what the linker does.
I do not want to lock anything explicitly to ELF. Considering that OSX also supports 32 and 64 bit libraries on the same system (and ones that contain both), it's a bad idea. The question remains, how portable can we even be here? Do we maybe need support for different binary formats, implemented in different ways?
I did not like provides=(libfoo.so) magically turning into "libfoo.so-i686" in the .PKGINFO file. The details of the package should reflect what is in the PKGBUILD.
See, this is what I still don't understand. Is this libprovides, or is this "well we'll hijack your provides array sometimes depending on what you put there".
It *still* hasn't been explained how this thing works in either a commit message or docs. Do I include soprovides=() in my PKGBUILD? Do I need to insert something in provides=() to get a library to show up in the final package? Is it all completely automatic- all libraries in the package get an automatic provide entry generated? This is why I am so damn frustrated with these patches.
The original idea was this: makepkg automatically adds provides for all libraries with a proper SONAME. These are added as provides to PKGINFO in addition to the user-specified provides. Alternatively, the PKGBUILD has to specify which libraries to add. So we would write soprovides=(libfoo.so) and this would turn into a provider for PKGINFO that would contain some information about architecture and SONAME. I don't like this option because the list of libraries is maintained by a human and not the computer. TBH, reading the current patch again, I am unsure what it actually does now. I think it is using the first option.
So, to summarize:
1) Dan does not want a new PKGINFO field to be introduced for this. Where did I say that- if I did, I'm not remembering.
Now that you ask, I am not 100% sure you even did. I thought I remembered something from a few months back. I apologize for not confirming this. But back to the point: Would you want this information separate? Adding a new field to PKGINFO here would essentially duplicate the whole provides/depends handling, and add new pacman code. When adding this auto-generated info to PKGINFO's provides and depends, there are no changes in pacman.
2) Allan does not want any auto-generated meta-info in the provides and depends in PKGINFO. No, he stated he doesn't want *magic changes* to data the user specified in the PKGBUILD. I think he and I would be more comfortable if this was less magic.
And what is "magic"? It is simply automatic. Furthermore, as far as I can see, there are no changes to the user data, but only additions to it.
3) Dan does not want non-versioning information in the version field in provides/depends in PKGINFO. Sure.
So, let's summarize again what we want (because it seems you don't understand that): 1) We want to have information in the PKGINFO that tells the user and pacman which libraries are included in the package, and what their SONAME versions are. 2) We also want (optional?) information in the PKGINFO about which libraries in which versions are needed to run this package. 3) We want this information automatically generated, not manually maintained. The least invasive approach in our opinion was to add this information to PKGINFO's provides and depends, as it would work without any changes to pacman. This means that the provides and depends are extended by auto-generated ones. Nothing is magically modified or removed. Now, as everything we suggested is so weird to you: How would you want such a feature implemented?
Am 21.01.2011 19:09, schrieb Thomas Bächler:
And what is "magic"? It is simply automatic. Furthermore, as far as I can see, there are no changes to the user data, but only additions to it.
Heh, wrong, sorry. I discussed so many variants of this with Florian, that I have lost track of what we do now.
On 22/01/11 02:54, Thomas Bächler wrote:
Where does that leave us? Essentially, we cannot get this feature implemented, as all those restrictions combined leave us with no options. Clearly, you don't see any options either, as you only provide destructive criticism, no suggestions on how you think it should be done.
I only pointed out the things that I see are wrong because they are what needed fixed. Also it was 2am when I replied in between dealing with a sick child so my time for pleasantries was limited. I did not think everybodies self esteem was so low that I needed to praise the correct parts too. And how can we do this portably and subject to the restrictions given... Well, it has been established that using objdump or elfutils has issues so lets just exclude them from the start. How else can we get information about a file? file... That tells you whether a binary is 32 or 64 bit, and is already required by makepkg. So: provides=(libfoo.so) Results in %PROVIDES% libfoo.so=6-32 where pkgver is the library soname and pkgrel indicates 32/64 bit. If the "file" output does not include 32-bit or 64-bit, then just do not set the pkgrel value. That solves all comments made so far apart from the documentation needed on what this does... Allan
On 22.01.2011 01:20, Allan McRae wrote:
And how can we do this portably and subject to the restrictions given... Well, it has been established that using objdump or elfutils has issues so lets just exclude them from the start. How else can we get information about a file? file... That tells you whether a binary is 32 or 64 bit, and is already required by makepkg.
file doesn't tell you the library soname so I don't know what ld will use for linking. So far readelf seems to be the best choice because I can also use it to generate the dependencies (right now it seems like we are too focused on the provides). Your example below can be achieved using readelf. Also file's output doesn't have to be the same on different platforms.
provides=(libfoo.so)
Results in
%PROVIDES% libfoo.so=6-32
Are you okay with that format Dan? -- Florian Pritz -- {flo,bluewind}@server-speed.net
On 22/01/11 19:31, Florian Pritz wrote:
On 22.01.2011 01:20, Allan McRae wrote:
And how can we do this portably and subject to the restrictions given... Well, it has been established that using objdump or elfutils has issues so lets just exclude them from the start. How else can we get information about a file? file... That tells you whether a binary is 32 or 64 bit, and is already required by makepkg.
file doesn't tell you the library soname so I don't know what ld will use for linking. So far readelf seems to be the best choice because I can also use it to generate the dependencies (right now it seems like we are too focused on the provides). Your example below can be achieved using readelf. Also file's output doesn't have to be the same on different platforms.
What are we going to do about OSX and its lack of elf binaries?
On Sat, Jan 22, 2011 at 11:25 AM, Allan McRae <allan@archlinux.org> wrote:
What are we going to do about OSX and its lack of elf binaries?
Who cares about soprovides/sodepends besides ArchLinux people ?
On 22.01.2011 11:54, Xavier Chantry wrote:
On Sat, Jan 22, 2011 at 11:25 AM, Allan McRae <allan@archlinux.org> wrote:
What are we going to do about OSX and its lack of elf binaries?
Who cares about soprovides/sodepends besides ArchLinux people ?
After a quick google search it looks like OS X uses .dylib so I won't even look at those files anyway since my find command only search *.so* -- Florian Pritz -- {flo,bluewind}@server-speed.net
Am 22.01.2011 11:25, schrieb Allan McRae:
What are we going to do about OSX and its lack of elf binaries?
A solution that fits all platforms probably doesn't exist here. We can implement this for ELF now, and maybe someone can figure out the OS X version at some point. (.dylib is very weird IIRC, I had to look into that once).
On 22/01/11 21:25, Thomas Bächler wrote:
Am 22.01.2011 11:25, schrieb Allan McRae:
What are we going to do about OSX and its lack of elf binaries?
A solution that fits all platforms probably doesn't exist here. We can implement this for ELF now, and maybe someone can figure out the OS X version at some point. (.dylib is very weird IIRC, I had to look into that once).
That sounds fine. I did not realise that a different extension is used for shared libraries on OSX (although I should have thinking about it...). So just working with ELF for now seems fine. Some other things to think about. I just built a bunch of packages using soprovides/sodepends with readline providing libreadline.so. With this, on an upgrade with a soname bump we will get a whole heap of messages like: warning: provider package was selected (readline provides libreadline.so) So we need to consider whether we want to keep that message displayed, or if there is some way to only display it once (see next). I also noticed the performance hit in satisfying dependencies using provides. I wonder if we can optimise dependency checking such that we check for a given dependency only once, even if it is needed across multiple packages? That would also solve the output issue above. I'm not familiar enough with the dependency checking code to know if that is viable. Allan
On 22.01.2011 13:15, Allan McRae wrote:
With this, on an upgrade with a soname bump we will get a whole heap of messages like:
warning: provider package was selected (readline provides libreadline.so)
I also noticed the performance hit in satisfying dependencies using provides.
When I tried that in August 2009 it only displayed that message once (at least I think it did, but I'm pretty sure) and the time to resolve the dependencies for ~900 packages with (using -U; all sodeps/provides and not just readline iirc) went from 25 secs to 27 secs. Could you stop the time so we can compare the results? -- Florian Pritz -- {flo,bluewind}@server-speed.net
On 22/01/11 22:43, Florian Pritz wrote:
On 22.01.2011 13:15, Allan McRae wrote:
With this, on an upgrade with a soname bump we will get a whole heap of messages like:
warning: provider package was selected (readline provides libreadline.so)
I also noticed the performance hit in satisfying dependencies using provides.
When I tried that in August 2009 it only displayed that message once (at least I think it did, but I'm pretty sure)
Ugh... my bad... you have to weird things to your pacman databases get this message to appear multiple times. Moving along...
and the time to resolve the dependencies for ~900 packages with (using -U; all sodeps/provides and not just readline iirc) went from 25 secs to 27 secs.
Could you stop the time so we can compare the results?
When installing two packages, changing the depend to a provide takes 8.7sec compared to 7.9sec. So about 10%, which is consistent with what you got. I'm a bit surprised it scales so well to a much large number of packages and provides. Allan
Am 22.01.2011 01:20, schrieb Allan McRae:
I only pointed out the things that I see are wrong because they are what needed fixed. Also it was 2am when I replied in between dealing with a sick child so my time for pleasantries was limited.
Sorry, I sounded way harsher than I meant to in that post (as pointed out by Dan already). Happens to the best of us.
On 22/01/11 00:25, Florian Pritz wrote:
On 19.01.2011 19:54, Dan McGee wrote: <snip>
* introducing dependencies on readelf and objdump. At least elsewhere in the strip code, we use `file` to locate shared libraries. And you can get the SONAME bit out of objdump -p, which would at least limit this to one tool.
readelf and objdump are both in binutils, but I've noticed that objdump doesn't support the file format generated by tcc although readelf does and it's a valid ELF file afaict.
eu-objdump (elfutils) can't output that much, but it understands tcc's format and outputs pretty nice architecture information. Sadly I have no idea if that's portable.
Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's fine since both are in core and you only need them for makepkg anyway.
We have pacman/makepkg users on Linux, OSX, BSD, Hurd, Cygwin... Whatever is used here needs to be portable to all those. I do not believe elfutils is particularly portable... Allan
On 21.01.2011 15:49, Allan McRae wrote:
On 22/01/11 00:25, Florian Pritz wrote:
On 19.01.2011 19:54, Dan McGee wrote: <snip>
* introducing dependencies on readelf and objdump. At least elsewhere in the strip code, we use `file` to locate shared libraries. And you can get the SONAME bit out of objdump -p, which would at least limit this to one tool.
readelf and objdump are both in binutils, but I've noticed that objdump doesn't support the file format generated by tcc although readelf does and it's a valid ELF file afaict.
eu-objdump (elfutils) can't output that much, but it understands tcc's format and outputs pretty nice architecture information. Sadly I have no idea if that's portable.
Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's fine since both are in core and you only need them for makepkg anyway.
We have pacman/makepkg users on Linux, OSX, BSD, Hurd, Cygwin... Whatever is used here needs to be portable to all those. I do not believe elfutils is particularly portable...
I'd use objdump (binutils), but I don't know if it's compatible with binaries not generated by gcc (at least tcc doesn't work) and I can't really assume all users use gcc either. There is a "portability" patch [1] for elfutils. Not sure if that is what we need though. [1] https://fedorahosted.org/releases/e/l/elfutils/0.151/elfutils-portability.pa... -- Florian Pritz -- {flo,bluewind}@server-speed.net
On 22/01/11 01:02, Florian Pritz wrote:
On 21.01.2011 15:49, Allan McRae wrote:
On 22/01/11 00:25, Florian Pritz wrote:
On 19.01.2011 19:54, Dan McGee wrote: <snip>
* introducing dependencies on readelf and objdump. At least elsewhere in the strip code, we use `file` to locate shared libraries. And you can get the SONAME bit out of objdump -p, which would at least limit this to one tool.
readelf and objdump are both in binutils, but I've noticed that objdump doesn't support the file format generated by tcc although readelf does and it's a valid ELF file afaict.
eu-objdump (elfutils) can't output that much, but it understands tcc's format and outputs pretty nice architecture information. Sadly I have no idea if that's portable.
Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's fine since both are in core and you only need them for makepkg anyway.
We have pacman/makepkg users on Linux, OSX, BSD, Hurd, Cygwin... Whatever is used here needs to be portable to all those. I do not believe elfutils is particularly portable...
I'd use objdump (binutils), but I don't know if it's compatible with binaries not generated by gcc (at least tcc doesn't work) and I can't really assume all users use gcc either.
There is a "portability" patch [1] for elfutils. Not sure if that is what we need though.
That portability patch seems to be mainly to add compatibility to older Linux systems, not for non-Linux systems. Allan
On Friday, January 21, 2011 08:49:48 am Allan McRae wrote:
On 22/01/11 00:25, Florian Pritz wrote:
On 19.01.2011 19:54, Dan McGee wrote: <snip>
* introducing dependencies on readelf and objdump. At least elsewhere in the strip code, we use `file` to locate shared libraries. And you can get the SONAME bit out of objdump -p, which would at least limit this to one tool.
readelf and objdump are both in binutils, but I've noticed that objdump doesn't support the file format generated by tcc although readelf does and it's a valid ELF file afaict.
eu-objdump (elfutils) can't output that much, but it understands tcc's format and outputs pretty nice architecture information. Sadly I have no idea if that's portable.
Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's fine since both are in core and you only need them for makepkg anyway.
We have pacman/makepkg users on Linux, OSX, BSD, Hurd, Cygwin... Whatever is used here needs to be portable to all those. I do not believe elfutils is particularly portable...
Allan
Stop me if I am wrong, but don't all those platforms use ELF by default?
On 22/01/11 02:05, Yaro Kasear wrote:
On Friday, January 21, 2011 08:49:48 am Allan McRae wrote:
On 22/01/11 00:25, Florian Pritz wrote:
On 19.01.2011 19:54, Dan McGee wrote: <snip>
* introducing dependencies on readelf and objdump. At least elsewhere in the strip code, we use `file` to locate shared libraries. And you can get the SONAME bit out of objdump -p, which would at least limit this to one tool.
readelf and objdump are both in binutils, but I've noticed that objdump doesn't support the file format generated by tcc although readelf does and it's a valid ELF file afaict.
eu-objdump (elfutils) can't output that much, but it understands tcc's format and outputs pretty nice architecture information. Sadly I have no idea if that's portable.
Now I'm using readelf (binutils) and eu-objdump (elfutils). I think it's fine since both are in core and you only need them for makepkg anyway.
We have pacman/makepkg users on Linux, OSX, BSD, Hurd, Cygwin... Whatever is used here needs to be portable to all those. I do not believe elfutils is particularly portable...
Allan
Stop me if I am wrong, but don't all those platforms use ELF by default?
Yes, and... That does not make elfutils portable or fix the issue with objdump and tcc generated files. Allan
From: Florian Pritz <bluewind@xssn.at> The user adds libaries to the provides array without a version. These must end with .so. Example: provides=(readline libreadline.so) find_soprovides() looks for .so files (not symlinks because these could point outside of pkgdir) in $pkgdir, extracts the library soname (ld links the binary to this name) and outputs provides seperated by spaces. This list contains all libraries provided by the package. Example: libfoo.so=3_64 write_pkginfo() only keeps .so provides with version information and warns the user about unneded ones. Support-by: Thomas Bächler <thomas@archlinux.org> Support-by: Christoph Schied <Christoph.Schied@uni-ulm.de> Signed-off-by: Florian Pritz <bluewind@server-speed.net> --- scripts/makepkg.sh.in | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 46 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 6ebfac0..06631f4 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -917,6 +917,34 @@ tidy_install() { fi } +find_soprovides() { + local soprovides + local os=$(uname -s | tr [:upper:] [:lower:]) + find "$pkgdir" -type f -name \*.so\* | while read filename + do + # check if we really have a shared object + if LC_ALL=C readelf -h "$filename" 2>/dev/null | grep -q '.*Type:.*DYN (Shared object file).*'; then + # 64 + soarch=$(LC_ALL=C readelf -h "$filename" | sed -n 's/.*Class.*ELF\(32\|64\)/\1/p') + # get the string binaries link to: libfoo.so.1.2 -> libfoo.so.1 + sofile=$(LC_ALL=C readelf -d "$filename" 2>/dev/null | sed -n 's/.*Library soname: \[\(.*\)\].*/\1/p') + [ -z "$sofile" ] && sofile="${$filename##*/}" + + # extract the library name: libfoo.so + soname="${sofile%%\.so\.*}.so" + # extract the major version: 1 + soversion="${sofile##*\.so\.}" + if in_array "${soname}" ${provides[@]}; then + if ! in_array "${soname}=${soversion}-${soarch}" ${soprovides[@]}; then + # libfoo.so=1-elf_x86_64_linux + echo "${soname}=${soversion}-${soarch}" + soprovides=(${soprovides[@]} "${soname}=${soversion}-${soarch}") + fi + fi + fi + done +} + write_pkginfo() { local builddate=$(date -u "+%s") if [[ -n $PACKAGER ]]; then @@ -950,10 +978,27 @@ write_pkginfo() { [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}" [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}" - [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" local it + + soprovides=$(find_soprovides) + provides=("${provides[@]}" ${soprovides}) + + for it in "${provides[@]}"; do + # ignore versionless entires (those come from the PKGBUILD) + if [[ $it = *.so ]]; then + # check if the entry has been found by find_soprovides + # if not, it's unneeded; tell the user so he can remove it + if [[ ! $soprovides =~ (^|\s)${it}=.* ]]; then + error "$(gettext "Can't find library listed in \$provides: %s")" "$it" + return 1 + fi + else + echo "provides = $it" + fi + done + for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" if [[ $ret != "?" ]]; then -- 1.7.3.5
Support-by: brain0 <thomas@archlinux.org> Support-by: GNU\caustic <Christoph.Schied@uni-ulm.de> Signed-off-by: Florian Pritz <bluewind@xssn.at> --- scripts/makepkg.sh.in | 35 ++++++++++++++++++++++++++++++++++- 1 files changed, 34 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cf7dbb9..a851983 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -917,6 +917,27 @@ tidy_install() { fi } +find_sodepends() { + local sodepends + find $pkgdir -type f | while read filename + do + soarch="$(objdump -a "$filename" 2>/dev/null | \ + sed -rn 's/.* file format elf[0-9]+-(.*)$/\1/p' | tr - _)_$(uname -s)" + [ -n "$soarch" ] || continue + for sofile in $(readelf -d "$filename" 2> /dev/null | sed -nr 's/.*Shared library: \[(.*)\].*/\1/p') + do + soname=$(sed -rn 's/(.*)\.so.*/\1.so/p' <<< "$sofile") + soversion=$(sed -rn 's/.*\.so\.(.*)/\1/p' <<< "$sofile") + if in_array "${soname}" ${depends[@]}; then + if ! in_array "${soname}=${soversion}-${soarch}" ${sodepends[@]}; then + echo "${soname}=${soversion}-${soarch}" + sodepends=(${sodepends[@]} "${soname}=${soversion}-${soarch}") + fi + fi + done + done +} + find_soprovides() { local soprovides find $pkgdir -type f -name \*.so\* | while read filename @@ -969,7 +990,6 @@ write_pkginfo() { [[ $license ]] && printf "license = %s\n" "${license[@]}" [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" [[ $groups ]] && printf "group = %s\n" "${groups[@]}" - [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}" [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}" [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" @@ -977,7 +997,20 @@ write_pkginfo() { local it soprovides=$(find_soprovides) + sodepends=$(find_sodepends) provides=("${provides[@]}" ${soprovides}) + depends=("${depends[@]}" ${sodepends}) + + for it in "${depends[@]}"; do + if grep -q ".*\.so$" <<< "$it"; then + if ! grep -q "\(^\|\s\)${it}=.*" <<< $sodepends; then + error "$(gettext "Can't find library listed in \$depends: %s")" "$it" + return 1 + fi + else + echo "depend = $it" + fi + done for it in "${provides[@]}"; do if grep -q ".*\.so$" <<< "$it"; then -- 1.7.3.5
participants (7)
-
Allan McRae
-
Dan McGee
-
Florian Pritz
-
Florian Pritz
-
Thomas Bächler
-
Xavier Chantry
-
Yaro Kasear