[PATCH] isar-init-build-env: Always set ISARROOT

[PATCH] isar-init-build-env: Always set ISARROOT

[vijaikumar.kanagarajan]

From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>

When switching between two ISAR workspaces in the same shell
session, the ISARROOT setting of the previous workspace would be
picked up for the new workspace resulting in an incorrect
configuration.

Always set ISARROOT irrespective of whether it is empty or not.

Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
---
 isar-init-build-env | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/isar-init-build-env b/isar-init-build-env
index b08bb59..9c308e4 100755
--- a/isar-init-build-env
+++ b/isar-init-build-env
@@ -42,10 +42,8 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" ]; then
     exit 1
 fi
 
-if [ -z "$ISARROOT" ]; then
-    ISARROOT=$(dirname "$THIS_SCRIPT")
-    ISARROOT=$(readlink -f "$ISARROOT")
-fi
+ISARROOT=$(dirname "$THIS_SCRIPT")
+ISARROOT=$(readlink -f "$ISARROOT")
 unset THIS_SCRIPT
 
 ISARROOT=$(readlink -f "$ISARROOT")
-- 
2.17.1

[PATCH v2] isar-init-build-env: Always set ISARROOT

[vijaikumar.kanagarajan]

From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>

When switching between two ISAR workspaces in the same shell
session, the ISARROOT setting of the previous workspace would be
picked up for the new workspace resulting in an incorrect
configuration.

Always set ISARROOT irrespective of whether it is empty or not.

Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
---
Changes in v2:
    - Remove redundant code

 isar-init-build-env | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/isar-init-build-env b/isar-init-build-env
index b08bb59..404efcb 100755
--- a/isar-init-build-env
+++ b/isar-init-build-env
@@ -42,13 +42,10 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" ]; then
     exit 1
 fi
 
-if [ -z "$ISARROOT" ]; then
-    ISARROOT=$(dirname "$THIS_SCRIPT")
-    ISARROOT=$(readlink -f "$ISARROOT")
-fi
+ISARROOT=$(dirname "$THIS_SCRIPT")
+ISARROOT=$(readlink -f "$ISARROOT")
 unset THIS_SCRIPT
 
