From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 6741467829768290304 Date: Wed, 11 Dec 2019 02:14:03 -0800 (PST) From: vijai kumar To: isar-users Message-Id: <556d1edf-1533-4c30-b104-58e7f8235325@googlegroups.com> In-Reply-To: References: <20190927213804.31651-1-Vijaikumar_Kangarajan@mentor.com> <20190927214352.32114-1-Vijaikumar_Kangarajan@mentor.com> <20190930064655.GA10223@lightning> Subject: Re: [PATCH v2] isar-init-build-env: Always set ISARROOT MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_Part_5162_380026302.1576059244029" X-Google-Token: EOuCw-8F55UJIoo27200 X-Google-IP: 139.181.36.34 X-TUID: ojMC3qS5f9aZ ------=_Part_5162_380026302.1576059244029 Content-Type: multipart/alternative; boundary="----=_Part_5163_2015863263.1576059244030" ------=_Part_5163_2015863263.1576059244030 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 wrote: > >>> From: Vijai Kumar K > > >>> > >>> 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 > > > >>> --- > >>> 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 > ------=_Part_5163_2015863263.1576059244030 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Ok. On a recent thought, Things would go wrong only w= hen someone moves the isar-init-build-env and writes
a wrapper sc= ript in the same location. In that case, one would need to pass in the ISAR= ROOT from the caller to isar-init-build-env.

Right= now I dont see anyone moving this thing. The wrapper script could very wel= l be in the same location
as in isar-init-build-env, unless you w= ant 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 v= ery well be written in other ways.

Thanks,
Vijai Kumar K

On Monday, September 30, 2019 at 1= 2: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 wrote:
>>> From: Vijai Kumar K <Vijaikumar_...@mentor.com>
>>>
>>> When switching between two ISAR workspaces in the same she= ll
>>> session, the ISARROOT setting of the previous workspace wo= uld 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= >
>>> ---
>>> Changes in v2:
>>> =C2=A0 =C2=A0 =C2=A0 - Remove redundant code
>>>
>>> =C2=A0 =C2=A0isar-init-build-env | 7 ++-----
>>> =C2=A0 =C2=A01 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" ] &&= amp; [ "$0" =3D "$THIS_SCRIPT" ]; then
>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0exit 1
>>> =C2=A0 =C2=A0fi
>>> -if [ -z "$ISARROOT" ]; then
>>> - =C2=A0 =C2=A0ISARROOT=3D$(dirname "$THIS_SCRIPT&quo= t;)
>>> - =C2=A0 =C2=A0ISARROOT=3D$(readlink -f "$ISARROOT&qu= ot;)
>>> -fi
>>> +ISARROOT=3D$(dirname "$THIS_SCRIPT")
>>> +ISARROOT=3D$(readlink -f "$ISARROOT")
>>> =C2=A0 =C2=A0unset THIS_SCRIPT
>>> -ISARROOT=3D$(readlink -f "$ISARROOT")
>>> =C2=A0 =C2=A0export ISARROOT
>>> =C2=A0 =C2=A0. "$ISARROOT/scripts/isar-buildenv-= internal" "$1" && \
>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0TEMPLATECONF=3D"$TEMPLATEC= ONF" "$ISARROOT/scripts/isar-setup-builddir" || {
>>>
>>
>> OE is doing the same, i.e. does not update OEROOT on second ex= ecution. This
>> allows to set OEROOT prior to calling the script (https://github.com= /openembedded/openembedded-core/commit/3327e2a9222004d8ac797= 4cb1d9fe77c81176cfc).
>=20
> Yes. But then OE also unsets OEROOT later in that script.
>=20
> But with our below change we are no longer doing the same.
> https://github.com/ilbers/isar/commit/b59cf9= ef8aae682937e8a4a5cda1c6d080d329b7
>=20

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

Jan

--=20
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
------=_Part_5163_2015863263.1576059244030-- ------=_Part_5162_380026302.1576059244029--