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: Fri, 02 Nov 2018 19:35:26
Message-Id: 1541187316.24926.13.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 12:22 -0700, Zac Medico wrote:
2 > On 11/02/2018 12:16 PM, Michał Górny wrote:
3 > > On Fri, 2018-11-02 at 11:49 -0700, Zac Medico wrote:
4 > > > On 11/01/2018 02:50 AM, Michał Górny wrote:
5 > > > > Add FEATURES=binpkg-docompress that can be used whether docompress
6 > > > > compression is performed before or after creating binary packages. With
7 > > > > the feature enabled (the default), the current behavior of storing
8 > > > > compressed files in binpkg is preserved. With it disabled, uncompressed
9 > > > > files are stored inside binary package and are compressed when
10 > > > > installing.
11 > > > >
12 > > > > Storing uncompressed files in binary packages has two advantages:
13 > > > >
14 > > > > 1. Avoids the double-compression penalty, effectively improving binary
15 > > > > package compression speed and compression ratio.
16 > > > >
17 > > > > 2. Allows the same packages to be reused on systems with different
18 > > > > docompress configurations.
19 > > > >
20 > > > > The option is roughly backwards compatible. Old Portage versions will
21 > > > > install packages created with FEATURES=-binpkg-docompress correctly,
22 > > > > albeit without compression. Portage with FEATURES=binpkg-docompress
23 > > > > should install old binpackages semi-correctly, potentially recompressing
24 > > > > them (and throwing already-compressed warnings on format mismatch).
25 > > > > The new behavior is left off by default to avoid those problems.
26 > > > >
27 > > > > Signed-off-by: Michał Górny <mgorny@g.o>
28 > > > > ---
29 > > > > bin/misc-functions.sh | 34 +++++++++++++++++++++++---
30 > > > > cnf/make.globals | 2 +-
31 > > > > lib/portage/const.py | 1 +
32 > > > > lib/portage/dbapi/vartree.py | 12 +++++++++
33 > > > > lib/portage/package/ebuild/doebuild.py | 3 ++-
34 > > > > man/make.conf.5 | 6 +++++
35 > > > > 6 files changed, 52 insertions(+), 6 deletions(-)
36 > > > >
37 > > > > diff --git a/bin/misc-functions.sh b/bin/misc-functions.sh
38 > > > > index db7aaed5a..96aa66611 100755
39 > > > > --- a/bin/misc-functions.sh
40 > > > > +++ b/bin/misc-functions.sh
41 > > > > @@ -109,10 +109,13 @@ install_qa_check() {
42 > > > >
43 > > > > [[ -d ${ED%/}/usr/share/info ]] && prepinfo
44 > > > >
45 > > > > - # Apply compression.
46 > > > > - "${PORTAGE_BIN_PATH}"/ecompress --queue "${PORTAGE_DOCOMPRESS[@]}"
47 > > > > - "${PORTAGE_BIN_PATH}"/ecompress --ignore "${PORTAGE_DOCOMPRESS_SKIP[@]}"
48 > > > > - "${PORTAGE_BIN_PATH}"/ecompress --dequeue
49 > > > > + # If binpkg-docompress is enabled, apply compression before creating
50 > > > > + # the binary package.
51 > > > > + if has binpkg-docompress ${FEATURES}; then
52 > > > > + "${PORTAGE_BIN_PATH}"/ecompress --queue "${PORTAGE_DOCOMPRESS[@]}"
53 > > > > + "${PORTAGE_BIN_PATH}"/ecompress --ignore "${PORTAGE_DOCOMPRESS_SKIP[@]}"
54 > > > > + "${PORTAGE_BIN_PATH}"/ecompress --dequeue
55 > > > > + fi
56 > > > >
57 > > > > export STRIP_MASK
58 > > > > if ___eapi_has_dostrip; then
59 > > > > @@ -160,6 +163,29 @@ install_qa_check() {
60 > > > > rm -f "${ED%/}"/usr/share/info/dir{,.gz,.bz2} || die "rm failed!"
61 > > > > }
62 > > > >
63 > > > > +__dyn_instprep() {
64 > > > > + if has chflags ${FEATURES}; then
65 > > > > + # Save all the file flags for restoration afterwards.
66 > > > > + mtree -c -p "${ED}" -k flags > "${T}/bsdflags.mtree"
67 > > > > + # Remove all the file flags so that we can do anything necessary.
68 > > > > + chflags -R noschg,nouchg,nosappnd,nouappnd "${ED}"
69 > > > > + chflags -R nosunlnk,nouunlnk "${ED}" 2>/dev/null
70 > > > > + fi
71 > > > > +
72 > > > > + # If binpkg-docompress is disabled, we need to apply compression
73 > > > > + # before installing.
74 > > > > + if ! has binpkg-docompress ${FEATURES}; then
75 > > > > + "${PORTAGE_BIN_PATH}"/ecompress --queue "${PORTAGE_DOCOMPRESS[@]}"
76 > > > > + "${PORTAGE_BIN_PATH}"/ecompress --ignore "${PORTAGE_DOCOMPRESS_SKIP[@]}"
77 > > > > + "${PORTAGE_BIN_PATH}"/ecompress --dequeue
78 > > > > + fi
79 > > > > +
80 > > > > + if has chflags ${FEATURES}; then
81 > > > > + # Restore all the file flags that were saved earlier on.
82 > > > > + mtree -U -e -p "${ED}" -k flags < "${T}/bsdflags.mtree" &> /dev/null
83 > > > > + fi
84 > > > > +}
85 > > > > +
86 > > > > preinst_qa_check() {
87 > > > > postinst_qa_check preinst
88 > > > > }
89 > > > > diff --git a/cnf/make.globals b/cnf/make.globals
90 > > > > index 04a708af8..72b567e98 100644
91 > > > > --- a/cnf/make.globals
92 > > > > +++ b/cnf/make.globals
93 > > > > @@ -50,7 +50,7 @@ RESUMECOMMAND_SSH=${FETCHCOMMAND_SSH}
94 > > > > FETCHCOMMAND_SFTP="bash -c \"x=\\\${2#sftp://} ; host=\\\${x%%/*} ; port=\\\${host##*:} ; host=\\\${host%:*} ; [[ \\\${host} = \\\${port} ]] && port= ; eval \\\"declare -a ssh_opts=(\\\${3})\\\" ; exec sftp \\\${port:+-P \\\${port}} \\\"\\\${ssh_opts[@]}\\\" \\\"\\\${host}:/\\\${x#*/}\\\" \\\"\\\$1\\\"\" sftp \"\${DISTDIR}/\${FILE}\" \"\${URI}\" \"\${PORTAGE_SSH_OPTS}\""
95 > > > >
96 > > > > # Default user options
97 > > > > -FEATURES="assume-digests binpkg-logs
98 > > > > +FEATURES="assume-digests binpkg-docompress binpkg-logs
99 > > > > config-protect-if-modified distlocks ebuild-locks
100 > > > > fixlafiles merge-sync multilib-strict news
101 > > > > parallel-fetch preserve-libs protect-owned
102 > > > > diff --git a/lib/portage/const.py b/lib/portage/const.py
103 > > > > index 7f84bf0e9..a343fc040 100644
104 > > > > --- a/lib/portage/const.py
105 > > > > +++ b/lib/portage/const.py
106 > > > > @@ -122,6 +122,7 @@ EBUILD_PHASES = (
107 > > > > )
108 > > > > SUPPORTED_FEATURES = frozenset([
109 > > > > "assume-digests",
110 > > > > + "binpkg-docompress",
111 > > > > "binpkg-logs",
112 > > > > "binpkg-multi-instance",
113 > > > > "buildpkg",
114 > > > > diff --git a/lib/portage/dbapi/vartree.py b/lib/portage/dbapi/vartree.py
115 > > > > index c16fdfe88..fd8aaeb8e 100644
116 > > > > --- a/lib/portage/dbapi/vartree.py
117 > > > > +++ b/lib/portage/dbapi/vartree.py
118 > > > > @@ -3719,6 +3719,7 @@ class dblink(object):
119 > > > >
120 > > > > This function does the following:
121 > > > >
122 > > > > + calls doebuild(mydo=instprep)
123 > > > > calls get_ro_checker to retrieve a function for checking whether Portage
124 > > > > will write to a read-only filesystem, then runs it against the directory list
125 > > > > calls self._preserve_libs if FEATURES=preserve-libs
126 > > > > @@ -3768,6 +3769,17 @@ class dblink(object):
127 > > > > level=logging.ERROR, noiselevel=-1)
128 > > > > return 1
129 > > > >
130 > > > > + # run instprep internal phase
131 > > > > + doebuild_environment(myebuild, "instprep",
132 > > > > + settings=self.settings, db=mydbapi)
133 > > > > + phase = EbuildPhase(background=False, phase="instprep",
134 > > > > + scheduler=self._scheduler, settings=self.settings)
135 > > > > + phase.start()
136 > > > > + if phase.wait() != os.EX_OK:
137 > > > > + showMessage(_("!!! instprep failed\n"),
138 > > > > + level=logging.ERROR, noiselevel=-1)
139 > > > > + return 1
140 > > > > +
141 > > > > is_binpkg = self.settings.get("EMERGE_FROM") == "binary"
142 > > > > slot = ''
143 > > > > for var_name in ('CHOST', 'SLOT'):
144 > > > > diff --git a/lib/portage/package/ebuild/doebuild.py b/lib/portage/package/ebuild/doebuild.py
145 > > > > index 9706de422..d7b535698 100644
146 > > > > --- a/lib/portage/package/ebuild/doebuild.py
147 > > > > +++ b/lib/portage/package/ebuild/doebuild.py
148 > > > > @@ -674,7 +674,7 @@ def doebuild(myebuild, mydo, _unused=DeprecationWarning, settings=None, debug=0,
149 > > > > "config", "info", "setup", "depend", "pretend",
150 > > > > "fetch", "fetchall", "digest",
151 > > > > "unpack", "prepare", "configure", "compile", "test",
152 > > > > - "install", "rpm", "qmerge", "merge",
153 > > > > + "install", "instprep", "rpm", "qmerge", "merge",
154 > > > > "package", "unmerge", "manifest", "nofetch"]
155 > > > >
156 > > > > if mydo not in validcommands:
157 > > > > @@ -1402,6 +1402,7 @@ def _spawn_actionmap(settings):
158 > > > > "compile": {"cmd":ebuild_sh, "args":{"droppriv":droppriv, "free":nosandbox, "sesandbox":sesandbox, "fakeroot":0}},
159 > > > > "test": {"cmd":ebuild_sh, "args":{"droppriv":droppriv, "free":nosandbox, "sesandbox":sesandbox, "fakeroot":0}},
160 > > > > "install": {"cmd":ebuild_sh, "args":{"droppriv":0, "free":0, "sesandbox":sesandbox, "fakeroot":fakeroot}},
161 > > > > +"instprep": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox":sesandbox, "fakeroot":fakeroot}},
162 > > > > "rpm": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox":0, "fakeroot":fakeroot}},
163 > > > > "package": {"cmd":misc_sh, "args":{"droppriv":0, "free":0, "sesandbox":0, "fakeroot":fakeroot}},
164 > > > > }
165 > > >
166 > > > The feature seems find but the instprep phase is not needed if we use
167 > > > MiscFunctionsProcess instead of EbuildPhase. Here's a list of odd things
168 > > > about the instprep ebuild phase implementation as it is:
169 > > >
170 > > > 1) You can call instprep explicitly via the ebuild(1) command, but I
171 > > > doubt that we really want or need to allow that.
172 > >
173 > > I was actually wondering about that. I suppose it might be useful for
174 > > debugging purposes. However, since I wasn't able to figure out how to
175 > > fully integrate it with the phase machinery, I went for the minimal set
176 > > of code that wouldn't be definitive either way.
177 > >
178 > > > 2) Unlinke other ebuild phases, the doebuild function doesn't created a
179 > > > .instpreped file to indicate when the instprep phase has executed.
180 > >
181 > > I suppose I can try to do that if that's desirable.
182 > >
183 > > > 3) Currently instprep executes when FEATURES=binpkg-docompress is
184 > > > enabled, even though it does nothing of value. I think we should instead
185 > > > generate a relevant list of MiscFunctionsProcess commands for the
186 > > > enabled FEATURES, and only start a MiscFunctionsProcess instance if the
187 > > > list of commands is non-empty.
188 > >
189 > > That sounds like premature optimization with a bit of going against
190 > > principle of least surprise. Given it's a phase function with specific
191 > > implementation that could be extended in the future, I'd rather avoid
192 > > hiding additional conditions for running it elsewhere, as it opens
193 > > a strong possibility that somebody adds additional functionality but
194 > > forgets to update the condition resulting either in immediate WTF
195 > > or the new code being skipped only for some users, with even harder-to-
196 > > debug WTF.
197 >
198 > I wasn't really sure that instprep deserved to be a full-fledged phase
199 > at this point. I guess you're planning to add more stuff there?
200
201 At least stripping. But as usually happens, others may also have more
202 ideas. I suppose the main use case would be stuff that needs to happen
203 after install but isn't strictly necessary for building binary packages.
204
205 --
206 Best regards,
207 Michał Górny

Attachments

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

Replies