Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 516014

Summary: [Future EAPI] call eclass phase functions from all eclasses by default
Product: Gentoo Hosted Projects Reporter: Patrick McLean <chutzpah>
Component: PMS/EAPIAssignee: PMS/EAPI <pms>
Status: CONFIRMED ---    
Severity: normal CC: esigra, hasufell, pacho, sam, williamh
Priority: Normal    
Version: unspecified   
Hardware: All   
OS: Linux   
See Also: https://bugs.gentoo.org/show_bug.cgi?id=241442
https://bugs.gentoo.org/show_bug.cgi?id=795006
https://bugs.gentoo.org/show_bug.cgi?id=585432
Whiteboard:
Package list:
Runtime testing required: ---
Bug Depends on:    
Bug Blocks: 174380    

Description Patrick McLean gentoo-dev 2014-07-01 17:34:16 UTC
Currently portage only calls the eclass phase functions from the last-inherited eclass rather than all inherited eclasses. This behaviour is completely non-intuitive. This also results in a large number of ebuilds that define phase functions that only call the eclass functions from the eclasses they inherit.

I propose that the default implementations of all phase functions call the phase functions of every inherited class. This will still be overridable by overriding the phase function in the ebuild.

Reproducible: Always
Comment 1 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-07-01 17:38:31 UTC
Sounds crazy. So maybe it could work. Though updating ebuilds to a new EAPI would be full of surprises.
Comment 2 Andreas K. Hüttel archtester gentoo-dev 2014-07-01 17:58:27 UTC
A nice add-on to this proposal would be that from that EAPI on every eclass exports automatically *all* phase functions (just empty stubs if not defined explicitly in the eclass).

