Re: [PATCH v2 01/25] bitbake.conf: support for merged usr with DISTRO_FEATURE usrmerge

Peter Kjellerstedt
2017-06-07T17:04:06Z

> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Patrick Ohly
> Sent: den 7 juni 2017 16:32
> To: Richard Purdie <richard.purdie@linuxfoundation.org>
> Cc: Joshua Lock <joshua.g.lock@intel.com>; openembedded-
> core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH v2 01/25] bitbake.conf: support for
> merged usr with DISTRO_FEATURE usrmerge
>
> On Wed, 2017-06-07 at 12:52 +0100, Richard Purdie wrote:
> > On Wed, 2017-02-22 at 10:26 +0200, Amarnath Valluri wrote:
> > > From: Joshua Lock <joshua.g.lock@intel.com>
> > >
> > > Modify bindir, libdir and sbindir to be exec_prefix/$d, rather than
> > > base_prefix/$d, when the usrmerge DISTRO_FEATURE is enabled.
> > >
> > > Signed-off-by: Joshua Lock <joshua.g.lock@intel.com>
> > > ---
> > > meta/conf/bitbake.conf | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > I was asked to clarify the status of this series. We've taken many of
> > the changes but the core changes to bitbake.conf haven't been merged.
> >
> > Ross/I discussed it and we both feel that the bitbake.conf make the
> > file pretty unreadable. We'd therefore like to see this implemented
> > as a .inc file which gets included when the relevant DISTRO_FEATURE
> > is set (or could just be included by the appropriate distro configs).
> > This .inc file would then simply override the paths. I believe this
> > would be a lot more readable than the current approach which is a
> > key concern in the core config.

I actually suggested an alternative change to bitbake.conf in the GitHub
review that changes bitbake.conf to take usrmerge into account:

https://github.com/avalluri/openembedded-core/commit/1655a93238902a883052a55d88ae14dd02243530

I will include my suggested solution here since I will refer to it
below:

diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 0f27e92e96..305eaff583 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -17,11 +17,13 @@ export base_prefix = ""
export prefix = "/usr"
export exec_prefix = "${prefix}"

+root_prefix = "${@bb.utils.contains('DISTRO_FEATURES', 'usrmerge', '${exec_prefix}', '${base_prefix}', d)}"
+
# Base paths
-export base_bindir = "${base_prefix}/bin"
-export base_sbindir = "${base_prefix}/sbin"
-export base_libdir = "${base_prefix}/${baselib}"
-export nonarch_base_libdir = "${base_prefix}/lib"
+export base_bindir = "${root_prefix}/bin"
+export base_sbindir = "${root_prefix}/sbin"
+export base_libdir = "${root_prefix}/${baselib}"
+export nonarch_base_libdir = "${root_prefix}/lib"

# Architecture independent paths
export sysconfdir = "${base_prefix}/etc"

I think that better maintains the readability of the bitbake.conf file,
while making the $root_prefix variable available as it can be useful in
its own (e.g., in the systemd recipe).

> > The merge of the rest of this feature is therefore pending on the
> > development of such a patch, or a discussion convincing Ross/I why we
> > shouldn't take that approach for some reason.
>
> Such a .inc file would have to be included after the "Include the rest
> of the config files." section, because DISTRO_FEATURES only has its
> final value (?) at that point. One also needs the patch that I
> coincidentally just sent today to the bitbake list which allows
> including such a file only when "usrmerge" is in DISTRO_FEATURES.
>
> Would that be okay?
>
> One downside is that code using the variables earlier will just see the
> non-usrmerge version, with no indication in "bitbake -e" that the value
> might have been different in the middle of parsing. The if check in the
> proposed patch has the same effect on the actual values, but at least
> it shows up in "bitbake -e". This is not a particularly strong argument
> either way, I just wanted to mention it.

Based on your concerns about when the path variables affected by
usrmerge would have what values, maybe my suggested change above
should be changed to define root_prefix like this instead:

root_prefix ?= "${base_prefix}"

and then we create a usrmerge.inc file with the following contents:

DISTRO_FEATURES_append = " usrmerge"

root_prefix = "${exec_prefix}"

This file should then be required by a layer.conf file so that
root_prefix is set before bitbake.conf is read. That should make sure
that there never is any risk of the paths being defined differently
based on whether usrmerge has been added to DISTRO_FEATURES yet or not.
And it should be clear in bitbake -e how such a variable got its value.

The only minor drawback I see is that the usrmerge.inc file needs to
be required from a layer.conf file rather than just adding usrmerge
to DISTRO_FEATURES in ${DISTRO}.conf, but at least I could live with
that. One could of course add an appropriate comment before the
definition of the root_prefix variable, to clarify its use and maybe
even point at the usrmerge.inc file.

> --
> Best Regards, Patrick Ohly
>
> The content of this message is my personal opinion only and although
> I am an employee of Intel, the statements I make here in no way
> represent Intel's position on the issue, nor am I authorized to speak
> on behalf of Intel on this matter.

//Peter