Patchwork [2/2] bb-matrix: Make sure local.conf does not interfere

login
register
mail settings
Submitter Peter Kjellerstedt
Date Sept. 6, 2013, 4:12 p.m.
Message ID <398be88b2b4779bc227d9f716ebcc360958e02dd.1378483732.git.pkj@axis.com>
Download mbox | patch
Permalink /patch/57575/
State New
Headers show

Comments

Peter Kjellerstedt - Sept. 6, 2013, 4:12 p.m.
If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR happened to be
set in local.conf then the bb-matrix script would not perform as
intended.

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
 scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
 1 file changed, 4 insertions(+)
Darren Hart - Sept. 10, 2013, 3:33 p.m.
On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR happened to be
> set in local.conf then the bb-matrix script would not perform as
> intended.
> 
> Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> ---
>  scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> index 1064565..d5127e7 100755
> --- a/scripts/contrib/bb-perf/bb-matrix.sh
> +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> @@ -53,6 +53,10 @@ if [ $? -ne 0 ]; then
>  	exit 1
>  fi
>  
> +# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and SSTATE_DIR are set
> +# in local.conf
> +sed -ri 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]]*\??=.*)/#\1/' conf/local.conf
> +

Unless I'm mistaken, you are modifying the users local.conf?

We definitely should *not* be doing that. I would support documenting
this as a requirement and even printing a warning if any of a set of
variables are found in the bitbake environment.

Note that local.conf is not the only place where these could be set.

Richard, shouldn't the env setting override anything in local.conf?

Thanks,
Richard Purdie - Sept. 10, 2013, 3:37 p.m.
On Tue, 2013-09-10 at 08:33 -0700, Darren Hart wrote:
> On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR happened to be
> > set in local.conf then the bb-matrix script would not perform as
> > intended.
> > 
> > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > ---
> >  scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> > index 1064565..d5127e7 100755
> > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > @@ -53,6 +53,10 @@ if [ $? -ne 0 ]; then
> >  	exit 1
> >  fi
> >  
> > +# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and SSTATE_DIR are set
> > +# in local.conf
> > +sed -ri 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]]*\??=.*)/#\1/' conf/local.conf
> > +
> 
> Unless I'm mistaken, you are modifying the users local.conf?
> 
> We definitely should *not* be doing that. I would support documenting
> this as a requirement and even printing a warning if any of a set of
> variables are found in the bitbake environment.
> 
> Note that local.conf is not the only place where these could be set.
> 
> Richard, shouldn't the env setting override anything in local.conf?

No, it will override a ?= or ??= but not a =.

Cheers,

Richard
Darren Hart - Sept. 10, 2013, 3:42 p.m.
On Tue, 2013-09-10 at 16:37 +0100, Richard Purdie wrote:
> On Tue, 2013-09-10 at 08:33 -0700, Darren Hart wrote:
> > On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > > If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR happened to be
> > > set in local.conf then the bb-matrix script would not perform as
> > > intended.
> > > 
> > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > ---
> > >  scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> > > index 1064565..d5127e7 100755
> > > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > > @@ -53,6 +53,10 @@ if [ $? -ne 0 ]; then
> > >  	exit 1
> > >  fi
> > >  
> > > +# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and SSTATE_DIR are set
> > > +# in local.conf
> > > +sed -ri 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]]*\??=.*)/#\1/' conf/local.conf
> > > +
> > 
> > Unless I'm mistaken, you are modifying the users local.conf?
> > 
> > We definitely should *not* be doing that. I would support documenting
> > this as a requirement and even printing a warning if any of a set of
> > variables are found in the bitbake environment.
> > 
> > Note that local.conf is not the only place where these could be set.
> > 
> > Richard, shouldn't the env setting override anything in local.conf?
> 
> No, it will override a ?= or ??= but not a =.

