| Message ID | a20f68229f1499d9020bd9cd8dc593e5f6635967.1365179842.git.pkj@axis.com |
|---|---|
| State | Accepted |
| Commit | 161abcd3672f83990ede03d67b7388678c07150e |
| Headers | show |
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
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
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
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.
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
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 :)
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?
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?
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
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
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 >
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(-)