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

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

[Peter Kjellerstedt]

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.

//Peter

The following changes since commit 7a0e795373653886452a7a2992ced10080711c26:

  build-appliance-image: Update to master head revision (2017-04-21 08:22:18 +0100)

are available in the git repository at:

  git://git.yoctoproject.org/poky-contrib pkj/hosttools_dir
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=pkj/hosttools_dir

Peter Kjellerstedt (2):
  bitbake.conf: Add HOSTTOOLS_DIR for ${TMPDIR}/hosttools
  sstate.bbclass, staging.bbclass: Handle HOSTTOOLS_DIR when restoring
    state

 meta/classes/base.bbclass                           | 4 ++--
 meta/classes/sstate.bbclass                         | 2 +-
 meta/classes/staging.bbclass                        | 2 +-
 meta/conf/bitbake.conf                              | 3 +++
 meta/conf/layer.conf                                | 2 +-
 meta/recipes-kernel/kmod/depmodwrapper-cross_1.0.bb | 2 +-
 6 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.12.0

[PATCH 1/2] bitbake.conf: Add HOSTTOOLS_DIR for ${TMPDIR}/hosttools

[Peter Kjellerstedt]

The path to where to install and find the tools copied from the host
environment is already used in a couple of places. This warrants it to
get its own variable.

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
 meta/classes/base.bbclass | 4 ++--
 meta/conf/bitbake.conf    | 3 +++
 meta/conf/layer.conf      | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass
