diff mbox series

[V2] oe-buildenv-internal: remove path from previous project

Message ID 20230512031550.1715288-1-Qi.Chen@windriver.com
State Accepted, archived
Commit 3405a3221b8f6641a8e42b04bc7acd6e032aeab8
Headers show
Series [V2] oe-buildenv-internal: remove path from previous project | expand

Commit Message

ChenQi May 12, 2023, 3:15 a.m. UTC 
From: Chen Qi <Qi.Chen@windriver.com>

For now, only that paths added from current project are removed
to avoid PATH growing unnecessarily. This is to handle the case
of sourcing the init script into different build directories.

However, if we source the init script from different projects into
different build directories, the paths added by previous projects
are not cleaned up.

To avoid this, we record the paths added into OE_ADDED_PATH, and
remove it in the next sourcing.

Note that the paths, "$OEROOT/scripts:$BITBAKEDIR/bin:", are added
as a whole. A previous commit, "oe-buildenv-internal: Add paths to $PATH individually",
made the change to treat these two paths separately, the reason was
not "assuming the path to the scripts directory always is in $PATH
directly before the bitbake directory". But this is exactly the effect
of the codes. I see no reason why we should complicate things.

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 scripts/oe-buildenv-internal | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Peter Kjellerstedt May 12, 2023, 12:27 p.m. UTC | #1 
> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Chen Qi via
> lists.openembedded.org
> Sent: den 12 maj 2023 05:16
> To: openembedded-core@lists.openembedded.org
> Subject: [OE-core][PATCH V2] oe-buildenv-internal: remove path from previous project
> 
> From: Chen Qi <Qi.Chen@windriver.com>
> 
> For now, only that paths added from current project are removed

Change to:

Until now, only paths added for the current project were removed

> to avoid PATH growing unnecessarily. This is to handle the case
> of sourcing the init script into different build directories.
> 
> However, if we source the init script from different projects into
> different build directories, the paths added by previous projects
> are not cleaned up.
> 
> To avoid this, we record the paths added into OE_ADDED_PATH, and
> remove it in the next sourcing.
> 
> Note that the paths, "$OEROOT/scripts:$BITBAKEDIR/bin:", are added
> as a whole. A previous commit, "oe-buildenv-internal: Add paths to $PATH individually",
> made the change to treat these two paths separately, the reason was
> not "assuming the path to the scripts directory always is in $PATH
> directly before the bitbake directory". But this is exactly the effect
> of the codes. I see no reason why we should complicate things.

And now I have to try to remember why I changed this seven years ago...
Unfortunately, time has taken its toll on my memory and I do not 
remember why I changed this. It may have been as simple as to make the 
code more generic and possibly handle more paths being added, or to take 
into account that other things may modify PATH after oe-init-build-env 
has been sourced. I guess I will just have to wait and see if anything 
breaks after this change is applied...

