Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 535870 - sys-apps/portage: best_version in pkg_postrm() assumes the package is still installed
Summary: sys-apps/portage: best_version in pkg_postrm() assumes the package is still i...
Status: CONFIRMED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Core - Ebuild Support (show other bugs)
Hardware: All Linux
: Normal normal with 1 vote (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-07 05:27 UTC by Michał Górny
Modified: 2015-01-08 01:11 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 05:27:04 UTC
Test case:

pkg_preinst() { einfo "${FUNCNAME}: $(best_version "${CATEGORY}/${PN}")"; }
pkg_postinst() { einfo "${FUNCNAME}: $(best_version "${CATEGORY}/${PN}")"; }
pkg_prerm() { einfo "${FUNCNAME}: $(best_version "${CATEGORY}/${PN}")"; }
pkg_postrm() { einfo "${FUNCNAME}: $(best_version "${CATEGORY}/${PN}")"; }

On upgrade:

>>> Merging app-misc/testzor-2 to /
 * pkg_preinst: app-misc/testzor-1
>>> Safely unmerging already-installed instance...
 * pkg_prerm: app-misc/testzor-1
No package files given... Grabbing a set.
 * pkg_postrm: app-misc/testzor-1
>>> Original instance of package unmerged safely.
 * pkg_postinst: app-misc/testzor-2
>>> app-misc/testzor-2 merged.

On removal:

 * pkg_prerm: app-misc/testzor-2
 * pkg_postrm: app-misc/testzor-2

I'd say in post-remove, the package should be... removed :).
Comment 1 Ulrich Müller gentoo-dev 2015-01-07 07:44:10 UTC
One could argue that the uninstall is not complete until the last phase function has returned.
Comment 2 Zac Medico gentoo-dev 2015-01-07 08:32:51 UTC
(In reply to Ulrich Müller from comment #1)
> One could argue that the uninstall is not complete until the last phase
> function has returned.

Yes, indeed. Also, REPLACING_VERSIONS makes it possible for ebuilds to distinguish between the relevant states, right? If so, then this quirk should not be a show-stopper for anyone.
Comment 3 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 09:34:10 UTC
(In reply to Ulrich Müller from comment #1)
> One could argue that the uninstall is not complete until the last phase
> function has returned.

Same goes for install, yet pkg_postinst() assumes the package is already installed.

(In reply to Zac Medico from comment #2)
> (In reply to Ulrich Müller from comment #1)
> > One could argue that the uninstall is not complete until the last phase
> > function has returned.
> 
> Yes, indeed. Also, REPLACING_VERSIONS makes it possible for ebuilds to
> distinguish between the relevant states, right? If so, then this quirk
> should not be a show-stopper for anyone.

Well, yes, one has many ways of hacking arounds issues.
Comment 4 Ciaran McCreesh 2015-01-07 16:02:58 UTC
Remember what happened last time someone changed this behaviour...
Comment 5 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 16:29:46 UTC
(In reply to Ciaran McCreesh from comment #4)
> Remember what happened last time someone changed this behaviour...

Maybe you could 'remind' us? Because I'm not aware of it, and nobody seems to have bothered to document it in PMS.
Comment 6 Ciaran McCreesh 2015-01-07 16:35:34 UTC
(In reply to Michał Górny from comment #5)
> (In reply to Ciaran McCreesh from comment #4)
> > Remember what happened last time someone changed this behaviour...
> 
> Maybe you could 'remind' us? Because I'm not aware of it, and nobody seems
> to have bothered to document it in PMS.

The behaviour was changed without EAPI control, breaking every package that used a version check to display a different message when upgrading. PMS got retroactively amended to match Portage's new behaviour. Someone spent ages fixing the tree. The Council promised that this would never be allowed to happen again.
Comment 7 Ulrich Müller gentoo-dev 2015-01-07 16:55:59 UTC
(In reply to Ciaran McCreesh from comment #6)
Do you refer to the change in call order (bug 174328, bug 338339)?

What does Paludis output for the test case in comment #0? The same as Portage?

Or maybe we should just say that the result of a self-referential call to has_version() or best_version(), while in a transitional install phase, is undefined.
Comment 8 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 17:25:21 UTC
I'll summarize the results with all 3 PMs in a minute.
Comment 9 Ciaran McCreesh 2015-01-07 17:28:30 UTC
I think it might be EAPI dependent in Paludis.
Comment 10 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 17:34:11 UTC
                            portage         pkgcore         paludis
1. install testzor-1

preinst                     (none)          (none)          (none)
postinst                    testzor-1       testzor-1       testzor-1

2. upgrade to testzor-2

preinst                     testzor-1       testzor-1       testzor-1
prerm                       testzor-1       testzor-1       testzor-2
postrm                      testzor-1       testzor-1       testzor-2
postinst                    testzor-2       testzor-1       testzor-2

3. remove testzor-2

prerm                       testzor-2       testzor-2       testzor-2
postrm                      testzor-2       (none)          testzor-2

Tested with EAPI 5.
Comment 11 Ciaran McCreesh 2015-01-07 17:39:51 UTC
Pretty sure it's dependent both upon the EAPI of the package being removed, and the EAPI of the package being installed in Paludis. We were pretty careful with this.
Comment 12 Ulrich Müller gentoo-dev 2015-01-07 18:38:27 UTC
"Behaviour for a query matching the atom of the ebuild itself is undefined in pkg_preinst, pkg_postinst, pkg_prerm, and pkg_postrm."

This would be added to the first paragraph of section 11.3.3.4 "Package manager query commands". http://dev.gentoo.org/~ulm/pms/5/pms.html#x1-13200011.3.3.4
Comment 13 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 19:13:33 UTC
Sure. But provide a useful behavior for EAPI 6. There's no point in having a spec that doesn't allow you to solve stuff easily, especially when a easy solution is easy to implement.
Comment 14 Tim Harder gentoo-dev 2015-01-07 19:55:33 UTC
It seems like making this consistent would requiring defining when the livefs (files/vdb/etc) must be written to for various operations (install/replace/remove).

In my opinion, I think pkgcore clearly got it wrong for replacements so now it should provide the following output instead which I think is what most people would expect if the behavior for a query matching the atom of the ebuild itself was allowed to  be undefined.

                            pkgcore
1. install testzor-1
 
preinst                     (none)
postinst                    testzor-1
 
2. upgrade to testzor-2
 
preinst                     testzor-1
prerm                       testzor-1
postrm                      testzor-2
postinst                    testzor-2
 
3. remove testzor-2
 
prerm                       testzor-2
postrm                      (none)
Comment 15 Tim Harder gentoo-dev 2015-01-07 19:57:13 UTC
(In reply to Tim Harder from comment #14)
> In my opinion, I think pkgcore clearly got it wrong for replacements so now
> it should provide the following output instead which I think is what most
> people would expect if the behavior for a query matching the atom of the
> ebuild itself was allowed to  be undefined.

To correct myself, I meant if the behavior for a query matching the atom of the ebuild itself *wasn't* allowed to be undefined.
Comment 16 Ciaran McCreesh 2015-01-07 20:01:50 UTC
(In reply to Tim Harder from comment #14)
> In my opinion, I think pkgcore clearly got it wrong for replacements so now
> it should provide the following output instead which I think is what most
> people would expect if the behavior for a query matching the atom of the
> ebuild itself was allowed to  be undefined.

But that all depends upon the EAPI of the package being removed... The pkg_ phases aren't necessarily run in the order you expect.
Comment 17 Tim Harder gentoo-dev 2015-01-07 20:06:53 UTC
(In reply to Ciaran McCreesh from comment #16)
> (In reply to Tim Harder from comment #14)
> > In my opinion, I think pkgcore clearly got it wrong for replacements so now
> > it should provide the following output instead which I think is what most
> > people would expect if the behavior for a query matching the atom of the
> > ebuild itself was allowed to  be undefined.
> 
> But that all depends upon the EAPI of the package being removed... The pkg_
> phases aren't necessarily run in the order you expect.

Can you point out where the pkg_ phase order changes in PMS due to EAPI? All I see is the note about EAPI 0 and 1 where the phase functions can (not must) be called in a different order which it also mentions is deprecated.

Also thinking about it a bit more, probably paludis does it the expected way for replacements if we assume to write the new pkg changes to the livefs directly after pkg_preinst as PMS alludes to.
Comment 18 Ciaran McCreesh 2015-01-07 20:12:46 UTC
(In reply to Tim Harder from comment #17)
> Can you point out where the pkg_ phase order changes in PMS due to EAPI?

This is the thing from Comment #6 where Portage changed behaviours and PMS got retroactively changed. Paludis implements the old Portage behaviour for old EAPIs, and the new Portage behaviour for new EAPIs.
Comment 19 Tim Harder gentoo-dev 2015-01-07 20:16:26 UTC
(In reply to Ciaran McCreesh from comment #18)
> (In reply to Tim Harder from comment #17)
> > Can you point out where the pkg_ phase order changes in PMS due to EAPI?
> 
> This is the thing from Comment #6 where Portage changed behaviours and PMS
> got retroactively changed. Paludis implements the old Portage behaviour for
> old EAPIs, and the new Portage behaviour for new EAPIs.

Ah thanks, that makes more sense now.
Comment 20 Ulrich Müller gentoo-dev 2015-01-07 20:31:02 UTC
Is that really worth the effort? Since we have introduced REPLACING_VERSIONS and REPLACED_BY_VERSION variables, I'd consider best_version and has_version for the package itself as deprecated.

Updated wording:
"In the pkg_preinst, pkg_postinst, pkg_prerm, and pkg_postrm functions, behaviour is undefined for a query matching the atom of the ebuild itself, the atom replacing it, or the atom being replaced by it."
Comment 21 Ciaran McCreesh 2015-01-07 20:39:10 UTC
It's broken with Portage's parallel shenanigans too, so I'd say it's just plain undefined behaviour entirely, and should be banned in every phase.
Comment 22 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 21:26:46 UTC
(In reply to Ulrich Müller from comment #20)
> Is that really worth the effort? Since we have introduced REPLACING_VERSIONS
> and REPLACED_BY_VERSION variables, I'd consider best_version and has_version
> for the package itself as deprecated.

REPLAC* variables don't work for multi-slot packages.
Comment 23 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 22:46:36 UTC
(In reply to Ulrich Müller from comment #12)
> "Behaviour for a query matching the atom of the ebuild itself is undefined
> in pkg_preinst, pkg_postinst, pkg_prerm, and pkg_postrm."
> 
> This would be added to the first paragraph of section 11.3.3.4 "Package
> manager query commands".
> http://dev.gentoo.org/~ulm/pms/5/pms.html#x1-13200011.3.3.4

Hmm, please make that reference only the current slot. I don't see why you couldn't reference other slots.
Comment 24 Ciaran McCreesh 2015-01-07 22:50:56 UTC
(In reply to Michał Górny from comment #23)
> Hmm, please make that reference only the current slot. I don't see why you
> couldn't reference other slots.

Portage might be changing other slots in parallel...
Comment 25 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2015-01-07 22:55:39 UTC
As well as doing anything to any other random packages. Your point?
Comment 26 Ciaran McCreesh 2015-01-07 23:01:58 UTC
The point is you can't expect them to work on other packages or slots either.
Comment 27 Ulrich Müller gentoo-dev 2015-01-08 01:11:30 UTC
(In reply to Michał Górny from comment #23)
> Hmm, please make that reference only the current slot. I don't see why you
> couldn't reference other slots.

I thought that the wording in comment #20 would account for it: "a query matching the atom of the ebuild itself, the atom replacing it, or the atom being replaced by it". This can match only the current slot.


(In reply to Ciaran McCreesh from comment #24)
> Portage might be changing other slots in parallel...

Yes, or the package manager might be installing or removing the target of the query at some later point. So best_version is inherently unreliable and shouldn't be used for anything critical. But was this ever different?