[OE-core] [PATCH 0/2] Handle the hossttools directory when restoring from the sstate cache

Wed May 3 21:12:23 UTC 2017

> -----Original Message-----
> From: Richard Purdie [mailto:richard.purdie at linuxfoundation.org]
> Sent: den 1 maj 2017 10:15
> To: Peter Kjellerstedt <peter.kjellerstedt at axis.com>; openembedded-
> core at lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 0/2] Handle the hossttools directory when
> restoring from the sstate cache
> 
> On Sat, 2017-04-29 at 18:07 +0000, Peter Kjellerstedt wrote:
> > > Richard Purdie
> > > Sent: den 29 april 2017 10:53
> > > On Fri, 2017-04-28 at 17:01 +0200, Peter Kjellerstedt wrote:
> > > >
> > > > After the introduction of copying host tools to the build
> > > > directory
> > > > and cleaning out $PATH, we got a problem with one of our tools.
> > > > It
> > > > turned out that its configure.ac uses AC_PATH_PROG(PERL, perl) to
> > > > locate the perl interpreter, and uses that on the shebang line of
> > > > the
> > > > installed tool. Previously it found /usr/bin/perl and used that,
> > > > but
> > > > now it will find ${TMPDIR}/hosttools/perl and use that instead,
> > > > which
> > > > means that if the tool is restored from the sstate cache in
> > > > another
> > > > build directory than where it originated from, the path will be
> > > > wrong.
> > > >
> > > > These two patches adds a new variable (HOSTTOOLS_DIR) for the
> > > > ${TMPDIR}/hosttools directory, and then makes sure it is handled
> > > > by
> > > > the staging code so that any references to its value are properly
> > > > corrected when restoring from the sstate cache.
> > > I did mention on irc that I didn't really want to do this, I only
> > > wanted to fix this issue on a case by case basis.
> > >
> > > The reason is that we currently have pretty clean sstate files in
> > > this
> > > regard, we haven't needed to do this. If we add this, we'll become
> > > reliant on the fixups. The fixups are slow and ideally we want to
> > > pass
> > > in correct paths in the first place if/as/where we can.
> >
> > I am not sure how much my change will affect performance, but I think
> > it should be negligible. It only adds one more regular expression to
> > a sed command that would be run anyway. And all files (at least that
> > I can see in my tmp/sysroot-components) that need to be fixed for
> > HOSTTOOLS_DIR already need some other fixup so it does not add to the
> > number of files needing to be fixed.
> 
> Whether its negligible or not, we're going to end up enouraging people
> to be lazy and generate files which don't relocate without manual
> fixups.
> 
> That said, based on the number of files with problems, I don't think we
> have any other choice but to take the patches, particularly this close
> to release. I've therefore merged them as I don't have any better
> option.
> 
> > > So is there a way we can only do this if/as/where needed instead of
> > > universally?
> > >
> > > The performance overhead of fixups verses no fixups is significant.
> > >
> > > Cheers,
> > >
> > > Richard
> > This actually turned out much worse than I had first thought.
> >
> > When I examined what files had been modified in tmp/sysroot-
> > components after
> > applying my change, the following files were listed:
> >
> > tmp/sysroots-components/mips32r2el-nf/libtool-
> > cross/usr/bin/crossscripts/mipsel-poky-linux-
> > libtool:lt_truncate_bin="FIXME_HOSTTOOLS_DIR/dd bs=4096 count=1"
> > tmp/sysroots-components/mips32r2el-
> > nf/python/usr/lib/python2.7/config/Makefile:INSTALL=
> > FIXME_HOSTTOOLS_DIR/install -c
> > tmp/sysroots-components/mips32r2el-
> > nf/python/usr/lib/python2.7/config/Makefile:MKDIR_P=
> > FIXME_HOSTTOOLS_DIR/mkdir -p
> > tmp/sysroots-components/mips32r2el-
> > nf/python3/usr/lib/python3.5/config-
> > 3.5m/Makefile:INSTALL=   FIXME_HOSTTOOLS_DIR/install -c
> > tmp/sysroots-components/mips32r2el-
> > nf/python3/usr/lib/python3.5/config-
> > 3.5m/Makefile:MKDIR_P=   FIXME_HOSTTOOLS_DIR/mkdir -p
> > tmp/sysroots-components/mips32r2el-
> > nf/python3/usr/lib/python3.5/config/Makefile:INSTALL=        FIXME_HO
> > STTOOLS_DIR/install -c
> > tmp/sysroots-components/mips32r2el-
> > nf/python3/usr/lib/python3.5/config/Makefile:MKDIR_P=        FIXME_HO
> > STTOOLS_DIR/mkdir -p
> > tmp/sysroots-components/mips32r2el-
> > nf/apache2/usr/share/apache2/build/config_vars.mk:EGREP =
> > FIXME_HOSTTOOLS_DIR/grep -E
> > tmp/sysroots-components/mips32r2el-nf/apr/usr/share/build-
> > 1/libtool:lt_truncate_bin="FIXME_HOSTTOOLS_DIR/dd bs=4096 count=1"
> >
> > So I guess there are potential problems with them. However, in
> > addition to the
> > files listed above, all postinst-useradd-* scripts where also listed.
> > That is
> > because they set up PATH like this:
> >
> > export PATH="${BUILDDIR}/scripts:FIXMESTAGINGDIRTARGET-
> > native/usr/bin/mipsel-poky-
> > linux:FIXMESTAGINGDIRTARGET/usr/bin/crossscripts:FIXMESTAGINGDIRTARGE
> > T-native/usr/sbin:FIXMESTAGINGDIRTARGET-
> > native/usr/bin:FIXMESTAGINGDIRTARGET-
> > native/sbin:FIXMESTAGINGDIRTARGET-
> > native/bin:${BUILDDIR}/poky/bitbake/bin:FIXME_HOSTTOOLS_DIR"
> >
> > [ I modified the line to use ${BUILDDIR} rather than the expanded
> > value to
> >   make it shorter in the mail. ]
> 
> Its actually worse again as the stashed source directories for
> gcc/glibc also have a ton of this kind of issue too.
> 
> > And sure enough, when I tested a build without my fixes where a
> > recipe that creates a user is restored from sstate to satisfy the
> > dependencies of another recipe, and the hosttools path that is in the
> > postinst-useradd script no longer exists, the script fails to create
> > the user. But it does it silently, without causing a build failure!
> > And even if I add the -e option to sh for these scripts they still do
> > not fail, because the missing commands are called from within if
> > tests and thus do not trigger the shell to abort. This is very
> > bad, and if at all possible should be fixed for Pyro before release,
> > as it may lead to very odd failures for anyone using recipes that
> > create users/groups.
> 
> The piece that particularly worries me here is the silent nature of the
> failures. We really need to figure out why its silent and fix that.
> 
> > Another thing I noticed when looking at that PATH line, was the
> > existence of FIXMESTAGINGDIRTARGET-native. Those should have been
> > FIXMESTAGINGDIRHOST, but I think the logic in sstate.bbclass is
> > flawed as it only does that replacement if native.bbclass or any
> > cross*.bbclass has been inherited, but the native paths can very well
> > be used in a target recipe. It only happens to work because
> > RECIPE_SYSROOT and RECIPE_SYSROOT_NATIVE are defined as:
> >
> > RECIPE_SYSROOT = "${WORKDIR}/recipe-sysroot"
> > RECIPE_SYSROOT_NATIVE = "${WORKDIR}/recipe-sysroot-native"
> >
> > If RECIPE_SYSROOT_NATIVE instead had been defined as:
> >
> > RECIPE_SYSROOT_NATIVE = "${WORKDIR}/native-recipe-sysroot "
> >
> > I am sure we would have seen failures since the paths would no longer
> > have been fixed...
> >
> > I believe the sed expression used in sstate_hardcode_path() when
> > cross.bbclass or crosssdk.bbclass has been inherited should be used
> > for all the cases, but it needs to be corrected so that
> > FIXMESTAGINGDIRHOST is searched for before FIXMESTAGINGDIRTARGET, or
> > it will never match. Fixing this is not critical due to how
> > RECIPE_SYSROOT and RECIPE_SYSROOT_NATIVE are defined, so fixing it
> > can wait till after Pyro.
> 
> If you think about this a bit more, target output should not be
> referencing native paths. The postinst case is kind of special and my
> first instinct to fix this would be to simply stop exporting PATH in
> those scripts. The parent environment they're run in should have the
> correct PATH.
> 
> This is my big worry about this patch set, it papers over problems
> which should really be fixed by setting variables correctly in the
> first place, rather than relying on the relocation mechanism. Even in
> your original example, I think we should likely be passing in the
> correct paths, not relying on relocation magic.
> 
> Unfortunately we are close to release and there are paths which aren't
> easily fixed. You can't for example set ac_cv_path_install=install
> since autoconf then decides this must be a relative path and prefixes
> it with "../" and similar. Sure, we can then tweak autoconf but not
> quite everything uses reautoconf (notably gcc) and then things get
> messy :(.
> 
> I would also add that we need some better sanity tests on the sstate
> output to make sure bad paths aren't leaking through. We likely need
> some bugs filed on the various issues.
> 
> I will respin the release to rc4 with these patches applied though as
> there are too many potential risks here to release without doing that.
> I'm still running local tests.
> 
> Cheers,
> 
> Richard

Unfortunately I found another case where the postinst-useradd scripts 
bites us in the same way. This time it is with the sysroots-components 
directory. In the useradd_sysroot() function, PSEUDO is explicitly setup, 
I guess to cover all cases when this function can be called. It is 
defined as:

	export PSEUDO="${FAKEROOTENV} PSEUDO_LOCALSTATEDIR=${STAGING_DIR_TARGET}${localstatedir}/pseudo ${PSEUDO_SYSROOT}${bindir_native}/pseudo"

which expands to:

	export PSEUDO="PSEUDO_PREFIX=<tmpdir>/sysroots-components/x86_64/pseudo-native/usr PSEUDO_LOCALSTATEDIR=<tmpdir>/work/mips32r2el-nf-poky-linux/apache2/2.4.25-r0/pseudo/ PSEUDO_PASSWD=FIXMESTAGINGDIRTARGET:<tmpdir>/sysroots-components/x86_64/pseudo-native PSEUDO_NOSYMLINKEXP=1 PSEUDO_DISABLED=0 PSEUDO_LOCALSTATEDIR=FIXMESTAGINGDIRTARGET/var/pseudo <tmpdir>/sysroots-components/x86_64/pseudo-native/usr/bin/pseudo"

(The first PSEUDO_LOCALSTATEDIR= can be ignored as the second one replaces 
it.) These references to <tmpdir>/sysroots-components leads to problems 
when the script is installed into another build directory where the original 
sysroots-components directory does not exist.

I have two patches that fixes this, which I will send right away.

//Peter