Re: [OE-core] [PATCHv2 3/4] bitbake.conf: Canonicalize paths in PSEUDO_IGNORE_PATHS

["Richard Purdie" <richard.purdie@linuxfoundation.org>]

On Mon, 2020-11-30 at 17:44 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-
> > core@lists.openembedded.org> On Behalf Of Richard Purdie
> > Sent: den 27 november 2020 00:42
> > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> > core@lists.openembedded.org
> > Subject: Re: [OE-core] [PATCHv2 3/4] bitbake.conf: Canonicalize paths in
> > PSEUDO_IGNORE_PATHS
> > 
> > On Thu, 2020-11-26 at 20:38 +0000, Peter Kjellerstedt wrote:
> > > > -----Original Message-----
> > > > From: Richard Purdie <richard.purdie@linuxfoundation.org>
> > > > Sent: den 26 november 2020 15:47
> > > > To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> > > > core@lists.openembedded.org
> > > > Subject: Re: [OE-core] [PATCHv2 3/4] bitbake.conf: Canonicalize
> > > > paths in
> > > > PSEUDO_IGNORE_PATHS
> > > > 
> > > > On Wed, 2020-11-25 at 14:48 +0100, Peter Kjellerstedt wrote:
> > > > > Introduce PSEUDO_IGNORE_REAL_PATHS and make it contain the
> > > > canonicalized
> > > > > paths from PSEUDO_IGNORE_PATHS, obtained by passing the latter to
> > > > > oe.path.to_real_paths(). This is needed since pseudo's
> > > > > pseudo_client_ignore_path_chroot() will compare the ignored paths
> > > > > to
> > > > > paths that have been canonicalized.
> > > > > 
> > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > > ---
> > > > >  meta/conf/bitbake.conf | 8 +++++---
> > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > This looks like a good way to fix this, except I have a strong
> > > > dislike
> > > > of "REAL" variables.
> > > 
> > > Well, I named it PSEUDO_IGNORE_REAL_PATHS to match
> > > os.path.realpath().
> > > I can name it, e.g., PSEUDO_IGNORE_CANONICAL_PATHS if you prefer
> > > that?
> > 
> > My point was less about the name and more that I'd prefer not to have
> > an indirect variable which isn't useful.
> > 
> > > > > diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> > > > > index 9742fe4fe2..4862095a1b 100644
> > > > > --- a/meta/conf/bitbake.conf
> > > > > +++ b/meta/conf/bitbake.conf
> > > > > @@ -685,15 +685,16 @@ SRC_URI = ""
> > > > >  PSEUDO_LOCALSTATEDIR ?= "${WORKDIR}/pseudo/"
> > > > >  PSEUDO_PASSWD ?= "${STAGING_DIR_TARGET}:${PSEUDO_SYSROOT}"
> > > > >  PSEUDO_SYSROOT = "${COMPONENTS_DIR}/${BUILD_ARCH}/pseudo-native"
> > > > > +PSEUDO_IGNORE_REAL_PATHS = "${@','.join(os.path.realpath(path) for
> > > > path in (d.getVar('PSEUDO_IGNORE_PATHS') or '').split(','))}"
> > > > >  PSEUDO_IGNORE_PATHS =
> > "/usr/,/etc/,/lib,/dev/,${T},${WORKDIR}/recipe-
> > > > sysroot,${SSTATE_DIR},${STAMPS_DIR},${WORKDIR}/pkgdata-
> > > > sysroot,${TMPDIR}/sstate-control,${DEPLOY_DIR},${WORKDIR}/deploy-
> > > > ,${TMPDIR}/buildstats,${WORKDIR}/sstate-build-
> > package_,${WORKDIR}/sstate-
> > > > install-package_,${WORKDIR}/sstate-build-
> > > > image_complete,${TMPDIR}/sysroots-
> > > > 
> > components,${BUILDHISTORY_DIR},${TMPDIR}/pkgdata,${TOPDIR}/cache,${COREBA
> > > > SE}/scripts,${@','.join(d.getVar('BBLAYERS').split())},${CCACHE_DIR}"
> > > > >  export PSEUDO_DISABLED = "1"
> > > > >  #export PSEUDO_PREFIX = "${STAGING_DIR_NATIVE}${prefix_native}"
> > > > >  #export PSEUDO_BINDIR = "${STAGING_DIR_NATIVE}${bindir_native}"
> > > > >  #export PSEUDO_LIBDIR =
> > > > "${STAGING_DIR_NATIVE}$PSEUDOBINDIR/../lib/pseudo/lib
> > > > > -FAKEROOTBASEENV = "PSEUDO_BINDIR=${PSEUDO_SYSROOT}${bindir_native}
> > > > PSEUDO_LIBDIR=${PSEUDO_SYSROOT}${prefix_native}/lib/pseudo/lib
> > > > PSEUDO_PREFIX=${PSEUDO_SYSROOT}${prefix_native}
> > > > PSEUDO_IGNORE_PATHS=${PSEUDO_IGNORE_PATHS} PSEUDO_DISABLED=1"
> > > > > +FAKEROOTBASEENV = "PSEUDO_BINDIR=${PSEUDO_SYSROOT}${bindir_native}
> > > > PSEUDO_LIBDIR=${PSEUDO_SYSROOT}${prefix_native}/lib/pseudo/lib
> > > > PSEUDO_PREFIX=${PSEUDO_SYSROOT}${prefix_native}
> > > > PSEUDO_IGNORE_PATHS=${PSEUDO_IGNORE_REAL_PATHS} PSEUDO_DISABLED=1"
> > > > >  FAKEROOTCMD = "${PSEUDO_SYSROOT}${bindir_native}/pseudo"
> > > > > -FAKEROOTENV = "PSEUDO_PREFIX=${PSEUDO_SYSROOT}${prefix_native}
> > > > PSEUDO_LOCALSTATEDIR=${PSEUDO_LOCALSTATEDIR}
> > > > PSEUDO_PASSWD=${PSEUDO_PASSWD} PSEUDO_NOSYMLINKEXP=1
> > > > PSEUDO_IGNORE_PATHS=${PSEUDO_IGNORE_PATHS} PSEUDO_DISABLED=0"
> > > > > +FAKEROOTENV = "PSEUDO_PREFIX=${PSEUDO_SYSROOT}${prefix_native}
> > > > PSEUDO_LOCALSTATEDIR=${PSEUDO_LOCALSTATEDIR}
> > > > PSEUDO_PASSWD=${PSEUDO_PASSWD} PSEUDO_NOSYMLINKEXP=1
> > > > PSEUDO_IGNORE_PATHS=${PSEUDO_IGNORE_REAL_PATHS} PSEUDO_DISABLED=0"
> > > > 
> > > > I'm wondering if we create a function in lib/oe/utils.py and then use
> > > > something like:
> > > > 
> > > > PSEUDO_IGNORE_PATHS=${@oe.utils.realpath("PSEUDO_IGNORE_REAL_PATHS")}
> > > 
> > > There already is an oe.path.realpath(). However, I am not sure when
> > > it is expected to be used instead of os.path.realpath().
> > 
> > That is a bad choice of name now I look at it! I was thinking more
> > about the use of a function.
> > 
> > > I initially created an oe.path.realpaths(d, var, separator=','), but
> > > after I had rewritten it a couple of times I was down to just the
> > > single list comprehension and then I felt as I could just as well
> > > use it directly since I don't see a great need for a common function
> > > to canonicalize a list of paths. But if you prefer that solution, I
> > > can restore it.
> > 
> > I don't really mind the list comprehension directly or a function, I'd
> > just prefer not to have an indirect variable around to confuse people.
> 
> Ok, I take that to mean that you instead want me to, e.g., use a function 
> to canonicalize the paths where ${PSEUDO_IGNORE_REAL_PATHS} is used now? 
> I.e., something like this (line broken here for readability):
> 
>   FAKEROOTENV = "PSEUDO_PREFIX=${PSEUDO_SYSROOT}${prefix_native} \
>                  PSEUDO_LOCALSTATEDIR=${PSEUDO_LOCALSTATEDIR} \
>                  PSEUDO_PASSWD=${PSEUDO_PASSWD} \
>                  PSEUDO_NOSYMLINKEXP=1 \
>                  PSEUDO_IGNORE_PATHS=${@oe.paths.canonicalize(d, 'PSEUDO_IGNORE_PATHS')} \
>                  PSEUDO_DISABLED=0"
>   FAKEROOTENV[vardeps] += "PSEUDO_IGNORE_PATHS"
> 
> alternatively:
> 
>   FAKEROOTENV = "PSEUDO_PREFIX=${PSEUDO_SYSROOT}${prefix_native} \
>                  PSEUDO_LOCALSTATEDIR=${PSEUDO_LOCALSTATEDIR} \
>                  PSEUDO_PASSWD=${PSEUDO_PASSWD} \
>                  PSEUDO_NOSYMLINKEXP=1 \
>                  PSEUDO_IGNORE_PATHS=${@oe.paths.canonicalize(d.getVar('PSEUDO_IGNORE_PATHS'))} \
>                  PSEUDO_DISABLED=0"
> 
> I am not sure whether there is a preference for utility functions 
> to take d and the name of a bitbake variable as arguments, or to 
> take the variable's value. The former is a little bit shorter, but 
> on the other hand it requires the extra vardeps declaration.
> 
> There is a minor drawback with this solution, and that is that I 
> cannot use that function in wic. So I will use the list comprehension 
> directly, with a comment referring to the oe.path.canonicalize() 
> function.

The version which avoids the vardeps is probably slightly neater
overall. I wasn't sure if using the list comprehension here would be
too ugly or not but I'm happy just not to have another variable name
used here, thanks!

The wic issue is unfortunate but hard to avoid.

Cheers,

Richard