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

[Michał Górny]

On Fri, 2018-11-02 at 11:49 -0700, Zac Medico wrote:
> On 11/01/2018 02:50 AM, Michał Górny wrote:
> > Add FEATURES=binpkg-docompress that can be used whether docompress
> > compression is performed before or after creating binary packages.  With
> > the feature enabled (the default), the current behavior of storing
> > compressed files in binpkg is preserved.  With it disabled, uncompressed
> > files are stored inside binary package and are compressed when
> > installing.
> >
> > Storing uncompressed files in binary packages has two advantages:
> >
> > 1. Avoids the double-compression penalty, effectively improving binary
> >    package compression speed and compression ratio.
> >
> > 2. Allows the same packages to be reused on systems with different
> >    docompress configurations.
> >
> > The option is roughly backwards compatible.  Old Portage versions will
> > install packages created with FEATURES=-binpkg-docompress correctly,
> > albeit without compression.  Portage with FEATURES=binpkg-docompress
> > should install old binpackages semi-correctly, potentially recompressing
> > them (and throwing already-compressed warnings on format mismatch).
> > The new behavior is left off by default to avoid those problems.
> >
> > Signed-off-by: Michał Górny <mgorny@g.o>
> > ---
> >  bin/misc-functions.sh                  | 34 +++++++++++++++++++++++---
> >  cnf/make.globals                       |  2 +-
> >  lib/portage/const.py                   |  1 +
> >  lib/portage/dbapi/vartree.py           | 12 +++++++++
> >  lib/portage/package/ebuild/doebuild.py |  3 ++-
> >  man/make.conf.5                        |  6 +++++
> >  6 files changed, 52 insertions(+), 6 deletions(-)
> >
> > diff --git a/bin/misc-functions.sh b/bin/misc-functions.sh
> > index db7aaed5a..96aa66611 100755
> > --- a/bin/misc-functions.sh
> > +++ b/bin/misc-functions.sh
> > @@ -109,10 +109,13 @@ install_qa_check() {
> >
> >  	[[ -d ${ED%/}/usr/share/info ]] && prepinfo
> >
> > -	# Apply compression.
> > -	"${PORTAGE_BIN_PATH}"/ecompress --queue "${PORTAGE_DOCOMPRESS[@]}"
> > -	"${PORTAGE_BIN_PATH}"/ecompress --ignore "${PORTAGE_DOCOMPRESS_SKIP[@]}"
> > -	"${PORTAGE_BIN_PATH}"/ecompress --dequeue
> > +	# If binpkg-docompress is enabled, apply compression before creating
> > +	# the binary package.
> > +	if has binpkg-docompress ${FEATURES}; then
> > +		"${PORTAGE_BIN_PATH}"/ecompress --queue "${PORTAGE_DOCOMPRESS[@]}"
> > +		"${PORTAGE_BIN_PATH}"/ecompress --ignore "${PORTAGE_DOCOMPRESS_SKIP[@]}"
> > +		"${PORTAGE_BIN_PATH}"/ecompress --dequeue
> > +	fi
> >
> >  	export STRIP_MASK
> >  	if ___eapi_has_dostrip; then
> > @@ -160,6 +163,29 @@ install_qa_check() {
> >  	rm -f "${ED%/}"/usr/share/info/dir{,.gz,.bz2} || die "rm failed!"
> >  }
> >
> > +__dyn_instprep() {
> > +	if has chflags ${FEATURES}; then
> > +		# Save all the file flags for restoration afterwards.
> > +		mtree -c -p "${ED}" -k flags > "${T}/bsdflags.mtree"
> > +		# Remove all the file flags so that we can do anything necessary.
> > +		chflags -R noschg,nouchg,nosappnd,nouappnd "${ED}"
> > +		chflags -R nosunlnk,nouunlnk "${ED}" 2>/dev/null
> > +	fi
> > +
> > +	# If binpkg-docompress is disabled, we need to apply compression
> > +	# before installing.
> > +	if ! has binpkg-docompress ${FEATURES}; then
> > +		"${PORTAGE_BIN_PATH}"/ecompress --queue "${PORTAGE_DOCOMPRESS[@]}"
> > +		"${PORTAGE_BIN_PATH}"/ecompress --ignore "${PORTAGE_DOCOMPRESS_SKIP[@]}"
> > +		"${PORTAGE_BIN_PATH}"/ecompress --dequeue
> > +	fi
> > +
> > +	if has chflags ${FEATURES}; then
> > +		# Restore all the file flags that were saved earlier on.
> > +		mtree -U -e -p "${ED}" -k flags < "${T}/bsdflags.mtree" &> /dev/null
> > +	fi
> > +}
> > +
> >  preinst_qa_check() {
> >  	postinst_qa_check preinst
> >  }
> > diff --git a/cnf/make.globals b/cnf/make.globals
> > index 04a708af8..72b567e98 100644
> > --- a/cnf/make.globals
> > +++ b/cnf/make.globals
> > @@ -50,7 +50,7 @@ RESUMECOMMAND_SSH=${FETCHCOMMAND_SSH}
> >  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}\""
> >
> >  # Default user options
> > -FEATURES="assume-digests binpkg-logs
> > +FEATURES="assume-digests binpkg-docompress binpkg-logs
> >            config-protect-if-modified distlocks ebuild-locks
> >            fixlafiles merge-sync multilib-strict news
> >            parallel-fetch preserve-libs protect-owned
> > diff --git a/lib/portage/const.py b/lib/portage/const.py
> > index 7f84bf0e9..a343fc040 100644
> > --- a/lib/portage/const.py
> > +++ b/lib/portage/const.py
> > @@ -122,6 +122,7 @@ EBUILD_PHASES            = (
> >  )
> >  SUPPORTED_FEATURES       = frozenset([
> >  	"assume-digests",
> > +	"binpkg-docompress",
> >  	"binpkg-logs",
> >  	"binpkg-multi-instance",
> >  	"buildpkg",
> > diff --git a/lib/portage/dbapi/vartree.py b/lib/portage/dbapi/vartree.py
> > index c16fdfe88..fd8aaeb8e 100644
> > --- a/lib/portage/dbapi/vartree.py
> > +++ b/lib/portage/dbapi/vartree.py
> > @@ -3719,6 +3719,7 @@ class dblink(object):
> >
> >  		This function does the following:
> >
> > +		calls doebuild(mydo=instprep)
> >  		calls get_ro_checker to retrieve a function for checking whether Portage
> >  		will write to a read-only filesystem, then runs it against the directory list
> >  		calls self._preserve_libs if FEATURES=preserve-libs
> > @@ -3768,6 +3769,17 @@ class dblink(object):
> >  				level=logging.ERROR, noiselevel=-1)
> >  			return 1
> >
> > +		# run instprep internal phase
> > +		doebuild_environment(myebuild, "instprep",
> > +			settings=self.settings, db=mydbapi)
> > +		phase = EbuildPhase(background=False, phase="instprep",
> > +			scheduler=self._scheduler, settings=self.settings)
> > +		phase.start()
> > +		if phase.wait() != os.EX_OK:
> > +			showMessage(_("!!! instprep failed\n"),
> > +				level=logging.ERROR, noiselevel=-1)
> > +			return 1
> > +
> >  		is_binpkg = self.settings.get("EMERGE_FROM") == "binary"
> >  		slot = ''
> >  		for var_name in ('CHOST', 'SLOT'):
> > 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.  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.

> 3) Currently instprep executes when FEATURES=binpkg-docompress is
> enabled, even though it does nothing of value. I think we should instead
> generate a relevant list of MiscFunctionsProcess commands for the
> enabled FEATURES, and only start a MiscFunctionsProcess instance if the
> list of commands is non-empty.

That sounds like premature optimization with a bit of going against
principle of least surprise.  Given it's a phase function with specific
implementation that could be extended in the future, I'd rather avoid
hiding additional conditions for running it elsewhere, as it opens
a strong possibility that somebody adds additional functionality but
forgets to update the condition resulting either in immediate WTF
or the new code being skipped only for some users, with even harder-to-
debug WTF.

-- 
Best regards,
Michał Górny