OK, well, a warning if found in local.conf (or site.conf?) is probably
the best we can do for now. We can't check bitbake -e since that will
report the default which we wouldn't be able to distinguish from an
explicit setting.... unless we checked for an explicit assignment? Might
be doable.... but a warning should be sufficient for a script in
contrib, yes?
Richard Purdie - Sept. 10, 2013, 3:47 p.m.
On Tue, 2013-09-10 at 08:42 -0700, Darren Hart wrote:
> On Tue, 2013-09-10 at 16:37 +0100, Richard Purdie wrote:
> > On Tue, 2013-09-10 at 08:33 -0700, Darren Hart wrote:
> > > On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > > > If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR happened to be
> > > > set in local.conf then the bb-matrix script would not perform as
> > > > intended.
> > > > 
> > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > ---
> > > >  scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> > > > index 1064565..d5127e7 100755
> > > > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > > > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > > > @@ -53,6 +53,10 @@ if [ $? -ne 0 ]; then
> > > >  	exit 1
> > > >  fi
> > > >  
> > > > +# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and SSTATE_DIR are set
> > > > +# in local.conf
> > > > +sed -ri 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]]*\??=.*)/#\1/' conf/local.conf
> > > > +
> > > 
> > > Unless I'm mistaken, you are modifying the users local.conf?
> > > 
> > > We definitely should *not* be doing that. I would support documenting
> > > this as a requirement and even printing a warning if any of a set of
> > > variables are found in the bitbake environment.
> > > 
> > > Note that local.conf is not the only place where these could be set.
> > > 
> > > Richard, shouldn't the env setting override anything in local.conf?
> > 
> > No, it will override a ?= or ??= but not a =.
> 
> OK, well, a warning if found in local.conf (or site.conf?) is probably
> the best we can do for now. We can't check bitbake -e since that will
> report the default which we wouldn't be able to distinguish from an
> explicit setting.... unless we checked for an explicit assignment? Might
> be doable.... but a warning should be sufficient for a script in
> contrib, yes?

Yes, although I think in the comments, bitbake -e will tell you which
kind of operation was used :)

Cheers,

Richard
Peter Kjellerstedt - Sept. 11, 2013, 2:49 p.m.
> -----Original Message-----
> From: Richard Purdie [mailto:richard.purdie@linuxfoundation.org]
> Sent: den 10 september 2013 17:48
> To: Darren Hart
> Cc: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 2/2] bb-matrix: Make sure local.conf does
> not interfere
> 
> On Tue, 2013-09-10 at 08:42 -0700, Darren Hart wrote:
> > On Tue, 2013-09-10 at 16:37 +0100, Richard Purdie wrote:
> > > On Tue, 2013-09-10 at 08:33 -0700, Darren Hart wrote:
> > > > On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > > > > If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR
> happened to be
> > > > > set in local.conf then the bb-matrix script would not perform
> as
> > > > > intended.
> > > > >
> > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > > ---
> > > > >  scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh
> b/scripts/contrib/bb-perf/bb-matrix.sh
> > > > > index 1064565..d5127e7 100755
> > > > > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > > > > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > > > > @@ -53,6 +53,10 @@ if [ $? -ne 0 ]; then
> > > > >  	exit 1
> > > > >  fi
> > > > >
> > > > > +# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and
> SSTATE_DIR are set
> > > > > +# in local.conf
> > > > > +sed -ri
> 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]
> ]*\??=.*)/#\1/' conf/local.conf
> > > > > +
> > > >
> > > > Unless I'm mistaken, you are modifying the users local.conf?
> > > >
> > > > We definitely should *not* be doing that. I would support
> documenting
> > > > this as a requirement and even printing a warning if any of a set
> of
> > > > variables are found in the bitbake environment.
> > > >
> > > > Note that local.conf is not the only place where these could be
> set.
> > > >
> > > > Richard, shouldn't the env setting override anything in
> local.conf?
> > >
> > > No, it will override a ?= or ??= but not a =.
> >
> > OK, well, a warning if found in local.conf (or site.conf?) is
> probably
> > the best we can do for now. We can't check bitbake -e since that will
> > report the default which we wouldn't be able to distinguish from an
> > explicit setting.... unless we checked for an explicit assignment?
> Might
> > be doable.... but a warning should be sufficient for a script in
> > contrib, yes?
> 
> Yes, although I think in the comments, bitbake -e will tell you which
> kind of operation was used :)
> 
> Cheers,
> 
> Richard

My suggestion is to test that the values can be set by the script 
before starting the test run. Something like 
'BB_NUMBER_THREADS=1234 PARALLEL_MAKE="-j 1234" bitbake -e core-image-minimal'
and then grep for the expected values. If the correct values are 
not found then just abort with an error message as the script will 
not be able to do what it is supposed to do.

I would also suggest aborting if SSTATE_DIR does not match 
"$(pwd)/sstate-cache" since the script would clear a directory 
other than the one that is actually used.

Finally, if SSTATE_MIRRORS is non-empty I would generate a warning. 
I would not abort in this case as it can be a useful setup (e.g., 
I used this to build with a sstate cache mirror that contained the 
native packages as this halved the build time and I thus could test 
more combinations over a weekend).

I'll update my patch as per above unless you have other suggestions.

