[PATCH 0/2] Fixes for bb-matrix

[PATCH 0/2] Fixes for bb-matrix

[Peter Kjellerstedt]

This fixes a couple of problems with bb-matrix that I found the other
day when trying to use the script. They would have saved me a day or
two of test builds that were not building what I thought they were due
to incorrect configurations...

//Peter

The following changes since commit ed3ef0823fde89371a473d20c0127c0bd16d062b:

  qemumips: fix keyboard entry in graphical boots (2013-09-05 16:27:54 +0100)

are available in the git repository at:

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

Peter Kjellerstedt (2):
  bb-matrix: Clean before, rather than after, building
  bb-matrix: Make sure local.conf does not interfere

 scripts/contrib/bb-perf/bb-matrix.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
1.8.2.1

[PATCH 1/2] bb-matrix: Clean before, rather than after, building

[Peter Kjellerstedt]

This makes sure the the first build starts from a clean state. Otherwise
one could have the first build affected by any leftover state from
a previous build.

This also leaves a working state behind after the final build.

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
 scripts/contrib/bb-perf/bb-matrix.sh | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
index 37721fe..1064565 100755
--- a/scripts/contrib/bb-perf/bb-matrix.sh
+++ b/scripts/contrib/bb-perf/bb-matrix.sh
@@ -63,6 +63,10 @@ for BB in $BB_RANGE; do
 		date
 		echo "BB=$BB PM=$PM Logging to $BB_LOG"
 
+		echo -n "  Preparing the work directory... "
+		rm -rf pseudodone tmp sstate-cache tmp-eglibc &> /dev/null
+		echo "done"
+
 		# Export the variables under test and run the bitbake command
 		# Strip any leading zeroes before passing to bitbake
 		export BB_NUMBER_THREADS=$(echo $BB | sed 's/^0*//')
@@ -70,12 +74,6 @@ for BB in $BB_RANGE; do
 		/usr/bin/time -f "$BB $PM $TIME_STR" -a -o $RUNTIME_LOG $BB_CMD &> $BB_LOG
 
 		echo "  $(tail -n1 $RUNTIME_LOG)"
-		echo -n "  Cleaning up..."
-		mv tmp/buildstats $RUNDIR/$BB-$PM-buildstats
-		rm -f pseudodone &> /dev/null
-		rm -rf tmp &> /dev/null
-		rm -rf sstate-cache &> /dev/null
-		rm -rf tmp-eglibc &> /dev/null
-		echo "done"
+		cp -a tmp/buildstats $RUNDIR/$BB-$PM-buildstats
 	done
 done
-- 
1.8.2.1

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

[Peter Kjellerstedt]

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
+
 # Add a simple header
 echo "BB PM $TIME_STR" > $RUNTIME_LOG
 for BB in $BB_RANGE; do
-- 
1.8.2.1

Re: [PATCH 1/2] bb-matrix: Clean before, rather than after, building

[Darren Hart]

On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> This makes sure the the first build starts from a clean state. Otherwise
> one could have the first build affected by any leftover state from
> a previous build.
> 
> This also leaves a working state behind after the final build.
> 
> Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>

Hi Peter,

Thanks for taking the time to send in a fix!

> ---
>  scripts/contrib/bb-perf/bb-matrix.sh | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> index 37721fe..1064565 100755
> --- a/scripts/contrib/bb-perf/bb-matrix.sh
> +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> @@ -63,6 +63,10 @@ for BB in $BB_RANGE; do
>  		date
>  		echo "BB=$BB PM=$PM Logging to $BB_LOG"
>  
> +		echo -n "  Preparing the work directory... "
> +		rm -rf pseudodone tmp sstate-cache tmp-eglibc &> /dev/null
> +		echo "done"
> +

Makes sense to me, although there is one point worth discussing. The
tmp-eglibc directory could change depending on the DISTRO setting iiuc.
All of tmp, sstate-cache, and tmp-eglibc could be dealt with using
cleansstate I believe:

bitbake <target> -c cleansstate

Richard, should we consider using this instead?

Either way, the above is an improvement, so:

Acked-by: Darren Hart <dvhart@linux.intel.com>

Finally, In the future Peter, please have a look at the git log for the
file(s) you are patching and if there is an obvious author/maintainer
from the log, please include them on Cc. This will help ensure your
patch gets reviewed and merged in a timely fashion.

Thanks again for the patch!

