1 |
On 11/02/2018 12:35 PM, Michał Górny wrote: |
2 |
> On Fri, 2018-11-02 at 12:22 -0700, Zac Medico wrote: |
3 |
>> On 11/02/2018 12:16 PM, Michał Górny wrote: |
4 |
>>> On Fri, 2018-11-02 at 11:49 -0700, Zac Medico wrote: |
5 |
>>>> On 11/01/2018 02:50 AM, Michał Górny wrote: |
6 |
>>>>> diff --git a/lib/portage/package/ebuild/doebuild.py b/lib/portage/package/ebuild/doebuild.py |
7 |
>>>>> index 9706de422..d7b535698 100644 |
8 |
>>>>> --- a/lib/portage/package/ebuild/doebuild.py |
9 |
>>>>> +++ b/lib/portage/package/ebuild/doebuild.py |
10 |
>>>>> @@ -674,7 +674,7 @@ def doebuild(myebuild, mydo, _unused=DeprecationWarning, settings=None, debug=0, |
11 |
>>>>> "config", "info", "setup", "depend", "pretend", |
12 |
>>>>> "fetch", "fetchall", "digest", |
13 |
>>>>> "unpack", "prepare", "configure", "compile", "test", |
14 |
>>>>> - "install", "rpm", "qmerge", "merge", |
15 |
>>>>> + "install", "instprep", "rpm", "qmerge", "merge", |
16 |
>>>>> "package", "unmerge", "manifest", "nofetch"] |
17 |
>>>>> |
18 |
>>>>> if mydo not in validcommands: |
19 |
>>>>> @@ -1402,6 +1402,7 @@ def _spawn_actionmap(settings): |
20 |
>>>>> "compile": {"cmd":ebuild_sh, "args":{"droppriv":droppriv, "free":nosandbox, "sesandbox":sesandbox, "fakeroot":0}}, |
21 |
>>>>> "test": {"cmd":ebuild_sh, "args":{"droppriv":droppriv, "free":nosandbox, "sesandbox":sesandbox, "fakeroot":0}}, |
22 |
>>>>> "install": {"cmd":ebuild_sh, "args":{"droppriv":0, "free":0, "sesandbox":sesandbox, "fakeroot":fakeroot}}, |
23 |
>>>>> +"instprep": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox":sesandbox, "fakeroot":fakeroot}}, |
24 |
>>>>> "rpm": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox":0, "fakeroot":fakeroot}}, |
25 |
>>>>> "package": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox":0, "fakeroot":fakeroot}}, |
26 |
>>>>> } |
27 |
>>>> |
28 |
>>>> The feature seems find but the instprep phase is not needed if we use |
29 |
>>>> MiscFunctionsProcess instead of EbuildPhase. Here's a list of odd things |
30 |
>>>> about the instprep ebuild phase implementation as it is: |
31 |
>>>> |
32 |
>>>> 1) You can call instprep explicitly via the ebuild(1) command, but I |
33 |
>>>> doubt that we really want or need to allow that. |
34 |
>>> |
35 |
>>> I was actually wondering about that. I suppose it might be useful for |
36 |
>>> debugging purposes. |
37 |
|
38 |
Sure. In fact, we could split out separate instcompress and inststrip |
39 |
phases. I suppose we could split instprep into multiple phases in the |
40 |
future, if we find a use for it. |
41 |
|
42 |
>>> However, since I wasn't able to figure out how to |
43 |
>>> fully integrate it with the phase machinery, I went for the minimal set |
44 |
>>> of code that wouldn't be definitive either way. |
45 |
>>> |
46 |
>>>> 2) Unlinke other ebuild phases, the doebuild function doesn't created a |
47 |
>>>> .instpreped file to indicate when the instprep phase has executed. |
48 |
>>> |
49 |
>>> I suppose I can try to do that if that's desirable. |
50 |
|
51 |
Note that a .instpreped file could be useful as a means to prevent |
52 |
people from triggering a QA notice about pre-compressed or pre-stripped |
53 |
files if they happen to pass an instprep argument to the ebuild(1) |
54 |
command more than once. Maybe this case is unlikely enough that we can |
55 |
safely neglect it? |
56 |
|
57 |
>>>> 3) Currently instprep executes when FEATURES=binpkg-docompress is |
58 |
>>>> enabled, even though it does nothing of value. I think we should instead |
59 |
>>>> generate a relevant list of MiscFunctionsProcess commands for the |
60 |
>>>> enabled FEATURES, and only start a MiscFunctionsProcess instance if the |
61 |
>>>> list of commands is non-empty. |
62 |
>>> |
63 |
>>> That sounds like premature optimization with a bit of going against |
64 |
>>> principle of least surprise. Given it's a phase function with specific |
65 |
>>> implementation that could be extended in the future, I'd rather avoid |
66 |
>>> hiding additional conditions for running it elsewhere, as it opens |
67 |
>>> a strong possibility that somebody adds additional functionality but |
68 |
>>> forgets to update the condition resulting either in immediate WTF |
69 |
>>> or the new code being skipped only for some users, with even harder-to- |
70 |
>>> debug WTF. |
71 |
>> |
72 |
>> I wasn't really sure that instprep deserved to be a full-fledged phase |
73 |
>> at this point. I guess you're planning to add more stuff there? |
74 |
> |
75 |
> At least stripping. But as usually happens, others may also have more |
76 |
> ideas. I suppose the main use case would be stuff that needs to happen |
77 |
> after install but isn't strictly necessary for building binary packages. |
78 |
|
79 |
Sure, delayed strip will be a useful feature. |
80 |
-- |
81 |
Thanks, |
82 |
Zac |