> 
> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> ---
>  scripts/oe-buildenv-internal | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
> index 485d4c52e1..598120a86d 100755
> --- a/scripts/oe-buildenv-internal
> +++ b/scripts/oe-buildenv-internal
> @@ -92,15 +92,13 @@ fi
>  PYTHONPATH=$BITBAKEDIR/lib:$PYTHONPATH
>  export PYTHONPATH
> 
> +# Remove old added path
> +[ -n "$OE_ADDED_PATH" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATH##") || PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")

Change to:

# Remove any paths added by sourcing this script before 
[ "$OE_ADDED_PATHS" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATHS##") ||
    PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")

and rename `OE_ADDED_PATH` to `OE_ADDED_PATHS` below.

> +OE_ADDED_PATH="$OEROOT/scripts:$BITBAKEDIR/bin:"

Move the above line to after the comment below.

>  # Make sure our paths are at the beginning of $PATH
> -for newpath in "$BITBAKEDIR/bin" "$OEROOT/scripts"; do
> -    # Remove any existences of $newpath from $PATH
> -    PATH=$(echo $PATH | sed -re "s#(^|:)$newpath(:|$)#\2#g;s#^:##")
> -
> -    # Add $newpath to $PATH
> -    PATH="$newpath:$PATH"
> -done
> -unset BITBAKEDIR newpath
> +PATH="$OE_ADDED_PATH$PATH"
> +export OE_ADDED_PATH

Remove `export OE_ADDED_PATH`. This is a sourced script and the 
variable should not be needed by anything else.

> +unset BITBAKEDIR

All in all, this is how I would like it to look in the end:

# Remove any paths added by sourcing this script before 
[ "$OE_ADDED_PATHS" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATHS##") ||
    PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")

# Make sure our paths are at the beginning of $PATH
OE_ADDED_PATHS="$OEROOT/scripts:$BITBAKEDIR/bin:"
PATH="$OE_ADDED_PATHS$PATH"

# This is not needed any more
unset BITBAKEDIR

> 
>  # Used by the runqemu script
>  export BUILDDIR

You may as well remove `export PATH` on the next line too. It is 
not needed.

> --
> 2.34.1

//Peter
ChenQi May 15, 2023, 4:53 a.m. UTC | #2 
Hi Peter,

Thanks for your careful review. I'll send out V3 according to your 
suggestion.

Just one thing. I think we need to export OE_ADDED_PATHS for two reason:
1. users can clearly see it in the `env' command's output.
2. if the prefixed PATH is carried into some subprocess (e.g., a subshell),
        so should this OE_ADDED_PATHS variable that prefixes it.

Regards,
Qi

On 5/12/23 20:27, Peter Kjellerstedt wrote:
>> -----Original Message-----
>> From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Chen Qi via
>> lists.openembedded.org
>> Sent: den 12 maj 2023 05:16
>> To: openembedded-core@lists.openembedded.org
>> Subject: [OE-core][PATCH V2] oe-buildenv-internal: remove path from previous project
>>
>> From: Chen Qi <Qi.Chen@windriver.com>
>>
>> For now, only that paths added from current project are removed
> Change to:
>
> Until now, only paths added for the current project were removed
>
>> to avoid PATH growing unnecessarily. This is to handle the case
>> of sourcing the init script into different build directories.
>>
>> However, if we source the init script from different projects into
>> different build directories, the paths added by previous projects
>> are not cleaned up.
>>
>> To avoid this, we record the paths added into OE_ADDED_PATH, and
>> remove it in the next sourcing.
>>
>> Note that the paths, "$OEROOT/scripts:$BITBAKEDIR/bin:", are added
>> as a whole. A previous commit, "oe-buildenv-internal: Add paths to $PATH individually",
>> made the change to treat these two paths separately, the reason was
>> not "assuming the path to the scripts directory always is in $PATH
>> directly before the bitbake directory". But this is exactly the effect
>> of the codes. I see no reason why we should complicate things.
> And now I have to try to remember why I changed this seven years ago...
> Unfortunately, time has taken its toll on my memory and I do not
> remember why I changed this. It may have been as simple as to make the
> code more generic and possibly handle more paths being added, or to take
> into account that other things may modify PATH after oe-init-build-env
> has been sourced. I guess I will just have to wait and see if anything
> breaks after this change is applied...
>
>> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
>> ---
>>   scripts/oe-buildenv-internal | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
>> index 485d4c52e1..598120a86d 100755
>> --- a/scripts/oe-buildenv-internal
>> +++ b/scripts/oe-buildenv-internal
>> @@ -92,15 +92,13 @@ fi
>>   PYTHONPATH=$BITBAKEDIR/lib:$PYTHONPATH
>>   export PYTHONPATH
>>
>> +# Remove old added path
>> +[ -n "$OE_ADDED_PATH" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATH##") || PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")
> Change to:
>
> # Remove any paths added by sourcing this script before
> [ "$OE_ADDED_PATHS" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATHS##") ||
>      PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")
>
> and rename `OE_ADDED_PATH` to `OE_ADDED_PATHS` below.
>
>> +OE_ADDED_PATH="$OEROOT/scripts:$BITBAKEDIR/bin:"
> Move the above line to after the comment below.
>
>>   # Make sure our paths are at the beginning of $PATH
>> -for newpath in "$BITBAKEDIR/bin" "$OEROOT/scripts"; do
>> -    # Remove any existences of $newpath from $PATH
>> -    PATH=$(echo $PATH | sed -re "s#(^|:)$newpath(:|$)#\2#g;s#^:##")
>> -
>> -    # Add $newpath to $PATH
>> -    PATH="$newpath:$PATH"
>> -done
>> -unset BITBAKEDIR newpath
>> +PATH="$OE_ADDED_PATH$PATH"
>> +export OE_ADDED_PATH
> Remove `export OE_ADDED_PATH`. This is a sourced script and the
> variable should not be needed by anything else.
>
>> +unset BITBAKEDIR
> All in all, this is how I would like it to look in the end:
>
> # Remove any paths added by sourcing this script before
> [ "$OE_ADDED_PATHS" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATHS##") ||
>      PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")
>
> # Make sure our paths are at the beginning of $PATH
> OE_ADDED_PATHS="$OEROOT/scripts:$BITBAKEDIR/bin:"
> PATH="$OE_ADDED_PATHS$PATH"
>
> # This is not needed any more
> unset BITBAKEDIR
>
>>   # Used by the runqemu script
>>   export BUILDDIR
> You may as well remove `export PATH` on the next line too. It is
> not needed.
>
>> --
>> 2.34.1
> //Peter
>
Peter Kjellerstedt May 15, 2023, 11:48 a.m. UTC | #3 
> -----Original Message-----
> From: ChenQi <Qi.Chen@windriver.com>
> Sent: den 15 maj 2023 06:53
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core][PATCH V2] oe-buildenv-internal: remove path from
> previous project
> 
> Hi Peter,
> 
> Thanks for your careful review. I'll send out V3 according to your
> suggestion.
> 
> Just one thing. I think we need to export OE_ADDED_PATHS for two reason:
> 1. users can clearly see it in the `env' command's output.

Sure, but it should really be seen as an internal variable for this 
script. The only reason it has to be in the environment is for it to 
be available for the next run of this script. Nothing else should care 
about it.

> 2. if the prefixed PATH is carried into some subprocess (e.g., a subshell),
>         so should this OE_ADDED_PATHS variable that prefixes it.

That is not how variables in shell works. They are expanded immediately, 
so PATH will be set to whatever value $OE_ADDED_PATHS expands to and you 
will not see any reference to it after PATH has been modified.

> 
> Regards,
> Qi

//Peter
ChenQi May 15, 2023, 2:01 p.m. UTC | #4 
Hi Peter,

Sorry I didn't express my point clearly.

I know that shell variable is expanded immediately. What I really meant is that as the paths, "$OEROOT/scripts:$BITBAKEDIR/bin:", have been added to PATH, and PATH is exported, some variable would also need to be exported to carry that information of the added paths. Otherwise, a subprocess (e.g., a subshell) will not know enough information to act accordingly.

e.g.,
1) . path1/to/poky/oe-init-build-env build-1
2) bash
3) . path2/to/poky/oe-init-build-env build-2

