[pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

older
[pacman-dev] [PATCHv2] libalpm:...

Olivier Brunel

28 Feb 2013 28 Feb '13

7:06 p.m.

During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed. Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this. lib/libalpm/sync.c | 13 ++++++++++++- test/pacman/tests/sync1104.py | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 test/pacman/tests/sync1104.py diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 204456d..31f5870 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -351,6 +351,16 @@ finish: return ret; } +static int _cmp_pkg (alpm_pkg_t *pkg1, alpm_pkg_t *pkg2) +{ + if (!pkg1) + return 1; + else if (!pkg2) + return -1; + else + return strcmp (pkg1->name, pkg2->name); +} + int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) { alpm_list_t *i, *j; @@ -408,7 +418,8 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) building up a list of packages which could not be resolved. */ for(i = trans->add; i; i = i->next) { alpm_pkg_t *pkg = i->data; - if(_alpm_resolvedeps(handle, localpkgs, pkg, trans->add, + if(!alpm_list_find(remove, pkg, (alpm_list_fn_cmp) _cmp_pkg) + && _alpm_resolvedeps(handle, localpkgs, pkg, trans->add, &resolved, remove, data) == -1) { unresolvable = alpm_list_add(unresolvable, pkg); } diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new") -- 1.8.1.4

Show replies by thread

Allan McRae

3 Mar 3 Mar

8:43 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 01/03/13 04:06, Olivier Brunel wrote:

...

During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed.

Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this.

lib/libalpm/sync.c | 13 ++++++++++++- test/pacman/tests/sync1104.py | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 test/pacman/tests/sync1104.py

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 204456d..31f5870 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -351,6 +351,16 @@ finish: return ret; }

+static int _cmp_pkg (alpm_pkg_t *pkg1, alpm_pkg_t *pkg2) +{ + if (!pkg1) + return 1; + else if (!pkg2) + return -1; + else + return strcmp (pkg1->name, pkg2->name); +} +

we have _alpm_pkg_cmp() in package.h

...

int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) { alpm_list_t *i, *j; @@ -408,7 +418,8 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) building up a list of packages which could not be resolved. */ for(i = trans->add; i; i = i->next) { alpm_pkg_t *pkg = i->data; - if(_alpm_resolvedeps(handle, localpkgs, pkg, trans->add, + if(!alpm_list_find(remove, pkg, (alpm_list_fn_cmp) _cmp_pkg) + && _alpm_resolvedeps(handle, localpkgs, pkg, trans->add,

Hrm... I think it would be best just to remove the packages from trans->add one the are flagged to be removed. I.e. about 10 lines above this. (I am assuming that can be done...).

...

&resolved, remove, data) == -1) { unresolvable = alpm_list_add(unresolvable, pkg); } diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new")

Allan McRae

8:45 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 04/03/13 05:43, Allan McRae wrote:

...

On 01/03/13 04:06, Olivier Brunel wrote:

...
During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed.

Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this.

lib/libalpm/sync.c | 13 ++++++++++++- test/pacman/tests/sync1104.py | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 test/pacman/tests/sync1104.py

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 204456d..31f5870 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -351,6 +351,16 @@ finish: return ret; }

+static int _cmp_pkg (alpm_pkg_t *pkg1, alpm_pkg_t *pkg2) +{ + if (!pkg1) + return 1; + else if (!pkg2) + return -1; + else + return strcmp (pkg1->name, pkg2->name); +} +

we have _alpm_pkg_cmp() in package.h

...
int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) { alpm_list_t *i, *j; @@ -408,7 +418,8 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data) building up a list of packages which could not be resolved. */ for(i = trans->add; i; i = i->next) { alpm_pkg_t *pkg = i->data; - if(_alpm_resolvedeps(handle, localpkgs, pkg, trans->add, + if(!alpm_list_find(remove, pkg, (alpm_list_fn_cmp) _cmp_pkg) + && _alpm_resolvedeps(handle, localpkgs, pkg, trans->add,

Hrm... I think it would be best just to remove the packages from trans->add one the are flagged to be removed. I.e. about 10 lines above this. (I am assuming that can be done...).

...
&resolved, remove, data) == -1) { unresolvable = alpm_list_add(unresolvable, pkg); } diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new")

Forgot to add - awesome for adding a pactest here!

Andrew Gregory

9:26 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 03/04/13 at 05:45am, Allan McRae wrote:

...

On 04/03/13 05:43, Allan McRae wrote:

...
On 01/03/13 04:06, Olivier Brunel wrote:

...
During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed.

Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this.

<snip>

...
diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new")

Forgot to add - awesome for adding a pactest here!

Is the test case correct though? Other than the fact that this one uses two sync repos, this looks identical to sync132 which has the opposite result. apg

Allan McRae

9:29 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 04/03/13 06:26, Andrew Gregory wrote:

...

On 03/04/13 at 05:45am, Allan McRae wrote:

...
On 04/03/13 05:43, Allan McRae wrote:

...
On 01/03/13 04:06, Olivier Brunel wrote:

...
During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed.

Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this.

<snip>

...
diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new")

Forgot to add - awesome for adding a pactest here!

Is the test case correct though? Other than the fact that this one uses two sync repos, this looks identical to sync132 which has the opposite result.

Hrm... I'd say that sync132 is wrong... If pkg1 replaces pkg2, I'd expect pkg1 to replace pkg2 whether or not pkg2 has an update. Allan Allan

Allan McRae

9:41 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 04/03/13 06:42, Andrew Gregory wrote:

...

On 03/04/13 at 06:29am, Allan McRae wrote:

...
On 04/03/13 06:26, Andrew Gregory wrote:

...
On 03/04/13 at 05:45am, Allan McRae wrote:

...
On 04/03/13 05:43, Allan McRae wrote:

...
On 01/03/13 04:06, Olivier Brunel wrote:

...
During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed.

Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this.

<snip>

...
diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new")

Forgot to add - awesome for adding a pactest here!

Is the test case correct though? Other than the fact that this one uses two sync repos, this looks identical to sync132 which has the opposite result.

Hrm... I'd say that sync132 is wrong...

If pkg1 replaces pkg2, I'd expect pkg1 to replace pkg2 whether or not pkg2 has an update.

Allan

Allan

I would actually come out the other way. Where a replacement is *unversioned* like in this test I would expect an updated package to override it. Otherwise it would try to replace the other package on every update even though it's still in a repo. Test replace102 (it would be awesome if we could get some kind of consistent naming scheme going for these tests) handles the situation where you have a versioned replace for a package still in a repo (python 2->3 upgrade).

Huh? Once a package is replaced, it is gone - no need to replace it again... At the moment, with pkg1 replacing pkg2 (installed) and an update to pkg2. pacman -Su (update pkg2) pacman -Su (replace pkg2) I think it should be pacman -Su (replace pkg2) done... Allan

Andrew Gregory

9:42 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 03/04/13 at 06:29am, Allan McRae wrote:

...

On 04/03/13 06:26, Andrew Gregory wrote:

...
On 03/04/13 at 05:45am, Allan McRae wrote:

...
On 04/03/13 05:43, Allan McRae wrote:

...
On 01/03/13 04:06, Olivier Brunel wrote:

...
During a sysupgrade, if a package is replaced by another, and an update for the former package is found (on another repo) the replaced package would be re-installed.

Signed-off-by: Olivier Brunel --- I'm not really familiar with inner workings of ALPM, so this is probably not the best way to do this.

<snip>

...
diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py new file mode 100644 index 0000000..5cec98a --- /dev/null +++ b/test/pacman/tests/sync1104.py @@ -0,0 +1,18 @@ +self.description = "Don't update (reinstall) a replaced package" + +lp = pmpkg("old", "1-1") +self.addpkg2db("local", lp) + +p1 = pmpkg("new") +p1.provides = ["old"] +p1.replaces = ["old"] +self.addpkg2db("sync1", p1) + +p2 = pmpkg("old", "1-2") +self.addpkg2db("sync2", p2) + +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!PKG_EXIST=old") +self.addrule("PKG_EXIST=new")

Forgot to add - awesome for adding a pactest here!

Is the test case correct though? Other than the fact that this one uses two sync repos, this looks identical to sync132 which has the opposite result.

Hrm... I'd say that sync132 is wrong...

If pkg1 replaces pkg2, I'd expect pkg1 to replace pkg2 whether or not pkg2 has an update.

Allan

Allan

I would actually come out the other way. Where a replacement is *unversioned* like in this test I would expect an updated package to override it. Otherwise it would try to replace the other package on every update even though it's still in a repo. Test replace102 (it would be awesome if we could get some kind of consistent naming scheme going for these tests) handles the situation where you have a versioned replace for a package still in a repo (python 2->3 upgrade). apg

jjacky

10:36 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 03/03/13 21:41, Allan McRae wrote:

...

On 04/03/13 06:42, Andrew Gregory wrote:

...
On 03/04/13 at 06:29am, Allan McRae wrote:

...
On 04/03/13 06:26, Andrew Gregory wrote:

...
On 03/04/13 at 05:45am, Allan McRae wrote:

...
On 04/03/13 05:43, Allan McRae wrote:

...
On 01/03/13 04:06, Olivier Brunel wrote: > During a sysupgrade, if a package is replaced by another, and an update for the > former package is found (on another repo) the replaced package would be > re-installed. > > Signed-off-by: Olivier Brunel > --- > I'm not really familiar with inner workings of ALPM, so this is probably not the > best way to do this. > <snip> > diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py > new file mode 100644 > index 0000000..5cec98a > --- /dev/null > +++ b/test/pacman/tests/sync1104.py > @@ -0,0 +1,18 @@ > +self.description = "Don't update (reinstall) a replaced package" > + > +lp = pmpkg("old", "1-1") > +self.addpkg2db("local", lp) > + > +p1 = pmpkg("new") > +p1.provides = ["old"] > +p1.replaces = ["old"] > +self.addpkg2db("sync1", p1) > + > +p2 = pmpkg("old", "1-2") > +self.addpkg2db("sync2", p2) > + > +self.args = "-Su" > + > +self.addrule("PACMAN_RETCODE=0") > +self.addrule("!PKG_EXIST=old") > +self.addrule("PKG_EXIST=new") >

Forgot to add - awesome for adding a pactest here!

Is the test case correct though? Other than the fact that this one uses two sync repos, this looks identical to sync132 which has the opposite result.

Hrm... I'd say that sync132 is wrong...

If pkg1 replaces pkg2, I'd expect pkg1 to replace pkg2 whether or not pkg2 has an update.

Allan

Allan

I would actually come out the other way. Where a replacement is *unversioned* like in this test I would expect an updated package to override it. Otherwise it would try to replace the other package on every update even though it's still in a repo. Test replace102 (it would be awesome if we could get some kind of consistent naming scheme going for these tests) handles the situation where you have a versioned replace for a package still in a repo (python 2->3 upgrade).

Huh?

Once a package is replaced, it is gone - no need to replace it again...

At the moment, with pkg1 replacing pkg2 (installed) and an update to pkg2.

pacman -Su (update pkg2) pacman -Su (replace pkg2)

I'm not sure that correct, though. If pkg1 replaces pkg2 but pkg2 is still in the repo (whether or not there's an update) then the replace will not take place if both packages are in the same repo; And if they're not, pkg2 is replaced the first time (only, without the patch, pkg2 can be reinstalled if there's an update). That's why, in my test, I used 2 repos. If the new package (e.g. pkg1, replacing pkg2) was in the same repo, the replacement would not occur. Actually, if would also not occur if the replacing package (pkg1) was in a repo with lower priority (i.e. processed after the one w/ pkg2). Which is why test sync132 still passes, even after the patch applied. When both a package (e.g. pkg2) and a package replacing it (e.g; pkg1) are in the same repo, the package installed is kept (only upgraded if possible). Or am I wrong there? re: test replace102 I'm not sure I understand that test. What is expected to happen in this case? Should python-yaml be upgraded (as happens when running the test), or should it be replaced with python2-yaml as the rule say? Also, what does the expectfailure mean? It's not mentioned in the README, and I'm not sure what an expected failure is/means. -j

...

I think it should be

pacman -Su (replace pkg2) done...

Allan

Allan McRae

10:50 p.m.

New subject: [pacman-dev] [PATCH] libalpm: Fix installing update of a replaced package

On 04/03/13 07:36, jjacky wrote:

...

On 03/03/13 21:41, Allan McRae wrote:

...
On 04/03/13 06:42, Andrew Gregory wrote:

...
On 03/04/13 at 06:29am, Allan McRae wrote:

...
On 04/03/13 06:26, Andrew Gregory wrote:

...
On 03/04/13 at 05:45am, Allan McRae wrote:

...
On 04/03/13 05:43, Allan McRae wrote: > On 01/03/13 04:06, Olivier Brunel wrote: >> During a sysupgrade, if a package is replaced by another, and an update for the >> former package is found (on another repo) the replaced package would be >> re-installed. >> >> Signed-off-by: Olivier Brunel >> --- >> I'm not really familiar with inner workings of ALPM, so this is probably not the >> best way to do this. >> <snip> >> diff --git a/test/pacman/tests/sync1104.py b/test/pacman/tests/sync1104.py >> new file mode 100644 >> index 0000000..5cec98a >> --- /dev/null >> +++ b/test/pacman/tests/sync1104.py >> @@ -0,0 +1,18 @@ >> +self.description = "Don't update (reinstall) a replaced package" >> + >> +lp = pmpkg("old", "1-1") >> +self.addpkg2db("local", lp) >> + >> +p1 = pmpkg("new") >> +p1.provides = ["old"] >> +p1.replaces = ["old"] >> +self.addpkg2db("sync1", p1) >> + >> +p2 = pmpkg("old", "1-2") >> +self.addpkg2db("sync2", p2) >> + >> +self.args = "-Su" >> + >> +self.addrule("PACMAN_RETCODE=0") >> +self.addrule("!PKG_EXIST=old") >> +self.addrule("PKG_EXIST=new") >> >

Forgot to add - awesome for adding a pactest here!

Is the test case correct though? Other than the fact that this one uses two sync repos, this looks identical to sync132 which has the opposite result.

Hrm... I'd say that sync132 is wrong...

If pkg1 replaces pkg2, I'd expect pkg1 to replace pkg2 whether or not pkg2 has an update.

Allan

Allan

I would actually come out the other way. Where a replacement is *unversioned* like in this test I would expect an updated package to override it. Otherwise it would try to replace the other package on every update even though it's still in a repo. Test replace102 (it would be awesome if we could get some kind of consistent naming scheme going for these tests) handles the situation where you have a versioned replace for a package still in a repo (python 2->3 upgrade).

Huh?

Once a package is replaced, it is gone - no need to replace it again...

At the moment, with pkg1 replacing pkg2 (installed) and an update to pkg2.

pacman -Su (update pkg2) pacman -Su (replace pkg2)

I'm not sure that correct, though. If pkg1 replaces pkg2 but pkg2 is still in the repo (whether or not there's an update) then the replace will not take place if both packages are in the same repo; And if they're not, pkg2 is replaced the first time (only, without the patch, pkg2 can be reinstalled if there's an update).

That's why, in my test, I used 2 repos. If the new package (e.g. pkg1, replacing pkg2) was in the same repo, the replacement would not occur. Actually, if would also not occur if the replacing package (pkg1) was in a repo with lower priority (i.e. processed after the one w/ pkg2).

Which is why test sync132 still passes, even after the patch applied. When both a package (e.g. pkg2) and a package replacing it (e.g; pkg1) are in the same repo, the package installed is kept (only upgraded if possible).

Or am I wrong there?

I am not thinking about what currently happens. I purely want to fix what should happen. If pkg1 replaces pkg2, pkg1 is in an equal or higher priority repo than pkg2 and I have pkg2 installed, I _ALWAYS_ expect pkg1 to replace it, no matter what repo the two packages are in and whether or not pkg2 has an update. I also think having pkg2 in the same repo as pkg1 in this situation is a repo management error. Having pkg1 in a higher priority repo than pkg2 is common though.

...

re: test replace102 I'm not sure I understand that test. What is expected to happen in this case? Should python-yaml be upgraded (as happens when running the test), or should it be replaced with python2-yaml as the rule say?

Also, what does the expectfailure mean? It's not mentioned in the README, and I'm not sure what an expected failure is/means.

Expected failure means it will fail and we know that so it should not cause an error on make check. We would still like to fix it. Allan

4070

Age (days ago)

4073

Last active (days ago)

List overview

Download

8 comments

4 participants

participants (4)

Allan McRae
Andrew Gregory
jjacky
Olivier Brunel