//Peter
Darren Hart - Sept. 11, 2013, 4:17 p.m.
On Wed, 2013-09-11 at 16:49 +0200, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: Richard Purdie [mailto:richard.purdie@linuxfoundation.org]
> > Sent: den 10 september 2013 17:48
> > To: Darren Hart
> > Cc: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
> > Subject: Re: [OE-core] [PATCH 2/2] bb-matrix: Make sure local.conf does
> > not interfere
> > 
> > On Tue, 2013-09-10 at 08:42 -0700, Darren Hart wrote:
> > > On Tue, 2013-09-10 at 16:37 +0100, Richard Purdie wrote:
> > > > On Tue, 2013-09-10 at 08:33 -0700, Darren Hart wrote:
> > > > > On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > > > > > If any of BB_NUMBER_THREADS, PARALLEL_MAKE or SSTATE_DIR
> > happened to be
> > > > > > set in local.conf then the bb-matrix script would not perform
> > as
> > > > > > intended.
> > > > > >
> > > > > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > > > > > ---
> > > > > >  scripts/contrib/bb-perf/bb-matrix.sh | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh
> > b/scripts/contrib/bb-perf/bb-matrix.sh
> > > > > > index 1064565..d5127e7 100755
> > > > > > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > > > > > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > > > > > @@ -53,6 +53,10 @@ if [ $? -ne 0 ]; then
> > > > > >  	exit 1
> > > > > >  fi
> > > > > >
> > > > > > +# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and
> > SSTATE_DIR are set
> > > > > > +# in local.conf
> > > > > > +sed -ri
> > 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]
> > ]*\??=.*)/#\1/' conf/local.conf
> > > > > > +
> > > > >
> > > > > Unless I'm mistaken, you are modifying the users local.conf?
> > > > >
> > > > > We definitely should *not* be doing that. I would support
> > documenting
> > > > > this as a requirement and even printing a warning if any of a set
> > of
> > > > > variables are found in the bitbake environment.
> > > > >
> > > > > Note that local.conf is not the only place where these could be
> > set.
> > > > >
> > > > > Richard, shouldn't the env setting override anything in
> > local.conf?
> > > >
> > > > No, it will override a ?= or ??= but not a =.
> > >
> > > OK, well, a warning if found in local.conf (or site.conf?) is
> > probably
> > > the best we can do for now. We can't check bitbake -e since that will
> > > report the default which we wouldn't be able to distinguish from an
> > > explicit setting.... unless we checked for an explicit assignment?
> > Might
> > > be doable.... but a warning should be sufficient for a script in
> > > contrib, yes?
> > 
> > Yes, although I think in the comments, bitbake -e will tell you which
> > kind of operation was used :)
> > 
> > Cheers,
> > 
> > Richard
> 
> My suggestion is to test that the values can be set by the script 
> before starting the test run. Something like 
> 'BB_NUMBER_THREADS=1234 PARALLEL_MAKE="-j 1234" bitbake -e core-image-minimal'
> and then grep for the expected values. If the correct values are 
> not found then just abort with an error message as the script will 
> not be able to do what it is supposed to do.
> 
> I would also suggest aborting if SSTATE_DIR does not match 
> "$(pwd)/sstate-cache" since the script would clear a directory 
> other than the one that is actually used.
> 
> Finally, if SSTATE_MIRRORS is non-empty I would generate a warning. 
> I would not abort in this case as it can be a useful setup (e.g., 
> I used this to build with a sstate cache mirror that contained the 
> native packages as this halved the build time and I thus could test 
> more combinations over a weekend).
> 
> I'll update my patch as per above unless you have other suggestions.
> 

All sound like good approaches to me. I especially like checking the
actual runtime value rather than scanning conf files.

Patch

diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
index 1064565..d5127e7 100755
--- a/scripts/contrib/bb-perf/bb-matrix.sh
+++ b/scripts/contrib/bb-perf/bb-matrix.sh
@@ -53,6 +53,10 @@  if [ $? -ne 0 ]; then
 	exit 1
 fi
 
+# Make sure neither of BB_NUMBER_THREADS, PARALLEL_MAKE and SSTATE_DIR are set
+# in local.conf
+sed -ri 's/^([[:space:]]*(BB_NUMBER_THREADS|PARALLEL_MAKE|SSTATE_DIR)[[:space:]]*\??=.*)/#\1/' conf/local.conf
+
 # Add a simple header
 echo "BB PM $TIME_STR" > $RUNTIME_LOG
 for BB in $BB_RANGE; do