Patchwork staging: Avoid staging the same binaries again and again

login
register
mail settings
Submitter Phil Blundell
Date Sept. 24, 2012, 6:26 a.m.
Message ID <1348467993.4444.252.camel@x121e.pbcl.net>
Download mbox | patch
Permalink /patch/37105/
State New
Headers show

Comments

Phil Blundell - Sept. 24, 2012, 6:26 a.m.
It is possible that some or all of ${bindir}, ${base_bindir},
${sbindir} and ${base_sbindir} might be the same directory.  In
particular, for micro it is often the case that all four are
the same.  There is clearly no point in staging the same directory
four times, so let's keep track of which paths have already been
staged and skip any later duplicates.

Signed-off-by: Phil Blundell <pb@pbcl.net>
---
 meta/classes/staging.bbclass |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)
Richard Purdie - Sept. 24, 2012, 8:51 a.m.
On Mon, 2012-09-24 at 07:26 +0100, Phil Blundell wrote:
> It is possible that some or all of ${bindir}, ${base_bindir},
> ${sbindir} and ${base_sbindir} might be the same directory.  In
> particular, for micro it is often the case that all four are
> the same.  There is clearly no point in staging the same directory
> four times, so let's keep track of which paths have already been
> staged and skip any later duplicates.
> 
> Signed-off-by: Phil Blundell <pb@pbcl.net>
> ---
>  meta/classes/staging.bbclass |   24 ++++++++++++++++++++----
>  1 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
> index 82624cb..bd97ed5 100644
> --- a/meta/classes/staging.bbclass
> +++ b/meta/classes/staging.bbclass
> @@ -21,6 +21,21 @@ sysroot_stage_dir() {
>  	fi
>  }
>  
> +sysroot_stage_dir_once() {
> +	src="$1"
> +	dest="$2"
> +
> +	src_b64=`echo $src | base64 -w 0`
> +	for d in $staged_dirs; do
> +	    if [ $d = $src_b64 ]; then
> +	       echo "$src has been staged already"
> +	       return
> +	    fi
> +	done
> +	sysroot_stage_dir $src $dest
> +	staged_dirs="$staged_dirs $src_b64"
> +}

Wouldn't testing whether $dest already exists work just as well?

Cheers,

Richard

