Re: [gentoo-portage-dev] [PATCH] Support disabling docompress for binary package builds

[Michał Górny]

On Sat, 2018-11-03 at 15:44 -0700, Zac Medico wrote:
> On 11/03/2018 01:10 AM, Michał Górny wrote:
> > 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.
>
> I have your patches applied, and for me it treats 'instprep' as a valid
> phase which I'm able to call. It also lists it in the list of available
> phases if I choose an invalid phase named 'foo', since 'instprep' is
> listed in validcommands:
>
> !!! doebuild: 'foo' is not one of the following valid commands:
> !!! clean      cleanrm    compile    config     configure  depend
> !!! digest     fetch      fetchall   help       info       install
> !!! instprep   manifest   merge      nofetch    package    postinst
> !!! postrm     preinst    prepare    prerm      pretend    qmerge
> !!! rpm        setup      test       unmerge    unpack
>
> > 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?
>
> Lets clarify our intention here, since apparently it works for me but
> not for you when you tested. I think it's fine as-is, but let's make
> sure that the behavior is what we collectively intend.

My mistake.  I typed 'prepinst' instead of 'instprep' ;-).

>
> > > 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'.
>
> The actionmap_deps map is just the edges of a directed dependency graph,
> and it currently only covers up to the 'install' phase. The 'package'
> phase currently depends on 'install', which should remain as-is, right?
>
> This change will cause dependencies of 'instprep' to be invoked
> automatically:
>
> --- a/lib/portage/package/ebuild/doebuild.py
> +++ b/lib/portage/package/ebuild/doebuild.py
> @@ -656,6 +656,7 @@ def doebuild(myebuild, mydo,
> _unused=DeprecationWarning, settings=None, debug=0,
>         "compile":["configure"],
>         "test":   ["compile"],
>         "install":["test"],
> +       "instprep":["install"],
>         "rpm":    ["install"],
>         "package":["install"],
>         "merge"  :["install"],
>
> There's no need to make 'merge' depend on 'instprep', since treewalk
> implicitly invokes 'instprep' when 'merge' is called. Adding a
> dependency on 'instprep' would cause it to be invoked *twice* (once by
> spawnebuild and again by treewalk).

Ok, that makes sense.  I'll update the patch with this and ebuild.1
as requested in followup mail.

-- 
Best regards,
Michał Górny