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

[Zac Medico]

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.

>> 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).
-- 
Thanks,
Zac