index e29821f199..d95afb7b9b 100644
--- a/meta/classes/base.bbclass
+++ b/meta/classes/base.bbclass
@@ -239,8 +239,8 @@ python base_eventhandler() {
         oe.utils.features_backfill("DISTRO_FEATURES", e.data)
         oe.utils.features_backfill("MACHINE_FEATURES", e.data)
         # Works with the line in layer.conf which changes PATH to point here
-        setup_hosttools_dir(d.expand('${TMPDIR}/hosttools'), 'HOSTTOOLS', d)
-        setup_hosttools_dir(d.expand('${TMPDIR}/hosttools'), 'HOSTTOOLS_NONFATAL', d, fatal=False)
+        setup_hosttools_dir(d.getVar('HOSTTOOLS_DIR'), 'HOSTTOOLS', d)
+        setup_hosttools_dir(d.getVar('HOSTTOOLS_DIR'), 'HOSTTOOLS_NONFATAL', d, fatal=False)
 
     if isinstance(e, bb.event.BuildStarted):
         localdata = bb.data.createCopy(e.data)
diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 227babd1b2..ba8099270c 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -452,6 +452,9 @@ export PATH
 # Build utility info.
 ##################################################################
 
+# Directory where host tools are copied
+HOSTTOOLS_DIR = "${TMPDIR}/hosttools"
+
 # Tools needed to run builds with OE-Core
 HOSTTOOLS += " \
     [ ar as awk basename bash bzip2 cat chgrp chmod chown chrpath cmp cp cpio \
diff --git a/meta/conf/layer.conf b/meta/conf/layer.conf
index 739d82ea56..fc165021c5 100644
--- a/meta/conf/layer.conf
+++ b/meta/conf/layer.conf
@@ -60,4 +60,4 @@ SIGGEN_EXCLUDE_SAFE_RECIPE_DEPS += " \
 "
 
 # We need to keep bitbake tools in PATH
-PATH := "${@os.path.dirname(bb.utils.which(d.getVar('PATH'),'bitbake'))}:${TMPDIR}/hosttools"
+PATH := "${@os.path.dirname(bb.utils.which(d.getVar('PATH'),'bitbake'))}:${HOSTTOOLS_DIR}"
-- 
2.12.0

[PATCH 2/2] sstate.bbclass, staging.bbclass: Handle HOSTTOOLS_DIR when restoring state

[Peter Kjellerstedt]

Paths to host tools that have been copied to ${HOSTTOOLS_DIR} may end
up in the sstate cache. They thus need to be corrected when restoring
from the sstate cache.

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
 meta/classes/sstate.bbclass                         | 2 +-
 meta/classes/staging.bbclass                        | 2 +-
 meta/recipes-kernel/kmod/depmodwrapper-cross_1.0.bb | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
index e50a38597d..ddc442cdf9 100644
--- a/meta/classes/sstate.bbclass
+++ b/meta/classes/sstate.bbclass
@@ -54,7 +54,7 @@ SSTATEPOSTCREATEFUNCS = ""
 SSTATEPREINSTFUNCS = ""
 SSTATEPOSTUNPACKFUNCS = "sstate_hardcode_path_unpack"
 SSTATEPOSTINSTFUNCS = ""
-EXTRA_STAGING_FIXMES ?= ""
+EXTRA_STAGING_FIXMES ?= "HOSTTOOLS_DIR"
 SSTATECLEANFUNCS = ""
 
 # Check whether sstate exists for tasks that support sstate and are in the
diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
index 8bdb437a1f..4015dd754c 100644
--- a/meta/classes/staging.bbclass
+++ b/meta/classes/staging.bbclass
@@ -249,7 +249,7 @@ def staging_processfixme(fixme, target, recipesysroot, recipesysrootnative, d):
     if not fixme:
         return
     cmd = "sed -e 's:^[^/]*/:%s/:g' %s | xargs sed -i -e 's:FIXMESTAGINGDIRTARGET:%s:g; s:FIXMESTAGINGDIRHOST:%s:g'" % (target, " ".join(fixme), recipesysroot, recipesysrootnative)
-    for fixmevar in ['PKGDATA_DIR']:
+    for fixmevar in ['HOSTTOOLS_DIR', 'PKGDATA_DIR']:
         fixme_path = d.getVar(fixmevar)
         cmd += " -e 's:FIXME_%s:%s:g'" % (fixmevar, fixme_path)
     bb.note(cmd)
diff --git a/meta/recipes-kernel/kmod/depmodwrapper-cross_1.0.bb b/meta/recipes-kernel/kmod/depmodwrapper-cross_1.0.bb
index 17a99a4afb..44d013f29d 100644
--- a/meta/recipes-kernel/kmod/depmodwrapper-cross_1.0.bb
+++ b/meta/recipes-kernel/kmod/depmodwrapper-cross_1.0.bb
@@ -9,7 +9,7 @@ PACKAGE_ARCH = "${MACHINE_ARCH}"
 
 # We need the following for the sstate code to process the wrapper
 SSTATE_SCAN_FILES += "depmodwrapper"
-EXTRA_STAGING_FIXMES = "PKGDATA_DIR"
+EXTRA_STAGING_FIXMES += "PKGDATA_DIR"
 
 do_populate_sysroot[depends] = ""
 
-- 
2.12.0

Re: [PATCH 2/2] sstate.bbclass, staging.bbclass: Handle HOSTTOOLS_DIR when restoring state

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

On 28 April 2017 at 16:01, Peter Kjellerstedt <peter.kjellerstedt@axis.com>
wrote:

> -EXTRA_STAGING_FIXMES ?= ""
> +EXTRA_STAGING_FIXMES ?= "HOSTTOOLS_DIR"
>

EXTRA and ?= suggests that this should remain "" and the HOSTTOOLS_DIR
relocation is handled in another way, as other classes that have done
="FOODIR" would override HOSTTOOLS_DIR.

Ross

[-- Attachment #2: Type: text/html, Size: 812 bytes --]

Re: [PATCH 2/2] sstate.bbclass, staging.bbclass: Handle HOSTTOOLS_DIR when restoring state

[Peter Kjellerstedt]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

So, should I change it to:

STAGING_FIXMES = "HOSTTOOLS_DIR ${EXTRA_STAGING_FIXMES}"
EXTRA_STAGING_FIXMES ?= ""

And use ${STAGING_FIXMES} instead of ${EXTRA_STAGING_FIXMES} later on?

//Peter

From: Burton, Ross [mailto:ross.burton@intel.com]
Sent: den 28 april 2017 17:06
To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
Cc: OE-core <openembedded-core@lists.openembedded.org>
Subject: Re: [OE-core] [PATCH 2/2] sstate.bbclass, staging.bbclass: Handle HOSTTOOLS_DIR when restoring state


On 28 April 2017 at 16:01, Peter Kjellerstedt <peter.kjellerstedt@axis.com<mailto:peter.kjellerstedt@axis.com>> wrote:
-EXTRA_STAGING_FIXMES ?= ""
+EXTRA_STAGING_FIXMES ?= "HOSTTOOLS_DIR"

EXTRA and ?= suggests that this should remain "" and the HOSTTOOLS_DIR relocation is handled in another way, as other classes that have done ="FOODIR" would override HOSTTOOLS_DIR.

Ross

[-- Attachment #2: Type: text/html, Size: 5462 bytes --]

Re: [PATCH 0/2] Handle the hossttools directory when restoring from the sstate cache

[Richard Purdie]

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.

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

Re: [PATCH 0/2] Handle the hossttools directory when restoring from the sstate cache

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Richard Purdie
> Sent: den 29 april 2017 10:53
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 0/2] Handle the hossttools directory when
> restoring from the sstate cache
> 
> 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.

> 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_HOSTTOOLS_DIR/install -c
tmp/sysroots-components/mips32r2el-nf/python3/usr/lib/python3.5/config/Makefile:MKDIR_P=        FIXME_HOSTTOOLS_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:FIXMESTAGINGDIRTARGET-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. ]

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.

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.

//Peter

Re: [PATCH 0/2] Handle the hossttools directory when restoring from the sstate cache

[Richard Purdie]

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

Re: [PATCH 0/2] Handle the hossttools directory when restoring from the sstate cache

[Peter Kjellerstedt]

> -----Original Message-----
> From: Richard Purdie [mailto:richard.purdie@linuxfoundation.org]
> Sent: den 1 maj 2017 10:15
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> core@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