As pointed out by ulm a while ago: > Ebuilds must not call nor assume the existence of any phase functions. In other words, it's invalid to call pkg_foo from pkg_bar. If phase functions share common code, it should be split into a separate function.
Ref: https://projects.gentoo.org/pms/6/pms.html#x1-940009.1
@ulm, those are all the violations I've been able to find. Do we want to hold this confusing rule?
Uhhhh I'll fix this if you want me to, but are you really sure that I can't assume that a pkg_pretend() function exists if I just defined it on the previous line? pkg_pretend() { # blah blah blah } pkg_setup() { # run again as pkg_pretend is not var safe pkg_pretend } It seems counterproductive to cut/paste the entire pkg_pretend() function into e.g. real_pkg_pretend() and then leave a comment about how it's a workaround for some goofy PMS wording.
Uh, this is not about "some goofy PMS wording"...
(In reply to Ciaran McCreesh from comment #4) > Uh, this is not about "some goofy PMS wording"... Suppose I change it to... real_pkg_pretend() { # everything cut/pasted from pkg_pretend } pkg_pretend() { real_pkg_pretend } pkg_setup() { # run again as pkg_pretend is not var safe real_pkg_pretend } Why am I (not?) allowed to assume that the real_pkg_pretend function exists? The above implementation purposefully overcomplicates a common construct with no obvious benefit to me. What am I missing?
(In reply to Michael Orlitzky from comment #5) > real_pkg_pretend() { > # everything cut/pasted from pkg_pretend > } > > pkg_pretend() { > real_pkg_pretend > } > > pkg_setup() { > # run again as pkg_pretend is not var safe > real_pkg_pretend > }
(In reply to Michael Orlitzky from comment #5) > real_pkg_pretend() { > # everything cut/pasted from pkg_pretend > } > > pkg_pretend() { > real_pkg_pretend > } > > pkg_setup() { > # run again as pkg_pretend is not var safe > real_pkg_pretend > } Indeed, that's how it is supposed to look like (the name of the first function could be improved, though). Trivial to fix, so I don't see what is the problem with it.
(In reply to Ulrich Müller from comment #7) > (In reply to Michael Orlitzky from comment #5) > > real_pkg_pretend() { > > # everything cut/pasted from pkg_pretend > > } > > > > pkg_pretend() { > > real_pkg_pretend > > } > > > > pkg_setup() { > > # run again as pkg_pretend is not var safe > > real_pkg_pretend > > } > > Indeed, that's how it is supposed to look like (the name of the first > function could be improved, though). Trivial to fix, so I don't see what is > the problem with it. I think mostly that the benefit of the change is not being communicated well, so it's seen as a change for no reason.
(In reply to Austin English from comment #8) > I think mostly that the benefit of the change is not being communicated > well, so it's seen as a change for no reason. The short version is, the package mangler is required to do special things to the phase functions to deal with environment saving, defined phases, eclasses, etc. It is not and has never been the case that the package mangler just sources the ebuild file and starts calling bash functions exactly as they're written. To avoid developers having to understand exactly what happens here (which, annoyingly, sometimes has to change when new bash releases come out), it is easiest if the phase functions are treated as being entirely special.
(In reply to Ulrich Müller from comment #7) > > Indeed, that's how it is supposed to look like (the name of the first > function could be improved, though). Trivial to fix, so I don't see what is > the problem with it. The problem is that the next person who looks at that code is going to think I'm an idiot and put it back the way it was. To avoid that, every such fix needs a big warning stating that the weird construct is to satisfy the PMS and should not be reverted to the "obviously correct" version. Should calling phase functions be fatal in a future EAPI? Then we wouldn't need to put a warning next to every workaround.
(In reply to Michael Orlitzky from comment #10) > Should calling phase functions be fatal in a future EAPI? Then we wouldn't > need to put a warning next to every workaround. No need for an EAPI bump, as it never was legal. @mgorny: Would it be difficult to add a repoman warning for this?
(In reply to Ulrich Müller from comment #11) > @mgorny: Would it be difficult to add a repoman warning for this? I don't know. I don't recall how good repoman was at parsing ebuild code. @dev-portage?
(In reply to Michał Górny from comment #12) > (In reply to Ulrich Müller from comment #11) > > @mgorny: Would it be difficult to add a repoman warning for this? > > I don't know. I don't recall how good repoman was at parsing ebuild code. > @dev-portage? Not very good, since it just uses ad-hoc regular expressions, and it will never be able to account for the call stack like a runtime check can.
Note that repoman does have a PhaseCheck class which has the ability to recognize which phase function that a particular line occurs inside of.
(In reply to Zac Medico from comment #14) > Note that repoman does have a PhaseCheck class which has the ability to > recognize which phase function that a particular line occurs inside of. Hmm, I guess that's irrelevant though, because any calls to phase functions should be banned, regardless of the function that they occur inside.
It looks like repoman's NoOffsetWithHelpers check is the closest thing to what you'd want to detect illegal phase function calls.
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/proj/pkgcore/pkgcheck.git/commit/?id=3fcc1f919251c10c1f1ea105f776d7e9c6f5e18e commit 3fcc1f919251c10c1f1ea105f776d7e9c6f5e18e Author: Arthur Zamarin <arthurzam@gentoo.org> AuthorDate: 2023-09-22 09:12:27 +0000 Commit: Arthur Zamarin <arthurzam@gentoo.org> CommitDate: 2023-09-23 15:12:43 +0000 BannedPhaseCall: detect calls of phase functions directly Resolves: https://github.com/pkgcore/pkgcheck/issues/625 Closes: https://bugs.gentoo.org/596616 Signed-off-by: Arthur Zamarin <arthurzam@gentoo.org> src/pkgcheck/checks/codingstyle.py | 12 +++++++++++- .../BadCommandsCheck/BannedPhaseCall/expected.json | 1 + .../BannedPhaseCall/BannedPhaseCall-0.ebuild | 14 ++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-)