-ISARROOT=$(readlink -f "$ISARROOT")
 export ISARROOT
 . "$ISARROOT/scripts/isar-buildenv-internal" "$1" && \
     TEMPLATECONF="$TEMPLATECONF" "$ISARROOT/scripts/isar-setup-builddir" || {
-- 
2.17.1

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[Jan Kiszka]

On 27.09.19 23:43, vijaikumar.kanagarajan@gmail.com wrote:
> From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> 
> When switching between two ISAR workspaces in the same shell
> session, the ISARROOT setting of the previous workspace would be
> picked up for the new workspace resulting in an incorrect
> configuration.
> 
> Always set ISARROOT irrespective of whether it is empty or not.
> 
> Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> ---
> Changes in v2:
>      - Remove redundant code
> 
>   isar-init-build-env | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/isar-init-build-env b/isar-init-build-env
> index b08bb59..404efcb 100755
> --- a/isar-init-build-env
> +++ b/isar-init-build-env
> @@ -42,13 +42,10 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" ]; then
>       exit 1
>   fi
>   
> -if [ -z "$ISARROOT" ]; then
> -    ISARROOT=$(dirname "$THIS_SCRIPT")
> -    ISARROOT=$(readlink -f "$ISARROOT")
> -fi
> +ISARROOT=$(dirname "$THIS_SCRIPT")
> +ISARROOT=$(readlink -f "$ISARROOT")
>   unset THIS_SCRIPT
>   
> -ISARROOT=$(readlink -f "$ISARROOT")
>   export ISARROOT
>   . "$ISARROOT/scripts/isar-buildenv-internal" "$1" && \
>       TEMPLATECONF="$TEMPLATECONF" "$ISARROOT/scripts/isar-setup-builddir" || {
> 

OE is doing the same, i.e. does not update OEROOT on second execution. This 
allows to set OEROOT prior to calling the script 
(https://github.com/openembedded/openembedded-core/commit/3327e2a9222004d8ac7974cb1d9fe77c81176cfc).

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[Vijai Kumar K]

On Mon, Sep 30, 2019 at 08:06:14AM +0200, Jan Kiszka wrote:
> On 27.09.19 23:43, vijaikumar.kanagarajan@gmail.com wrote:
> > From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > 
> > When switching between two ISAR workspaces in the same shell
> > session, the ISARROOT setting of the previous workspace would be
> > picked up for the new workspace resulting in an incorrect
> > configuration.
> > 
> > Always set ISARROOT irrespective of whether it is empty or not.
> > 
> > Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > ---
> > Changes in v2:
> >      - Remove redundant code
> > 
> >   isar-init-build-env | 7 ++-----
> >   1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/isar-init-build-env b/isar-init-build-env
> > index b08bb59..404efcb 100755
> > --- a/isar-init-build-env
> > +++ b/isar-init-build-env
> > @@ -42,13 +42,10 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" ]; then
> >       exit 1
> >   fi
> > -if [ -z "$ISARROOT" ]; then
> > -    ISARROOT=$(dirname "$THIS_SCRIPT")
> > -    ISARROOT=$(readlink -f "$ISARROOT")
> > -fi
> > +ISARROOT=$(dirname "$THIS_SCRIPT")
> > +ISARROOT=$(readlink -f "$ISARROOT")
> >   unset THIS_SCRIPT
> > -ISARROOT=$(readlink -f "$ISARROOT")
> >   export ISARROOT
> >   . "$ISARROOT/scripts/isar-buildenv-internal" "$1" && \
> >       TEMPLATECONF="$TEMPLATECONF" "$ISARROOT/scripts/isar-setup-builddir" || {
> > 
> 
> OE is doing the same, i.e. does not update OEROOT on second execution. This
> allows to set OEROOT prior to calling the script (https://github.com/openembedded/openembedded-core/commit/3327e2a9222004d8ac7974cb1d9fe77c81176cfc).

Yes. But then OE also unsets OEROOT later in that script.

But with our below change we are no longer doing the same.
https://github.com/ilbers/isar/commit/b59cf9ef8aae682937e8a4a5cda1c6d080d329b7

Thanks,
Vijai Kumar K

> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[Jan Kiszka]

On 30.09.19 08:46, Vijai Kumar K wrote:
> On Mon, Sep 30, 2019 at 08:06:14AM +0200, Jan Kiszka wrote:
>> On 27.09.19 23:43, vijaikumar.kanagarajan@gmail.com wrote:
>>> From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
>>>
>>> When switching between two ISAR workspaces in the same shell
>>> session, the ISARROOT setting of the previous workspace would be
>>> picked up for the new workspace resulting in an incorrect
>>> configuration.
>>>
>>> Always set ISARROOT irrespective of whether it is empty or not.
>>>
>>> Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
>>> ---
>>> Changes in v2:
>>>       - Remove redundant code
>>>
>>>    isar-init-build-env | 7 ++-----
>>>    1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/isar-init-build-env b/isar-init-build-env
>>> index b08bb59..404efcb 100755
>>> --- a/isar-init-build-env
>>> +++ b/isar-init-build-env
>>> @@ -42,13 +42,10 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" ]; then
>>>        exit 1
>>>    fi
>>> -if [ -z "$ISARROOT" ]; then
>>> -    ISARROOT=$(dirname "$THIS_SCRIPT")
>>> -    ISARROOT=$(readlink -f "$ISARROOT")
>>> -fi
>>> +ISARROOT=$(dirname "$THIS_SCRIPT")
>>> +ISARROOT=$(readlink -f "$ISARROOT")
>>>    unset THIS_SCRIPT
>>> -ISARROOT=$(readlink -f "$ISARROOT")
>>>    export ISARROOT
>>>    . "$ISARROOT/scripts/isar-buildenv-internal" "$1" && \
>>>        TEMPLATECONF="$TEMPLATECONF" "$ISARROOT/scripts/isar-setup-builddir" || {
>>>
>>
>> OE is doing the same, i.e. does not update OEROOT on second execution. This
>> allows to set OEROOT prior to calling the script (https://github.com/openembedded/openembedded-core/commit/3327e2a9222004d8ac7974cb1d9fe77c81176cfc).
> 
> Yes. But then OE also unsets OEROOT later in that script.
> 
> But with our below change we are no longer doing the same.
> https://github.com/ilbers/isar/commit/b59cf9ef8aae682937e8a4a5cda1c6d080d329b7
> 

But this is not changing the fact that we can current take ISARROOT from the 
caller's environment. If we remove that, in deviation from OE, it should be a 
clear decision. And it should be stated somewhere.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[Vijai Kumar K]

On Mon, Sep 30, 2019 at 09:01:57AM +0200, Jan Kiszka wrote:
> On 30.09.19 08:46, Vijai Kumar K wrote:
> > On Mon, Sep 30, 2019 at 08:06:14AM +0200, Jan Kiszka wrote:
> > > On 27.09.19 23:43, vijaikumar.kanagarajan@gmail.com wrote:
> > > > From: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > > > 
> > > > When switching between two ISAR workspaces in the same shell
> > > > session, the ISARROOT setting of the previous workspace would be
> > > > picked up for the new workspace resulting in an incorrect
> > > > configuration.
> > > > 
> > > > Always set ISARROOT irrespective of whether it is empty or not.
> > > > 
> > > > Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > > > ---
> > > > Changes in v2:
> > > >       - Remove redundant code
> > > > 
> > > >    isar-init-build-env | 7 ++-----
> > > >    1 file changed, 2 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/isar-init-build-env b/isar-init-build-env
> > > > index b08bb59..404efcb 100755
> > > > --- a/isar-init-build-env
> > > > +++ b/isar-init-build-env
> > > > @@ -42,13 +42,10 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" ]; then
> > > >        exit 1
> > > >    fi
> > > > -if [ -z "$ISARROOT" ]; then
> > > > -    ISARROOT=$(dirname "$THIS_SCRIPT")
> > > > -    ISARROOT=$(readlink -f "$ISARROOT")
> > > > -fi
> > > > +ISARROOT=$(dirname "$THIS_SCRIPT")
> > > > +ISARROOT=$(readlink -f "$ISARROOT")
> > > >    unset THIS_SCRIPT
> > > > -ISARROOT=$(readlink -f "$ISARROOT")
> > > >    export ISARROOT
> > > >    . "$ISARROOT/scripts/isar-buildenv-internal" "$1" && \
> > > >        TEMPLATECONF="$TEMPLATECONF" "$ISARROOT/scripts/isar-setup-builddir" || {
> > > > 
> > > 
> > > OE is doing the same, i.e. does not update OEROOT on second execution. This
> > > allows to set OEROOT prior to calling the script (https://github.com/openembedded/openembedded-core/commit/3327e2a9222004d8ac7974cb1d9fe77c81176cfc).
> > 
> > Yes. But then OE also unsets OEROOT later in that script.
> > 
> > But with our below change we are no longer doing the same.
> > https://github.com/ilbers/isar/commit/b59cf9ef8aae682937e8a4a5cda1c6d080d329b7
> > 
> 
> But this is not changing the fact that we can current take ISARROOT from the
> caller's environment. If we remove that, in deviation from OE, it should be
> a clear decision. And it should be stated somewhere.
> 
> Jan

Yes. You are right. I would rather not break that. The first thing that comes
to my mind to fix this is to use a different variable in ./meta/classes/wic-img.bbclass
and leave ISARROOT alone. Something like ISARHOME which would obviously be the value of
ISARROOT. Then we could unset ISARROOT at the end of the script. 

Thanks,
Vijai Kumar K

> 
> -- 
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[vijai kumar]

[-- Attachment #1.1: Type: text/plain, Size: 3260 bytes --]

Ok. On a recent thought, Things would go wrong only when someone moves the 
isar-init-build-env and writes
a wrapper script in the same location. In that case, one would need to pass 
in the ISARROOT from the caller to isar-init-build-env.

Right now I dont see anyone moving this thing. The wrapper script could 
very well be in the same location
as in isar-init-build-env, unless you want the wrapper to be in the same 
name for some reason(like for kas??).

It is not a lot of code, people could modify the isar-init-build-env 
directly instead of writing a wrapper in the same name
and moving the original one.

The issue we have in hand is more critical than this one feature, which 
could very well be written in other ways.

Thanks,
Vijai Kumar K

On Monday, September 30, 2019 at 12:32:00 PM UTC+5:30, Jan Kiszka wrote:
>
> On 30.09.19 08:46, Vijai Kumar K wrote: 
> > On Mon, Sep 30, 2019 at 08:06:14AM +0200, Jan Kiszka wrote: 
> >> On 27.09.19 23:43, vijaikumar...@gmail.com <javascript:> wrote: 
> >>> From: Vijai Kumar K <Vijaikumar_...@mentor.com <javascript:>> 
> >>> 
> >>> When switching between two ISAR workspaces in the same shell 
> >>> session, the ISARROOT setting of the previous workspace would be 
> >>> picked up for the new workspace resulting in an incorrect 
> >>> configuration. 
> >>> 
> >>> Always set ISARROOT irrespective of whether it is empty or not. 
> >>> 
> >>> Signed-off-by: Vijai Kumar K <Vijaikumar_...@mentor.com <javascript:>> 
>
> >>> --- 
> >>> Changes in v2: 
> >>>       - Remove redundant code 
> >>> 
> >>>    isar-init-build-env | 7 ++----- 
> >>>    1 file changed, 2 insertions(+), 5 deletions(-) 
> >>> 
> >>> diff --git a/isar-init-build-env b/isar-init-build-env 
> >>> index b08bb59..404efcb 100755 
> >>> --- a/isar-init-build-env 
> >>> +++ b/isar-init-build-env 
> >>> @@ -42,13 +42,10 @@ if [ -z "$ZSH_NAME" ] && [ "$0" = "$THIS_SCRIPT" 
> ]; then 
> >>>        exit 1 
> >>>    fi 
> >>> -if [ -z "$ISARROOT" ]; then 
> >>> -    ISARROOT=$(dirname "$THIS_SCRIPT") 
> >>> -    ISARROOT=$(readlink -f "$ISARROOT") 
> >>> -fi 
> >>> +ISARROOT=$(dirname "$THIS_SCRIPT") 
> >>> +ISARROOT=$(readlink -f "$ISARROOT") 
> >>>    unset THIS_SCRIPT 
> >>> -ISARROOT=$(readlink -f "$ISARROOT") 
> >>>    export ISARROOT 
> >>>    . "$ISARROOT/scripts/isar-buildenv-internal" "$1" && \ 
> >>>        TEMPLATECONF="$TEMPLATECONF" 
> "$ISARROOT/scripts/isar-setup-builddir" || { 
> >>> 
> >> 
> >> OE is doing the same, i.e. does not update OEROOT on second execution. 
> This 
> >> allows to set OEROOT prior to calling the script (
> https://github.com/openembedded/openembedded-core/commit/3327e2a9222004d8ac7974cb1d9fe77c81176cfc). 
>
> > 
> > Yes. But then OE also unsets OEROOT later in that script. 
> > 
> > But with our below change we are no longer doing the same. 
> > 
> https://github.com/ilbers/isar/commit/b59cf9ef8aae682937e8a4a5cda1c6d080d329b7 
> > 
>
> But this is not changing the fact that we can current take ISARROOT from 
> the 
> caller's environment. If we remove that, in deviation from OE, it should 
> be a 
> clear decision. And it should be stated somewhere. 
>
> Jan 
>
> -- 
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE 
> Corporate Competence Center Embedded Linux 
>

[-- Attachment #1.2: Type: text/html, Size: 6138 bytes --]

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[Baurzhan Ismagulov]

On Wed, Dec 11, 2019 at 02:14:03AM -0800, vijai kumar wrote:
> Ok. On a recent thought, Things would go wrong only when someone moves the 
> isar-init-build-env and writes
> a wrapper script in the same location. In that case, one would need to pass 
> in the ISARROOT from the caller to isar-init-build-env.

A pattern I see in Yocto downstream projects is the project directory
containing poky as subdirectory. The project wrapper script calls the poky one
from the subdirectory. I think the problem is that there is no reliable way to
determine ISARROOT under different shells, e.g.:

cd /home/ibr/myprj
. myprj-init
    . isar/isar-init-build-env

The desired value is /home/ibr/myprj/isar. This works with shell extensions
under bash and zsh, but not with dash, thus the ugly
THIS_SCRIPT="$(pwd)/isar-init-build-env" (which also doesn't work for the
scenario above). Dash is the default in Debian to speed up the boot process.


> The issue we have in hand is more critical than this one feature, which 
> could very well be written in other ways.

I agree this is an important issue. My suggestion:

* Keep accepting the preset ISARROOT from the environment.

* Remove ISARROOT from the wic scripts.

* Restore unsetting ISARROOT.


With kind regards,
Baurzhan.

Re: [PATCH v2] isar-init-build-env: Always set ISARROOT

[vijai kumar]

[-- Attachment #1.1: Type: text/plain, Size: 1692 bytes --]



On Tuesday, December 17, 2019 at 5:54:03 PM UTC+5:30, Baurzhan Ismagulov 
wrote:
>
> On Wed, Dec 11, 2019 at 02:14:03AM -0800, vijai kumar wrote: 
> > Ok. On a recent thought, Things would go wrong only when someone moves 
> the 
> > isar-init-build-env and writes 
> > a wrapper script in the same location. In that case, one would need to 
> pass 
> > in the ISARROOT from the caller to isar-init-build-env. 
>
> A pattern I see in Yocto downstream projects is the project directory 
> containing poky as subdirectory. The project wrapper script calls the poky 
> one 
> from the subdirectory. I think the problem is that there is no reliable 
> way to 
> determine ISARROOT under different shells, e.g.: 
>
> cd /home/ibr/myprj 
> . myprj-init 
>     . isar/isar-init-build-env 
>
> The desired value is /home/ibr/myprj/isar. This works with shell 
> extensions 
> under bash and zsh, but not with dash, thus the ugly 
> THIS_SCRIPT="$(pwd)/isar-init-build-env" (which also doesn't work for the 
> scenario above). Dash is the default in Debian to speed up the boot 
> process. 
>
>  
Ah, I see.


> > The issue we have in hand is more critical than this one feature, which 
> > could very well be written in other ways. 
>
> I agree this is an important issue. My suggestion: 
>
> * Keep accepting the preset ISARROOT from the environment. 
>
> * Remove ISARROOT from the wic scripts. 
>
> * Restore unsetting ISARROOT. 
>
>
Agreed. I think its better remove ISARROOT references now before it gets 
used in multiple places.
Right now, I think its mostly in wic-img and ci-build. I will keep this in 
my todo-list pipeline.

Thanks,
Vijai Kumar K



> With kind regards, 
> Baurzhan. 
>

[-- Attachment #1.2: Type: text/html, Size: 2357 bytes --]