With the above three steps, the resulting PATH is not what we expect if OE_ADDED_PATHS is not exported.

Regards,
Qi


-----Original Message-----
From: Peter Kjellerstedt <peter.kjellerstedt@axis.com> 
Sent: Monday, May 15, 2023 7:48 PM
To: Chen, Qi <Qi.Chen@windriver.com>; openembedded-core@lists.openembedded.org
Subject: RE: [OE-core][PATCH V2] oe-buildenv-internal: remove path from previous project

> -----Original Message-----
> From: ChenQi <Qi.Chen@windriver.com>
> Sent: den 15 maj 2023 06:53
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; 
> openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core][PATCH V2] oe-buildenv-internal: remove path 
> from previous project
> 
> Hi Peter,
> 
> Thanks for your careful review. I'll send out V3 according to your 
> suggestion.
> 
> Just one thing. I think we need to export OE_ADDED_PATHS for two reason:
> 1. users can clearly see it in the `env' command's output.

Sure, but it should really be seen as an internal variable for this script. The only reason it has to be in the environment is for it to be available for the next run of this script. Nothing else should care about it.

> 2. if the prefixed PATH is carried into some subprocess (e.g., a 
> subshell),
>         so should this OE_ADDED_PATHS variable that prefixes it.

That is not how variables in shell works. They are expanded immediately, so PATH will be set to whatever value $OE_ADDED_PATHS expands to and you will not see any reference to it after PATH has been modified.

> 
> Regards,
> Qi

//Peter
diff mbox series

Patch

diff --git a/scripts/oe-buildenv-internal b/scripts/oe-buildenv-internal
index 485d4c52e1..598120a86d 100755
--- a/scripts/oe-buildenv-internal
+++ b/scripts/oe-buildenv-internal
@@ -92,15 +92,13 @@  fi
 PYTHONPATH=$BITBAKEDIR/lib:$PYTHONPATH
 export PYTHONPATH
 
+# Remove old added path
+[ -n "$OE_ADDED_PATH" ] && PATH=$(echo $PATH | sed -e "s#$OE_ADDED_PATH##") || PATH=$(echo $PATH | sed -e "s#$OEROOT/scripts:$BITBAKEDIR/bin:##")
+OE_ADDED_PATH="$OEROOT/scripts:$BITBAKEDIR/bin:"
 # Make sure our paths are at the beginning of $PATH
-for newpath in "$BITBAKEDIR/bin" "$OEROOT/scripts"; do
-    # Remove any existences of $newpath from $PATH
-    PATH=$(echo $PATH | sed -re "s#(^|:)$newpath(:|$)#\2#g;s#^:##")
-
-    # Add $newpath to $PATH
-    PATH="$newpath:$PATH"
-done
-unset BITBAKEDIR newpath
+PATH="$OE_ADDED_PATH$PATH"
+export OE_ADDED_PATH
+unset BITBAKEDIR
 
 # Used by the runqemu script
 export BUILDDIR