On Fri, 2018-11-02 at 19:35 -0700, Zac Medico wrote:
> On 11/02/2018 12:35 PM, Michał Górny wrote:
> > On Fri, 2018-11-02 at 12:22 -0700, Zac Medico wrote:
> > > On 11/02/2018 12:16 PM, Michał Górny wrote:
> > > > On Fri, 2018-11-02 at 11:49 -0700, Zac Medico wrote:
> > > > > On 11/01/2018 02:50 AM, Michał Górny wrote:
> > > > > > diff --git a/lib/portage/package/ebuild/doebuild.py
> > > > > > b/lib/portage/package/ebuild/doebuild.py
> > > > > > index 9706de422..d7b535698 100644
> > > > > > --- a/lib/portage/package/ebuild/doebuild.py
> > > > > > +++ b/lib/portage/package/ebuild/doebuild.py
> > > > > > @@ -674,7 +674,7 @@ def doebuild(myebuild, mydo,
> > > > > > _unused=DeprecationWarning, settings=None, debug=0,
> > > > > > "config", "info", "setup", "depend", "pretend",
> > > > > > "fetch", "fetchall", "digest",
> > > > > > "unpack", "prepare", "configure", "compile",
> > > > > > "test",
> > > > > > - "install", "rpm", "qmerge", "merge",
> > > > > > + "install", "instprep", "rpm", "qmerge", "merge",
> > > > > > "package", "unmerge", "manifest", "nofetch"]
> > > > > >
> > > > > > if mydo not in validcommands:
> > > > > > @@ -1402,6 +1402,7 @@ def _spawn_actionmap(settings):
> > > > > > "compile": {"cmd":ebuild_sh, "args":{"droppriv":droppriv,
> > > > > > "free":nosandbox, "sesandbox":sesandbox, "fakeroot":0}},
> > > > > > "test": {"cmd":ebuild_sh, "args":{"droppriv":droppriv,
> > > > > > "free":nosandbox, "sesandbox":sesandbox, "fakeroot":0}},
> > > > > > "install": {"cmd":ebuild_sh, "args":{"droppriv":0,
> > > > > > "free":0, "sesandbox":sesandbox, "fakeroot":fakeroot}},
> > > > > > +"instprep": {"cmd":misc_sh, "args":{"droppriv":0,
> > > > > > "free":0, "sesandbox":sesandbox, "fakeroot":fakeroot}},
> > > > > > "rpm": {"cmd":misc_sh, "args":{"droppriv":0,
> > > > > > "free":0, "sesandbox":0, "fakeroot":fakeroot}},
> > > > > > "package": {"cmd":misc_sh, "args":{"droppriv":0,
> > > > > > "free":0, "sesandbox":0, "fakeroot":fakeroot}},
> > > > > > }
> > > > >
> > > > > The feature seems find but the instprep phase is not needed if we use
> > > > > MiscFunctionsProcess instead of EbuildPhase. Here's a list of odd
> > > > > things
> > > > > about the instprep ebuild phase implementation as it is:
> > > > >
> > > > > 1) You can call instprep explicitly via the ebuild(1) command, but I
> > > > > doubt that we really want or need to allow that.
> > > >
> > > > I was actually wondering about that. I suppose it might be useful for
> > > > debugging purposes.Ok, I've rechecked it and ebuild(1) currently rejects it as not a valid phase name. So we can either ignore the problem until somebody finds it useful to call it directly, or work on it. Would the existing patch be ok, provided that ebuild(1) doesn't allow calling prepinst manually? > Sure. In fact, we could split out separate instcompress and inststrip > phases. I suppose we could split instprep into multiple phases in the > future, if we find a use for it. > > > > > However, since I wasn't able to figure out how to > > > > fully integrate it with the phase machinery, I went for the minimal set > > > > of code that wouldn't be definitive either way. > > > > > > > > > 2) Unlinke other ebuild phases, the doebuild function doesn't created > > > > > a > > > > > .instpreped file to indicate when the instprep phase has executed. > > > > > > > > I suppose I can try to do that if that's desirable. > > Note that a .instpreped file could be useful as a means to prevent > people from triggering a QA notice about pre-compressed or pre-stripped > files if they happen to pass an instprep argument to the ebuild(1) > command more than once. Maybe this case is unlikely enough that we can > safely neglect it? That should be easy enough to implement. What was definitely harder for me to comprehend, are phase dependencies. I wasn't sure whether adding them to actionmap wouldn't make it possible for 'preinst' to be called automatically before 'package'. > > > > > 3) Currently instprep executes when FEATURES=binpkg-docompress is > > > > > enabled, even though it does nothing of value. I think we should > > > > > instead > > > > > generate a relevant list of MiscFunctionsProcess commands for the > > > > > enabled FEATURES, and only start a MiscFunctionsProcess instance if > > > > > the > > > > > list of commands is non-empty. > > > > > > > > That sounds like premature optimization with a bit of going against > > > > principle of least surprise. Given it's a phase function with specific > > > > implementation that could be extended in the future, I'd rather avoid > > > > hiding additional conditions for running it elsewhere, as it opens > > > > a strong possibility that somebody adds additional functionality but > > > > forgets to update the condition resulting either in immediate WTF > > > > or the new code being skipped only for some users, with even harder-to- > > > > debug WTF. > > > > > > I wasn't really sure that instprep deserved to be a full-fledged phase > > > at this point. I guess you're planning to add more stuff there? > > > > At least stripping. But as usually happens, others may also have more > > ideas. I suppose the main use case would be stuff that needs to happen > > after install but isn't strictly necessary for building binary packages. > > Sure, delayed strip will be a useful feature. -- Best regards, Michał Górny
signature.asc
Description: This is a digitally signed message part