>  sysroot_stage_libdir() {
>  	src="$1"
>  	dest="$2"
> @@ -34,10 +49,11 @@ sysroot_stage_dirs() {
>  
>  	sysroot_stage_dir $from${includedir} $to${includedir}
>  	if [ "${BUILD_SYS}" = "${HOST_SYS}" ]; then
> -		sysroot_stage_dir $from${bindir} $to${bindir}
> -		sysroot_stage_dir $from${sbindir} $to${sbindir}
> -		sysroot_stage_dir $from${base_bindir} $to${base_bindir}
> -		sysroot_stage_dir $from${base_sbindir} $to${base_sbindir}
> +		staged_dirs=
> +		sysroot_stage_dir_once $from${bindir} $to${bindir}
> +		sysroot_stage_dir_once $from${sbindir} $to${sbindir}
> +		sysroot_stage_dir_once $from${base_bindir} $to${base_bindir}
> +		sysroot_stage_dir_once $from${base_sbindir} $to${base_sbindir}
>  		sysroot_stage_dir $from${libexecdir} $to${libexecdir}
>  		sysroot_stage_dir $from${sysconfdir} $to${sysconfdir}
>  		sysroot_stage_dir $from${localstatedir} $to${localstatedir}
Phil Blundell - Sept. 24, 2012, 9:40 a.m.
On Mon, 2012-09-24 at 09:51 +0100, Richard Purdie wrote:
> Wouldn't testing whether $dest already exists work just as well?

I wasn't totally confident that $dest was guaranteed never to exist in
advance (and that we'd never want to stage multiple $srcs into a single
$dest).  But yes, if both of those are true then I think testing for the
existence of $dest would be fine.

p.
Richard Purdie - Sept. 24, 2012, 9:54 a.m.
On Mon, 2012-09-24 at 10:40 +0100, Phil Blundell wrote:
> On Mon, 2012-09-24 at 09:51 +0100, Richard Purdie wrote:
> > Wouldn't testing whether $dest already exists work just as well?
> 
> I wasn't totally confident that $dest was guaranteed never to exist in
> advance

do_populate_sysroot[cleandirs] = "${SYSROOT_DESTDIR}"

(dest is ${SYSROOT_DESTDIR})

>  (and that we'd never want to stage multiple $srcs into a single
> $dest).

sysroot_stage_* are symmetrical so I can't imagine this happening.

The main worry would be something happening before sysroot_stage_all.
SYSROOT_PREPROCESS_FUNCS happen afterwards so there is at least a hook
used in most cases that would avoid the issue.

I'm torn whether its better to be simple or less fragile in this case.
Or simply do some tests (if ${bindir} != ${sbindir}) and so on.

Cheers,

Richard

>   But yes, if both of those are true then I think testing for the
> existence of $dest would be fine.
> 
> p.
> 
>
Phil Blundell - Sept. 24, 2012, 9:59 a.m.
On Mon, 2012-09-24 at 10:54 +0100, Richard Purdie wrote:
> I'm torn whether its better to be simple or less fragile in this case.
> Or simply do some tests (if ${bindir} != ${sbindir}) and so on.

The tests would get a bit ungainly, since you'd end up with 'if !
[ "${sbindir} = "${bindir}" -o "${sbindir}" = "${base_sbindir}" -o
"${sbindir}" = "${base_sbindir}" ... ]' kind of things.  But yes, that
would certainly work as well.

p.
Phil Blundell - Sept. 27, 2012, 9:18 p.m.
On Mon, 2012-09-24 at 10:54 +0100, Richard Purdie wrote:
> sysroot_stage_* are symmetrical so I can't imagine this happening.
> 
> The main worry would be something happening before sysroot_stage_all.
> SYSROOT_PREPROCESS_FUNCS happen afterwards so there is at least a hook
> used in most cases that would avoid the issue.
> 
> I'm torn whether its better to be simple or less fragile in this case.
> Or simply do some tests (if ${bindir} != ${sbindir}) and so on.

I'm not quite sure what the conclusion was from this previous
discussion.  Did you want me to redo the patch to work in some other
way?

p.
Richard Purdie - Sept. 27, 2012, 9:35 p.m.
On Thu, 2012-09-27 at 22:18 +0100, Phil Blundell wrote:
> On Mon, 2012-09-24 at 10:54 +0100, Richard Purdie wrote:
> > sysroot_stage_* are symmetrical so I can't imagine this happening.
> > 
> > The main worry would be something happening before sysroot_stage_all.
> > SYSROOT_PREPROCESS_FUNCS happen afterwards so there is at least a hook
> > used in most cases that would avoid the issue.
> > 
> > I'm torn whether its better to be simple or less fragile in this case.
> > Or simply do some tests (if ${bindir} != ${sbindir}) and so on.
> 
> I'm not quite sure what the conclusion was from this previous
> discussion.  Did you want me to redo the patch to work in some other
> way?

The release is now at -rc2 and this patch hasn't reached the threshold
of things I'm willing to take. I've tried to at least get some of the
patches you're posted in but I wish we'd had them a couple of weeks
ago :/.

I also wish we could find some neater/simpler way of doing this since
I'm not a fan of "obscuring" the code more than we have to. I'm not
coming up with any great ways of doing it though.

Cheers,

Richard
Phil Blundell - Sept. 28, 2012, 10:43 a.m.
On Thu, 2012-09-27 at 22:35 +0100, Richard Purdie wrote:
> The release is now at -rc2 and this patch hasn't reached the threshold
> of things I'm willing to take. I've tried to at least get some of the
> patches you're posted in but I wish we'd had them a couple of weeks
> ago :/.

Okay, fair enough.  There's no particular rush with any of that stuff
and it can perfectly well wait until your release is out of the way.  Do
you want me to resend those patches at some later date?

You might like to consider taking the eglibc patch from a few days ago
since it is arguably a regression that eglibc has a build dependency on
perl when it didn't use to.  But of course this may not matter for the
majority of yocto users so it might not be a problem in practice.

p.

Patch

diff --git a/meta/classes/staging.bbclass b/meta/classes/staging.bbclass
index 82624cb..bd97ed5 100644
--- a/meta/classes/staging.bbclass
+++ b/meta/classes/staging.bbclass
@@ -21,6 +21,21 @@  sysroot_stage_dir() {
 	fi
 }
 
+sysroot_stage_dir_once() {
+	src="$1"
+	dest="$2"
+
+	src_b64=`echo $src | base64 -w 0`
+	for d in $staged_dirs; do
+	    if [ $d = $src_b64 ]; then
+	       echo "$src has been staged already"
+	       return
+	    fi
+	done
+	sysroot_stage_dir $src $dest
+	staged_dirs="$staged_dirs $src_b64"
+}
+
 sysroot_stage_libdir() {
 	src="$1"
 	dest="$2"
@@ -34,10 +49,11 @@  sysroot_stage_dirs() {
 
 	sysroot_stage_dir $from${includedir} $to${includedir}
 	if [ "${BUILD_SYS}" = "${HOST_SYS}" ]; then
-		sysroot_stage_dir $from${bindir} $to${bindir}
-		sysroot_stage_dir $from${sbindir} $to${sbindir}
-		sysroot_stage_dir $from${base_bindir} $to${base_bindir}
-		sysroot_stage_dir $from${base_sbindir} $to${base_sbindir}
+		staged_dirs=
+		sysroot_stage_dir_once $from${bindir} $to${bindir}
+		sysroot_stage_dir_once $from${sbindir} $to${sbindir}
+		sysroot_stage_dir_once $from${base_bindir} $to${base_bindir}
+		sysroot_stage_dir_once $from${base_sbindir} $to${base_sbindir}
 		sysroot_stage_dir $from${libexecdir} $to${libexecdir}
 		sysroot_stage_dir $from${sysconfdir} $to${sysconfdir}
 		sysroot_stage_dir $from${localstatedir} $to${localstatedir}