>  		# Export the variables under test and run the bitbake command
>  		# Strip any leading zeroes before passing to bitbake
>  		export BB_NUMBER_THREADS=$(echo $BB | sed 's/^0*//')
> @@ -70,12 +74,6 @@ for BB in $BB_RANGE; do
>  		/usr/bin/time -f "$BB $PM $TIME_STR" -a -o $RUNTIME_LOG $BB_CMD &> $BB_LOG
>  
>  		echo "  $(tail -n1 $RUNTIME_LOG)"
> -		echo -n "  Cleaning up..."
> -		mv tmp/buildstats $RUNDIR/$BB-$PM-buildstats
> -		rm -f pseudodone &> /dev/null
> -		rm -rf tmp &> /dev/null
> -		rm -rf sstate-cache &> /dev/null
> -		rm -rf tmp-eglibc &> /dev/null
> -		echo "done"
> +		cp -a tmp/buildstats $RUNDIR/$BB-$PM-buildstats
>  	done
>  done

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

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

[Darren Hart]

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,

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH 1/2] bb-matrix: Clean before, rather than after, building

[Richard Purdie]

On Tue, 2013-09-10 at 08:30 -0700, Darren Hart wrote:
> On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > This makes sure the the first build starts from a clean state. Otherwise
> > one could have the first build affected by any leftover state from
> > a previous build.
> > 
> > This also leaves a working state behind after the final build.
> > 
> > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> 
> Hi Peter,
> 
> Thanks for taking the time to send in a fix!
> 
> > ---
> >  scripts/contrib/bb-perf/bb-matrix.sh | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> > index 37721fe..1064565 100755
> > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > @@ -63,6 +63,10 @@ for BB in $BB_RANGE; do
> >  		date
> >  		echo "BB=$BB PM=$PM Logging to $BB_LOG"
> >  
> > +		echo -n "  Preparing the work directory... "
> > +		rm -rf pseudodone tmp sstate-cache tmp-eglibc &> /dev/null
> > +		echo "done"
> > +
> 
> Makes sense to me, although there is one point worth discussing. The
> tmp-eglibc directory could change depending on the DISTRO setting iiuc.
> All of tmp, sstate-cache, and tmp-eglibc could be dealt with using
> cleansstate I believe:
> 
> bitbake <target> -c cleansstate
> 
> Richard, should we consider using this instead?

Sadly this isn't recursive, it applies to <target> and not any
dependencies of it. I'd be tempted to simplify this to rm tmp* and
depend on a convention of calling them tmp*...

Cheers,

Richard

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

[Richard Purdie]

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

Re: [PATCH 1/2] bb-matrix: Clean before, rather than after, building

[Darren Hart]

On Tue, 2013-09-10 at 16:37 +0100, Richard Purdie wrote:
> On Tue, 2013-09-10 at 08:30 -0700, Darren Hart wrote:
> > On Fri, 2013-09-06 at 18:12 +0200, Peter Kjellerstedt wrote:
> > > This makes sure the the first build starts from a clean state. Otherwise
> > > one could have the first build affected by any leftover state from
> > > a previous build.
> > > 
> > > This also leaves a working state behind after the final build.
> > > 
> > > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > 
> > Hi Peter,
> > 
> > Thanks for taking the time to send in a fix!
> > 
> > > ---
> > >  scripts/contrib/bb-perf/bb-matrix.sh | 12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh
> > > index 37721fe..1064565 100755
> > > --- a/scripts/contrib/bb-perf/bb-matrix.sh
> > > +++ b/scripts/contrib/bb-perf/bb-matrix.sh
> > > @@ -63,6 +63,10 @@ for BB in $BB_RANGE; do
> > >  		date
> > >  		echo "BB=$BB PM=$PM Logging to $BB_LOG"
> > >  
> > > +		echo -n "  Preparing the work directory... "
> > > +		rm -rf pseudodone tmp sstate-cache tmp-eglibc &> /dev/null
> > > +		echo "done"
> > > +
> > 
> > Makes sense to me, although there is one point worth discussing. The
> > tmp-eglibc directory could change depending on the DISTRO setting iiuc.
> > All of tmp, sstate-cache, and tmp-eglibc could be dealt with using
> > cleansstate I believe:
> > 
> > bitbake <target> -c cleansstate
> > 
> > Richard, should we consider using this instead?
> 
> Sadly this isn't recursive, it applies to <target> and not any
> dependencies of it. I'd be tempted to simplify this to rm tmp* and
> depend on a convention of calling them tmp*...
> 

yes.... but.... any kind of a wildcard in an rm -rf statement makes me
see purple....

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH 1/2] bb-matrix: Clean before, rather than after, building

[Burton, Ross]

On 10 September 2013 16:37, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> Sadly this isn't recursive, it applies to <target> and not any
> dependencies of it. I'd be tempted to simplify this to rm tmp* and
> depend on a convention of calling them tmp*...

You could invoke bitbake -e and get the right directories to remove
(see wipe-sysroot).

Ross

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

[Darren Hart]

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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

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

[Richard Purdie]

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

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

[Peter Kjellerstedt]

> -----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

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

[Darren Hart]

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.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel