Gentoo Archives: gentoo-portage-dev

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

Attachments

File name MIME type
signature.asc application/pgp-signature

Replies