Patchwork [2/3] oe-buildenv-internal: Only add to $PATH if needed

login
register
mail settings
Submitter Peter Kjellerstedt
Date April 5, 2013, 4:59 p.m.
Message ID <a20f68229f1499d9020bd9cd8dc593e5f6635967.1365179842.git.pkj@axis.com>
Download mbox | patch
Permalink /patch/47523/
State Accepted
Commit 161abcd3672f83990ede03d67b7388678c07150e
Headers show

Comments

Peter Kjellerstedt - April 5, 2013, 4:59 p.m.
If $PATH already has the needed paths at the beginning, there is no need
to add them again. This allows rerunning oe-init-build-env for the same
directory without having $PATH increase unnecessarily every time.

Signed-off-by: Peter Kjellerstedt <pkj@axis.com>
---
 scripts/oe-buildenv-internal | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Trevor Woerner - April 8, 2013, 5:31 p.m.
On Fri, Apr 5, 2013 at 12:59 PM, Peter Kjellerstedt
<peter.kjellerstedt@axis.com> wrote:
> +[ "${PATH#$NEWPATHS}" != "$PATH" ] || PATH="$NEWPATHS$PATH"

This is certainly a welcome addition in functionality, but it relies
on the pattern remaining at the start of the PATH (i.e. the user
hasn't played with PATH in any way). Could we not use the
${parameter/pattern/string} parameter expansion instead (e.g.
"${PATH/$NEWPATHS/}") so it doesn't matter whether the user has
further modified the PATH?

Best regards,
    Trevor
Paul Eggleton - April 8, 2013, 5:48 p.m.
On Monday 08 April 2013 13:31:50 Trevor Woerner wrote:
> On Fri, Apr 5, 2013 at 12:59 PM, Peter Kjellerstedt
> <peter.kjellerstedt@axis.com> wrote:
> > +[ "${PATH#$NEWPATHS}" != "$PATH" ] || PATH="$NEWPATHS$PATH"
> 
> This is certainly a welcome addition in functionality, but it relies
> on the pattern remaining at the start of the PATH (i.e. the user
> hasn't played with PATH in any way). Could we not use the
> ${parameter/pattern/string} parameter expansion instead (e.g.
> "${PATH/$NEWPATHS/}") so it doesn't matter whether the user has
> further modified the PATH?

Unfortunately I think this is specific to bash, so it may not be portable. 
Maybe the equivalent can be achieved with sed however.

Cheers,
Paul
Chris Larson - April 8, 2013, 10:40 p.m.
On Mon, Apr 8, 2013 at 10:48 AM, Paul Eggleton <
paul.eggleton@linux.intel.com> wrote:

> On Monday 08 April 2013 13:31:50 Trevor Woerner wrote:
> > On Fri, Apr 5, 2013 at 12:59 PM, Peter Kjellerstedt
> > <peter.kjellerstedt@axis.com> wrote:
> > > +[ "${PATH#$NEWPATHS}" != "$PATH" ] || PATH="$NEWPATHS$PATH"
> >
> > This is certainly a welcome addition in functionality, but it relies
> > on the pattern remaining at the start of the PATH (i.e. the user
> > hasn't played with PATH in any way). Could we not use the
> > ${parameter/pattern/string} parameter expansion instead (e.g.
> > "${PATH/$NEWPATHS/}") so it doesn't matter whether the user has
> > further modified the PATH?
>
> Unfortunately I think this is specific to bash, so it may not be portable.
> Maybe the equivalent can be achieved with sed however.


Also, neither version matches the : separator, which means we could in
theory get false positive matches.
Peter Kjellerstedt - April 9, 2013, 10:01 a.m.
The colon separator is included at the end of $NEWPATHS so it should not be possible for any false positives.

The intention of the patch was to avoid $PATH growing when rerunning oe-init-build-env in the same directory multiple times. I only match at the start of $PATH as I wanted to maintain the current functionality that guarantees that the paths are added to the beginning of $PATH and therefore searched before anything else.

I did not want to go into analyzing $PATH which would have been needed to remove paths for other build trees. Also, I do not actually see this as possible since the static parts of the added paths (/scripts and /bin/) are to generic to be able to remove those paths without the risk of accidentally removing something unrelated.

However, one thing that can easily be done is to instead reorder “$SOMEPATHS:$NEWPATHS$SOMEOTHERPATHS” to “$NEWPATHS$SOMEPATHS:$SOMEOTHERPATHS”. That should maintain the property of having the new paths at the beginning, while at the same time not grow $PATH unnecessarily if the paths are already present, but preceded by some other paths.

I will provide a second version of the patch with the relevant part changed to:

# Make sure our paths are at the beginning of $PATH
NEWPATHS="${OEROOT}/scripts:$BITBAKEDIR/bin:"
PATH=$NEWPATHS$(echo $PATH | sed -e "s|:$NEWPATHS|:|g" -e "s|^$NEWPATHS||")
unset BITBAKEDIR NEWPATHS

//Peter

From: kergoth@gmail.com [mailto:kergoth@gmail.com] On Behalf Of Chris Larson

Sent: den 9 april 2013 00:40
To: Paul Eggleton
Cc: Trevor Woerner; Peter Kjellerstedt; Patches and discussions about the oe-core layer
Subject: Re: [OE-core] [PATCH 2/3] oe-buildenv-internal: Only add to $PATH if needed


On Mon, Apr 8, 2013 at 10:48 AM, Paul Eggleton <paul.eggleton@linux.intel.com<mailto:paul.eggleton@linux.intel.com>> wrote:
On Monday 08 April 2013 13:31:50 Trevor Woerner wrote:
> On Fri, Apr 5, 2013 at 12:59 PM, Peter Kjellerstedt

> <peter.kjellerstedt@axis.com<mailto:peter.kjellerstedt@axis.com>> wrote:

> > +[ "${PATH#$NEWPATHS}" != "$PATH" ] || PATH="$NEWPATHS$PATH"

>

> This is certainly a welcome addition in functionality, but it relies

> on the pattern remaining at the start of the PATH (i.e. the user

> hasn't played with PATH in any way). Could we not use the

> ${parameter/pattern/string} parameter expansion instead (e.g.

> "${PATH/$NEWPATHS/}") so it doesn't matter whether the user has

> further modified the PATH?

Unfortunately I think this is specific to bash, so it may not be portable.
Maybe the equivalent can be achieved with sed however.

Also, neither version matches the : separator, which means we could in theory get false positive matches.
--
Christopher Larson
Chris Larson - April 9, 2013, 2:44 p.m.
On Tue, Apr 9, 2013 at 3:01 AM, Peter Kjellerstedt <
peter.kjellerstedt@axis.com> wrote:

> The colon separator is included at the end of $NEWPATHS so it should not
> be possible for any false positives.


Ah, right, thanks for the clarification :)
Trevor Woerner - April 9, 2013, 5:29 p.m.
On Mon, Apr 8, 2013 at 1:48 PM, Paul Eggleton
<paul.eggleton@linux.intel.com> wrote:
> Unfortunately I think this is specific to bash, so it may not be portable.
> Maybe the equivalent can be achieved with sed however.

Under which shells do we expect a Yocto build to succeed? The latest
version of this change wouldn't work under tcsh, for example. (Not
that I'm suggesting it should!) Isn't there already an inherent
Bourne-ish expectation?
Trevor Woerner - April 9, 2013, 5:34 p.m.
On Tue, Apr 9, 2013 at 1:29 PM, Trevor Woerner <twoerner@gmail.com> wrote:
> Under which shells do we expect a Yocto build to succeed?

Whoops! My bad.

sh -> yes
bash -> not so much

Let me rephrase: are bash-specific features to be so feared?
Khem Raj - April 9, 2013, 5:40 p.m.
On Apr 9, 2013, at 10:29 AM, Trevor Woerner <twoerner@gmail.com> wrote:

> On Mon, Apr 8, 2013 at 1:48 PM, Paul Eggleton
> <paul.eggleton@linux.intel.com> wrote:
>> Unfortunately I think this is specific to bash, so it may not be portable.
>> Maybe the equivalent can be achieved with sed however.
> 
> Under which shells do we expect a Yocto build to succeed? The latest
> version of this change wouldn't work under tcsh, for example. (Not
> that I'm suggesting it should!) Isn't there already an inherent
> Bourne-ish expectation?
> 

well we try to not depend on bash or any other shell. e.g. I can use dash with no problems
Paul Eggleton - April 9, 2013, 6:12 p.m.
On Tuesday 09 April 2013 13:34:43 Trevor Woerner wrote:
> On Tue, Apr 9, 2013 at 1:29 PM, Trevor Woerner <twoerner@gmail.com> wrote:
> > Under which shells do we expect a Yocto build to succeed?
> 
> Whoops! My bad.
> 
> sh -> yes
> bash -> not so much
> 
> Let me rephrase: are bash-specific features to be so feared?

If we can avoid them, yes. The stuff we absolutely must not have is:

* bashisms in scripts that start with #!/bin/sh as opposed to #!/bin/bash (and 
for scripts that are installed on the target we try to avoid the latter as it 
just means bash has to be installed on the target as well, which is often not 
desirable)

* bashisms in shell functions within recipes since these are executed under 
/bin/sh

Even if we were to ignore alternative shells such as zsh and tcsh, bashisms in 
the above cases will cause breakages on Ubuntu (where /bin/sh is dash by 
default). From the denzil (Yocto Project 1.2) release onwards we have been 
stamping out bashisms and trying to avoid introducing any new ones.

Cheers,
Paul
Mark Hatle - April 9, 2013, 10:14 p.m.
On 4/9/13 12:29 PM, Trevor Woerner wrote:
> On Mon, Apr 8, 2013 at 1:48 PM, Paul Eggleton
> <paul.eggleton@linux.intel.com> wrote:
>> Unfortunately I think this is specific to bash, so it may not be portable.
>> Maybe the equivalent can be achieved with sed however.
>
> Under which shells do we expect a Yocto build to succeed? The latest
> version of this change wouldn't work under tcsh, for example. (Not
> that I'm suggesting it should!) Isn't there already an inherent
> Bourne-ish expectation?

dash, ash, bash and zsh.  POSIX compliant shells.

These are the places I try to verify scripts that refer to /bin/sh.

--Mark

>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>

Patch

diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
index 0a4d324..df95389 100755
--- a/scripts/oe-buildenv-internal
+++ b/scripts/oe-buildenv-internal
@@ -74,8 +74,9 @@  if ! (test -d "$BITBAKEDIR"); then
     return 1
 fi
 
-PATH="${OEROOT}/scripts:$BITBAKEDIR/bin/:$PATH"
-unset BITBAKEDIR
+NEWPATHS="${OEROOT}/scripts:$BITBAKEDIR/bin/:"
+[ "${PATH#$NEWPATHS}" != "$PATH" ] || PATH="$NEWPATHS$PATH"
+unset BITBAKEDIR NEWPATHS
 
 # Used by the runqemu script
 export BUILDDIR