We can't do that right now since the stubs might overwrite functional phase functions from other eclasses. If the phase functions of every inherited class are called, that does not matter anymore.
Comment 3 Ciaran McCreesh 2014-07-01 18:03:42 UTC
The solution here is to stop writing so many eclasses that override phase functions, and to just export utility functions instead...
Comment 4 Julian Ospald 2014-07-30 00:36:12 UTC
(In reply to Ciaran McCreesh from comment #3)
> The solution here is to stop writing so many eclasses that override phase
> functions, and to just export utility functions instead...

I can't belive that I have to agree with you again.

Ebuild writing is already so full of random pitfalls, that a lot of people just give up on it. And that IS a problem.
Comment 5 Patrick McLean gentoo-dev 2014-07-30 01:52:11 UTC
(In reply to Julian Ospald (hasufell) from comment #4)
> (In reply to Ciaran McCreesh from comment #3)
> > The solution here is to stop writing so many eclasses that override phase
> > functions, and to just export utility functions instead...
> 
> I can't belive that I have to agree with you again.
> 
> Ebuild writing is already so full of random pitfalls, that a lot of people
> just give up on it. And that IS a problem.

Even if moving to utility function only eclasses (which would make a lot of the ebuilds in the tree _much_ more complicated) becomes a long-term plan, the current behaviour completely violates the principle of least surprise.

I work with a lot of inexperienced ebuild developers, and the current behaviour is very surprising to them, it's one of the pitfalls.
Comment 6 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-07-30 06:54:18 UTC
Then I'm guess I'm terribly stupid since I call a random third-in-a-line indirect inherit setting part of phase function a terrible and hard-to-debug surprise.
Comment 7 Ulrich Müller gentoo-dev 2014-07-30 10:27:32 UTC
(In reply to Patrick McLean from comment #0)
> Currently portage only calls the eclass phase functions from the
> last-inherited eclass rather than all inherited eclasses. This behaviour is
> completely non-intuitive.

Why do you think so?

> This also results in a large number of ebuilds that define phase functions
> that only call the eclass functions from the eclasses they inherit.

IMHO, this is a good thing.

I wonder if we shouldn't make it a policy that an ebuild must define an explicit phase function, whenever it inherits eclasses with conflicting exports for that function.
Comment 8 Julian Ospald 2014-07-31 00:45:03 UTC
(In reply to Ulrich Müller from comment #7)
> (In reply to Patrick McLean from comment #0)
> > This also results in a large number of ebuilds that define phase functions
> > that only call the eclass functions from the eclasses they inherit.
> 
> IMHO, this is a good thing.
> 
> I wonder if we shouldn't make it a policy that an ebuild must define an
> explicit phase function, whenever it inherits eclasses with conflicting
> exports for that function.

Or rather just disable ALL exported functions if there are more than one potential candidates.
Comment 9 William Hubbs gentoo-dev 2014-08-14 16:14:19 UTC
(In reply to Julian Ospald (hasufell) from comment #4)
> (In reply to Ciaran McCreesh from comment #3)
> > The solution here is to stop writing so many eclasses that override phase
> > functions, and to just export utility functions instead...
> 
> I can't belive that I have to agree with you again.
> 
> Ebuild writing is already so full of random pitfalls, that a lot of people
> just give up on it. And that IS a problem.

I agree with these two comments. In fact, I suggest taking it a step further.
I propose that we completely disable the EXPORT_FUNCTIONS call, preferably in eapi 6.

Yes that does mean that there would be a lot of work to migrate ebuilds to eapi 6 that were depending on eclass exported phase functions, but in the future things will be much cleaner because these pitfalls will be eliminated.
Comment 10 Ulrich Müller gentoo-dev 2014-08-14 22:06:56 UTC
(In reply to William Hubbs from comment #9)
> I agree with these two comments. In fact, I suggest taking it a step further.
> I propose that we completely disable the EXPORT_FUNCTIONS call, preferably
> in eapi 6.

You can have this already now, and without the need for an EAPI bump: Just don't call EXPORT_FUNCTIONS any more.
Comment 11 William Hubbs gentoo-dev 2014-08-14 23:59:52 UTC
(In reply to Ulrich Müller from comment #10)
> (In reply to William Hubbs from comment #9)
> > I agree with these two comments. In fact, I suggest taking it a step further.
> > I propose that we completely disable the EXPORT_FUNCTIONS call, preferably
> > in eapi 6.
> 
> You can have this already now, and without the need for an EAPI bump: Just
> don't call EXPORT_FUNCTIONS any more.

My understanding is that we can't just drop the EXPORT_FUNCTIONS call from an eclass, because that would cause pretty wide breakage, including breaking the stable tree. For each eclass that calls EXPORT_FUNCTIONS we would have to create a new eclass that doesn't, then migrate all of the consumers to the new eclass, then finally remove the old eclass after all of its consumers are out of the tree. A quick check reveals 138 eclasses that call EXPORT_FUNCTIONS.

On the other hand, if we turn it off with an EAPI bump, it means that ebuilds would be rewritten as they are migrated (this would have to be a revbump or version bump), and ebuilds with older eapis would still work.
Comment 12 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-08-15 06:16:08 UTC
You are trying to move this from one extreme to another. I already see all those simple ebuilds:

src_prepare() { perl-module_src_prepare; }
src_configure() { perl-module_src_configure; }
src_compile() { perl-module_src_compile; }
src_test() { perl-module_src_test; }
src_install() { perl-module_src_install; }

And just hope I didn't omit a single phase because there's nothing to warn me about that. And I just removed forced games.eclass inherit to remove a similar block from my ebuild...

Maybe you should do this the games team way -- create new eclass with useless phase functions and make people inherit it last. Then it will wipe out phases and you will have to call them explicitly.
Comment 13 Ulrich Müller gentoo-dev 2014-08-15 07:49:38 UTC
(In reply to Michał Górny from comment #12)
> Maybe you should do this the games team way -- create new eclass with
> useless phase functions and make people inherit it last. Then it will wipe
> out phases and you will have to call them explicitly.

How about two of them? ;-)

empty.eclass:
src_unpack() { :; }
src_prepare() { :; }
[...]

default.eclass:
src_unpack() { default; }
src_prepare() { default; }
[...]
Comment 14 Pacho Ramos gentoo-dev 2014-08-15 10:07:44 UTC
Personally I wouldn't like the idea of needing to call each phase explicitly in every ebuild using gnome2.eclass (for example). On the other hand, I would change the behavior in future eapi to not export any phase *if* eclasses are exporting that colliding phases. For example:
- If an ebuild is inheriting autotools-utils.eclass and gnome2.eclass, it will have "colliding" src_configure default phases and, with my suggestion, it should simply die if the phase is not specified in ebuild to ensure it does what developer expects.
- If an ebuild is inheriting only gnome2.eclass it will end up running gnome2_src_configure as it does currently
Comment 15 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-08-15 10:26:39 UTC
What about indirect inherits? E.g. when xorg-x11 inherits autotools-utils and uses its phase function in its own.
Comment 16 Pacho Ramos gentoo-dev 2014-08-15 10:35:32 UTC
(In reply to Michał Górny from comment #15)
> What about indirect inherits? E.g. when xorg-x11 inherits autotools-utils
> and uses its phase function in its own.

In that case, ebuild would inherit xorg-x11 and, if it's not inheriting autotools-utils directly from ebuild (as xorg-x11.eclass will inherit the autotools-utils.eclass for calling the phases with the parameters they want) , nothing should change.

Why should the ebuild inheritting xorg-x11.eclass to also inherit autotools-utils.eclass for running the xorg-x11 phase that relies on autotools-utils.eclass? If it inherits both from ebuild developer will need to choose what phase wants to get executed instead of relying on the order of inheriting
Comment 17 Pacho Ramos gentoo-dev 2014-08-15 10:36:30 UTC
What I am trying to say is that I am referring to the direct inherits only, as I guess each eclass will properly inherit the eclass it needs and we shouldn't play with that
Comment 18 William Hubbs gentoo-dev 2014-08-15 15:31:28 UTC
(In reply to Michał Górny from comment #12)
> You are trying to move this from one extreme to another. I already see all
> those simple ebuilds:
> 
> src_prepare() { perl-module_src_prepare; }
> src_configure() { perl-module_src_configure; }
> src_compile() { perl-module_src_compile; }
> src_test() { perl-module_src_test; }
> src_install() { perl-module_src_install; }

That's right, and if you leave one out, The default phase function is the one that runs. You know, just from reading the ebuild, what will be run.

> And just hope I didn't omit a single phase because there's nothing to warn
> me about that.

This is nothing new. There is nothing that warns you about leaving out a phase function. The default phase function is the one that runs if you do this.

> And I just removed forced games.eclass inherit to remove a
> similar block from my ebuild...
> 
> Maybe you should do this the games team way -- create new eclass with
> useless phase functions and make people inherit it last. Then it will wipe
> out phases and you will have to call them explicitly.

Your eclass proposal is not good either because it wipes out the default phase functions.

(In reply to Pacho Ramos from comment #14)
> Personally I wouldn't like the idea of needing to call each phase explicitly
> in every ebuild using gnome2.eclass (for example). On the other hand, I
> would change the behavior in future eapi to not export any phase *if*
> eclasses are exporting that colliding phases. For example:
> - If an ebuild is inheriting autotools-utils.eclass and gnome2.eclass, it
> will have "colliding" src_configure default phases and, with my suggestion,
> it should simply die if the phase is not specified in ebuild to ensure it
> does what developer expects.

This might be a compromise, but I'm thinking about a better way to write the rule.

I think the rule would have to be that EXPORT_FUNCTIONS could only export a phase function one time, and trying to do so again would be an error condition. But, this means the first export of the phase function is the one that would take affect; not the last.
Comment 19 William Hubbs gentoo-dev 2014-08-15 16:31:48 UTC
I strongly feel, however, that comment #9 and the agreeing comments are the best answer, because, if we do that, from eapi 6 onwards, there would never be undefined behavior related to phase functions and inheritance of eclasses. At the same time, things would stay as they are for current ebuilds.
Comment 20 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-08-15 16:36:22 UTC
Comment 9 and onwards are trying to solve the wrong issue at the cost of high workload increase and increased risk of fault in simple ebuilds.
Comment 21 Patrick McLean gentoo-dev 2014-08-15 17:09:20 UTC
The proposal to disable phase functions completely basically makes eclasses considerably less useful. For example, a large number of the python ebuilds in the tree only set variables such as *DEPEND, DESCRIPTION and SRC_URI etc and let the eclasses do the rest.

With the proposal to disable eclass phases, then every python ebuild would have to define 5 or 6 phases, and hopefully not forget one. It seems like a way to make ebuild writing considerably more work, with very little gain.

Part of the point of my proposal is to make "declarative" ebuilds that simply inherit some standard eclasses and set some variables more flexible. It would be possible to use that style of ebuild for more diverse use cases than is currently possible. Since these sorts of ebuilds rely much more on well-exercised eclass code than custom per-ebuild functions, they could be less buggy, rather than more so.

Perhaps we may want to use different functions in eclasses if we know that phases of sub-classes will be automatically run, we could add some mechanism to make that easier or eclass authors could just block out their function definitions in EAPI-specific blocks.
Comment 22 Patrick McLean gentoo-dev 2014-08-15 17:22:32 UTC
Another thing I want to make clear here, if the phase function is defined in the ebuild, then unless default is called _no_ eclass functions will be automatically run.

You can disable eclass-provided with something like this:
src_configure() { :; }

Perhaps we could make this functionality optional, ie opt-in or opt-out, something like:
ECLASS_PHASES=1

Or even have a parameter to inherit that enables this functionality:

inherit -p .....
Comment 23 Patrick McLean gentoo-dev 2014-08-15 18:12:06 UTC
Some clarification, from a discussion on IRC.

In the case an ebuild contains:

inherit foo bar

Normally this would cause the phases to be run in this order:
foo_phase1()
bar_phase1()

But if foo.eclass contains:

inherit bar

Then the order would be:
bar_phase1()
foo_phase1()

That would make the phases from bar run before foo, despite the ebuild inheriting bar second. This allows eclasses to rely on their sub-eclasses phases being run. There would be duplicate elimination performed, so no eclass phase would be called more than once. The phase functions would always be called at the earliest time the inherit tree would imply.

For the PM-default phase implementations, these could be made idempotent, so if an eclass explicitly calls default(), then it gets run, then if another eclass calls it further along, then it will be a no-op.

If an ebuild has a complicated chain of phase dependencies that must be run in a specific order, with the PM default being run in a specific location along then chain, then the ebuild would just define the phase function and explicitly call the eclass phase functions.

We could separate this functionality from default() for ebuilds. If an ebuild defines a phase, then it can call default() for the PM default, and another function to run the eclass-defined phases automatically. In the case where there is 

phase1() {
Comment 24 Patrick McLean gentoo-dev 2014-08-15 18:15:39 UTC
Some clarification, from a discussion on IRC.

In the case an ebuild contains:

inherit foo bar

Normally this would cause the phases to be run in this order:
foo_phase1()
bar_phase1()

But if foo.eclass contains:

inherit bar

Then the order would be:
bar_phase1()
foo_phase1()

That would make the phases from bar run before foo, despite the ebuild inheriting bar second. This allows eclasses to rely on their sub-eclasses phases being run. There would be duplicate elimination performed, so no eclass phase would be called more than once. The phase functions would always be called at the earliest time the inherit tree would imply.

For the PM-default phase implementations, these could be made idempotent, so if an eclass explicitly calls default(), then it gets run, then if another eclass calls it further along, then it will be a no-op.

If an ebuild has a complicated chain of phase dependencies that must be run in a specific order, with the PM default being run in a specific location along then chain, then the ebuild would just define the phase function and explicitly call the eclass phase functions.

We could separate this functionality from default() for ebuilds. If an ebuild defines a phase, then it can call default() for the PM default, and another function to run the eclass-defined phases automatically (for example it could be called eclasses()). In the case where there is no phase defined in the ebuild, the PM would run s

phase1() {
   eclasses
   default
}

If the ebuild defines the phase, it can call those functions as it likes, so something like:

inherit eclass2 eclass1 eclass3

phase1() {
    something ...
    more stuff ...
    eclass1_phase1
    eclass2_phase2
    default
}

This would do what one would expect, so even if eclass3 defines phase1, in this case it would _not_ be run.
Comment 25 William Hubbs gentoo-dev 2014-08-16 18:13:15 UTC
There is actually another way to handle this that might be worth considering.
What do folks think of turning off EXPORT_FUNCTIONS and adding another variable to ebuild global scope that lists the eclass phase functions the ebuild uses, e.g.

ECLASS_PHASES="foo_src_unpack
    foo_src_compile
    bar_src_compile
    bas_src_install
    ..."

The default pms phase function for each phase could check this variable and run eclass phase functions for the current phase in the order they are listed. If the variable is empty or no eclass phases for the current phase are listed, no eclass phase functions are run.

This minimizes boilerplating and still removes the automatic running of eclass phase functions which I think is where the confusion lies.
Comment 26 Michał Górny archtester Gentoo Infrastructure gentoo-dev Security 2014-08-16 21:44:10 UTC
You are still working hard on finding an acceptable solution for the wrong problem. If you really want to pursue that, please start by specifying the problem which you are trying to solve and the specific reason for the solutions you're trying to do. Preferably, please do that in a separate bug since this one is about something completely opposite and the comments have diverged hard from the original issue.