[PATCH] create-pull-request: cd to relative directory

[PATCH] create-pull-request: cd to relative directory

[Ed Bartosh]

create-pull-request -d path creates empty patches if directory
is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
It behaves expected way only if script is run with -d bitbake, i.e.
relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running
git format-patch in it with --relative, without specifying
relative path as a parameter.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/create-pull-request | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/create-pull-request b/scripts/create-pull-request
index 216edfd..7eac618 100755
--- a/scripts/create-pull-request
+++ b/scripts/create-pull-request
@@ -177,12 +177,15 @@ mkdir $ODIR
 
 if [ -n "$RELDIR" ]; then
 	ODIR=$(realpath $ODIR)
-	extraopts="--relative=$RELDIR"
+	pushd $RELDIR
+	extraopts="--relative"
 fi
 
 # Generate the patches and cover letter
 git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
 
+[ -n "$RELDIR" ] && popd
+
 # Customize the cover letter
 CL="$ODIR/0000-cover-letter.patch"
 PM="$ODIR/pull-msg"
-- 
2.1.4

Re: [PATCH] create-pull-request: cd to relative directory

[Khem Raj]

On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> create-pull-request -d path creates empty patches if directory
> is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
> It behaves expected way only if script is run with -d bitbake, i.e.
> relative dir name doesn't contain '\'.
>
> Fixed this unwanted behaviour by changing directory and running
> git format-patch in it with --relative, without specifying
> relative path as a parameter.
>
> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> ---
>  scripts/create-pull-request | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/create-pull-request b/scripts/create-pull-request
> index 216edfd..7eac618 100755
> --- a/scripts/create-pull-request
> +++ b/scripts/create-pull-request
> @@ -177,12 +177,15 @@ mkdir $ODIR
>
>  if [ -n "$RELDIR" ]; then
>         ODIR=$(realpath $ODIR)
> -       extraopts="--relative=$RELDIR"
> +       pushd $RELDIR

can we avoid using pushd so it works with non bash shells too ?

> +       extraopts="--relative"
>  fi
>
>  # Generate the patches and cover letter
>  git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
>
> +[ -n "$RELDIR" ] && popd
> +
>  # Customize the cover letter
>  CL="$ODIR/0000-cover-letter.patch"
>  PM="$ODIR/pull-msg"
> --
> 2.1.4
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [PATCH] create-pull-request: cd to relative directory

[Paul Eggleton]

On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:
> On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <ed.bartosh@linux.intel.com> 
wrote:
> > create-pull-request -d path creates empty patches if directory
> > is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
> > It behaves expected way only if script is run with -d bitbake, i.e.
> > relative dir name doesn't contain '\'.
> > 
> > Fixed this unwanted behaviour by changing directory and running
> > git format-patch in it with --relative, without specifying
> > relative path as a parameter.
> > 
> > Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> > ---
> > 
> >  scripts/create-pull-request | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/create-pull-request b/scripts/create-pull-request
> > index 216edfd..7eac618 100755
> > --- a/scripts/create-pull-request
> > +++ b/scripts/create-pull-request
> > @@ -177,12 +177,15 @@ mkdir $ODIR
> > 
> >  if [ -n "$RELDIR" ]; then
> >  
> >         ODIR=$(realpath $ODIR)
> > 
> > -       extraopts="--relative=$RELDIR"
> > +       pushd $RELDIR
> 
> can we avoid using pushd so it works with non bash shells too ?

Should be possible, but it's worth mentioning that this script already starts 
with #!/bin/bash.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH] create-pull-request: cd to relative directory

[Ed Bartosh]

On Thu, Aug 06, 2015 at 09:38:16AM +0100, Paul Eggleton wrote:
> On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:
> > On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <ed.bartosh@linux.intel.com> 
> wrote:
> > > create-pull-request -d path creates empty patches if directory
> > > is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
> > > It behaves expected way only if script is run with -d bitbake, i.e.
> > > relative dir name doesn't contain '\'.
> > > 
> > > Fixed this unwanted behaviour by changing directory and running
> > > git format-patch in it with --relative, without specifying
> > > relative path as a parameter.
> > > 
> > > Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> > > ---
> > > 
> > >  scripts/create-pull-request | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/create-pull-request b/scripts/create-pull-request
> > > index 216edfd..7eac618 100755
> > > --- a/scripts/create-pull-request
> > > +++ b/scripts/create-pull-request
> > > @@ -177,12 +177,15 @@ mkdir $ODIR
> > > 
> > >  if [ -n "$RELDIR" ]; then
> > >  
> > >         ODIR=$(realpath $ODIR)
> > > 
> > > -       extraopts="--relative=$RELDIR"
> > > +       pushd $RELDIR
> > 
> > can we avoid using pushd so it works with non bash shells too ?
> 
> Should be possible, but it's worth mentioning that this script already starts 
> with #!/bin/bash.

That was the reason I used pushd/popd.

--
Regards,
Ed

Re: [PATCH] create-pull-request: cd to relative directory

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]


> On Aug 6, 2015, at 1:38 AM, Paul Eggleton <paul.eggleton@linux.intel.com> wrote:
> 
> On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:
>> On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <ed.bartosh@linux.intel.com>
> wrote:
>>> create-pull-request -d path creates empty patches if directory
>>> is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
>>> It behaves expected way only if script is run with -d bitbake, i.e.
>>> relative dir name doesn't contain '\'.
>>> 
>>> Fixed this unwanted behaviour by changing directory and running
>>> git format-patch in it with --relative, without specifying
>>> relative path as a parameter.
>>> 
>>> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
>>> ---
>>> 
>>> scripts/create-pull-request | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/scripts/create-pull-request b/scripts/create-pull-request
>>> index 216edfd..7eac618 100755
>>> --- a/scripts/create-pull-request
>>> +++ b/scripts/create-pull-request
>>> @@ -177,12 +177,15 @@ mkdir $ODIR
>>> 
>>> if [ -n "$RELDIR" ]; then
>>> 
>>>        ODIR=$(realpath $ODIR)
>>> 
>>> -       extraopts="--relative=$RELDIR"
>>> +       pushd $RELDIR
>> 
>> can we avoid using pushd so it works with non bash shells too ?
> 
> Should be possible, but it's worth mentioning that this script already starts
> with #!/bin/bash.

Does it have to be ?

> 
> Cheers,
> Paul
> 
> --
> 
> Paul Eggleton
> Intel Open Source Technology Centre


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 211 bytes --]

Re: [PATCH] create-pull-request: cd to relative directory

[Paul Eggleton]

On Thursday 06 August 2015 08:05:40 Khem Raj wrote:
> > On Aug 6, 2015, at 1:38 AM, Paul Eggleton <paul.eggleton@linux.intel.com>
> > wrote:> 
> > On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:
> >> On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <ed.bartosh@linux.intel.com>
> > 
> > wrote:
> >>> create-pull-request -d path creates empty patches if directory
> >>> is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
> >>> It behaves expected way only if script is run with -d bitbake, i.e.
> >>> relative dir name doesn't contain '\'.
> >>> 
> >>> Fixed this unwanted behaviour by changing directory and running
> >>> git format-patch in it with --relative, without specifying
> >>> relative path as a parameter.
> >>> 
> >>> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> >>> ---
> >>> 
> >>> scripts/create-pull-request | 5 ++++-
> >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/scripts/create-pull-request b/scripts/create-pull-request
> >>> index 216edfd..7eac618 100755
> >>> --- a/scripts/create-pull-request
> >>> +++ b/scripts/create-pull-request
> >>> @@ -177,12 +177,15 @@ mkdir $ODIR
> >>> 
> >>> if [ -n "$RELDIR" ]; then
> >>> 
> >>>        ODIR=$(realpath $ODIR)
> >>> 
> >>> -       extraopts="--relative=$RELDIR"
> >>> +       pushd $RELDIR
> >> 
> >> can we avoid using pushd so it works with non bash shells too ?
> > 
> > Should be possible, but it's worth mentioning that this script already
> > starts with #!/bin/bash.
> 
> Does it have to be ?

Are you volunteering to fix any other bashisms in the script? If so no ;)

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH] create-pull-request: cd to relative directory

[Khem Raj]

[-- Attachment #1: Type: text/plain, Size: 2157 bytes --]


> On Aug 6, 2015, at 8:16 AM, Paul Eggleton <paul.eggleton@linux.intel.com> wrote:
> 
> On Thursday 06 August 2015 08:05:40 Khem Raj wrote:
>>> On Aug 6, 2015, at 1:38 AM, Paul Eggleton <paul.eggleton@linux.intel.com>
>>> wrote:>
>>> On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:
>>>> On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <ed.bartosh@linux.intel.com>
>>> 
>>> wrote:
>>>>> create-pull-request -d path creates empty patches if directory
>>>>> is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
>>>>> It behaves expected way only if script is run with -d bitbake, i.e.
>>>>> relative dir name doesn't contain '\'.
>>>>> 
>>>>> Fixed this unwanted behaviour by changing directory and running
>>>>> git format-patch in it with --relative, without specifying
>>>>> relative path as a parameter.
>>>>> 
>>>>> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
>>>>> ---
>>>>> 
>>>>> scripts/create-pull-request | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/scripts/create-pull-request b/scripts/create-pull-request
>>>>> index 216edfd..7eac618 100755
>>>>> --- a/scripts/create-pull-request
>>>>> +++ b/scripts/create-pull-request
>>>>> @@ -177,12 +177,15 @@ mkdir $ODIR
>>>>> 
>>>>> if [ -n "$RELDIR" ]; then
>>>>> 
>>>>>       ODIR=$(realpath $ODIR)
>>>>> 
>>>>> -       extraopts="--relative=$RELDIR"
>>>>> +       pushd $RELDIR
>>>> 
>>>> can we avoid using pushd so it works with non bash shells too ?
>>> 
>>> Should be possible, but it's worth mentioning that this script already
>>> starts with #!/bin/bash.
>> 
>> Does it have to be ?
> 
> Are you volunteering to fix any other bashisms in the script? If so no ;)

looking at the script there seems to be none and checkbashism confirms it

checkbashisms scripts/create-pull-request
could not find any possible bashisms in bash script scripts/create-pull-request

so in effect this will be the first one. May be in your V2 you should remove /bin/bash from interpreter
as well.

> 
> Cheers,
> Paul
> 
> --
> 
> Paul Eggleton
> Intel Open Source Technology Centre


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 211 bytes --]

[PATCH v2] create-pull-request: cd to relative directory

[Ed Bartosh]

create-pull-request -d path creates empty patches if directory
is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
It behaves expected way only if script is run with -d bitbake, i.e.
relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running
git format-patch in it with --relative, without specifying
relative path as a parameter.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/create-pull-request | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/create-pull-request b/scripts/create-pull-request
index 216edfd..786fd1ed 100755
--- a/scripts/create-pull-request
+++ b/scripts/create-pull-request
@@ -177,12 +177,16 @@ mkdir $ODIR
 
 if [ -n "$RELDIR" ]; then
 	ODIR=$(realpath $ODIR)
-	extraopts="--relative=$RELDIR"
+	prevdir=$(pwd)
+	cd $RELDIR
+	extraopts="--relative"
 fi
 
 # Generate the patches and cover letter
 git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
 
+[ -n "$RELDIR" ] && cd $prevdir
+
 # Customize the cover letter
 CL="$ODIR/0000-cover-letter.patch"
 PM="$ODIR/pull-msg"
-- 
2.1.4

[PATCH v3] create-pull-request: cd to relative directory

[Ed Bartosh]

create-pull-request -d path creates empty patches if directory
is specified as a path, i.e. ./bitbake or ./bitbake/ or full path.
It behaves expected way only if script is run with -d bitbake, i.e.
relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running
git format-patch in it with --relative, without specifying
relative path as a parameter.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/create-pull-request | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/create-pull-request b/scripts/create-pull-request
index 216edfd..786fd1ed 100755
--- a/scripts/create-pull-request
+++ b/scripts/create-pull-request
@@ -177,12 +177,16 @@ mkdir $ODIR
 
 if [ -n "$RELDIR" ]; then
 	ODIR=$(realpath $ODIR)
-	extraopts="--relative=$RELDIR"
+	prevdir=$(pwd)
+	cd $RELDIR
+	extraopts="--relative"
 fi
 
 # Generate the patches and cover letter
 git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
 
+[ -n "$RELDIR" ] && cd $prevdir
+
 # Customize the cover letter
 CL="$ODIR/0000-cover-letter.patch"
 PM="$ODIR/pull-msg"
-- 
2.1.4

[PATCH v4] create-pull-request: cleanup bashisms

[Ed Bartosh]

Made create-pull-request POSIX compatible:
   - Replaced /bin/bash -> /bin/sh in shebang.
   - Replaced usage of pushd/popd with generic shell commands.
   - Tested on zsh and dash.

Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
---
 scripts/create-pull-request | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/scripts/create-pull-request b/scripts/create-pull-request
index be49379..19ba588 100755
--- a/scripts/create-pull-request
+++ b/scripts/create-pull-request
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
 #
 # Copyright (c) 2010-2013,  Intel Corporation.
 # All Rights Reserved
@@ -177,14 +177,15 @@ mkdir $ODIR
 
 if [ -n "$RELDIR" ]; then
 	ODIR=$(realpath $ODIR)
-	pushd $RELDIR > /dev/null
+	pdir=$(pwd)
+	cd $RELDIR
 	extraopts="--relative"
 fi
 
 # Generate the patches and cover letter
 git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
 
-[ -n "$RELDIR" ] && popd > /dev/null
+[ -n "$RELDIR" ] && cd $pdir
 
 # Customize the cover letter
 CL="$ODIR/0000-cover-letter.patch"
-- 
2.1.4

Re: [PATCH v4] create-pull-request: cleanup bashisms

[Khem Raj]

On Wed, Aug 12, 2015 at 2:33 AM, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> Made create-pull-request POSIX compatible:
>    - Replaced /bin/bash -> /bin/sh in shebang.
>    - Replaced usage of pushd/popd with generic shell commands.
>    - Tested on zsh and dash.

thanks for doing this.

>
> Signed-off-by: Ed Bartosh <ed.bartosh@linux.intel.com>
> ---
>  scripts/create-pull-request | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/create-pull-request b/scripts/create-pull-request
> index be49379..19ba588 100755
> --- a/scripts/create-pull-request
> +++ b/scripts/create-pull-request
> @@ -1,4 +1,4 @@
> -#!/bin/bash
> +#!/bin/sh
>  #
>  # Copyright (c) 2010-2013,  Intel Corporation.
>  # All Rights Reserved
> @@ -177,14 +177,15 @@ mkdir $ODIR
>
>  if [ -n "$RELDIR" ]; then
>         ODIR=$(realpath $ODIR)
> -       pushd $RELDIR > /dev/null
> +       pdir=$(pwd)
> +       cd $RELDIR
>         extraopts="--relative"
>  fi
>
>  # Generate the patches and cover letter
>  git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
>
> -[ -n "$RELDIR" ] && popd > /dev/null
> +[ -n "$RELDIR" ] && cd $pdir
>
>  # Customize the cover letter
>  CL="$ODIR/0000-cover-letter.patch"
> --
> 2.1.4
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Mikko.Rapeli]

Sorry to hijack the thread but...

On Wed, Aug 12, 2015 at 12:33:31PM +0300, Ed Bartosh wrote:
> Made create-pull-request POSIX compatible:
>    - Replaced /bin/bash -> /bin/sh in shebang.
>    - Replaced usage of pushd/popd with generic shell commands.
>    - Tested on zsh and dash.

This reminded me of the problems I've seen with various oe-core shell scripts:
they are missing systematic error handling.

IMO using bash and 'set -euxo pipefail' are a good approach to catch errors
early in shell scripts. Manually checking for $? is error prone and things
like pipes can hide them. Using undefined variables is another thing
which should fail early before things go horribly wrong. And in build automation
I'd rather see output of -x since things can fail in mysterious ways.
And then there's quoting....

One useful guide is to use shellcheck as static analysis tool for shell scripts.

I guess internally bitbake uses at least set -e.

Comments?

-Mikko

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Khem Raj]

On Wed, Aug 12, 2015 at 2:49 AM,  <Mikko.Rapeli@bmw.de> wrote:
> This reminded me of the problems I've seen with various oe-core shell scripts:
> they are missing systematic error handling.
>
> IMO using bash and 'set -euxo pipefail' are a good approach to catch errors
> early in shell scripts. Manually checking for $? is error prone and things
> like pipes can hide them. Using undefined variables is another thing
> which should fail early before things go horribly wrong. And in build automation
> I'd rather see output of -x since things can fail in mysterious ways.
> And then there's quoting....
>
> One useful guide is to use shellcheck as static analysis tool for shell scripts.
>
> I guess internally bitbake uses at least set -e.
>
> Comments?

all these are good suggestions, I would recommend to open bugs for all
of these some of them might need
to cite the scripts which need fixing.

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Wed, Aug 12, 2015 at 9:56 AM, Khem Raj <raj.khem@gmail.com> wrote:

> On Wed, Aug 12, 2015 at 2:49 AM,  <Mikko.Rapeli@bmw.de> wrote:
> > This reminded me of the problems I've seen with various oe-core shell
> scripts:
> > they are missing systematic error handling.
> >
> > IMO using bash and 'set -euxo pipefail' are a good approach to catch
> errors
> > early in shell scripts. Manually checking for $? is error prone and
> things
> > like pipes can hide them. Using undefined variables is another thing
> > which should fail early before things go horribly wrong. And in build
> automation
> > I'd rather see output of -x since things can fail in mysterious ways.
> > And then there's quoting....
> >
> > One useful guide is to use shellcheck as static analysis tool for shell
> scripts.
> >
> > I guess internally bitbake uses at least set -e.
> >
> > Comments?
>
> all these are good suggestions, I would recommend to open bugs for all
> of these some of them might need
> to cite the scripts which need fixing.


Agreed, it's important to check the behavior of shell scripts is kosher and
standards-compliant. shellcheck is extremely useful, I recommend it as
well, but combined with the vim syntastic plugin, so every time you save
the file all the errors and warnings are identified :) Makes it harder to
ignore or forget about.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2046 bytes --]

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

On Wed, Aug 12, 2015 at 10:46 AM, Christopher Larson <clarson@kergoth.com>
wrote:

> On Wed, Aug 12, 2015 at 9:56 AM, Khem Raj <raj.khem@gmail.com> wrote:
>
>> On Wed, Aug 12, 2015 at 2:49 AM,  <Mikko.Rapeli@bmw.de> wrote:
>> > This reminded me of the problems I've seen with various oe-core shell
>> scripts:
>> > they are missing systematic error handling.
>> >
>> > IMO using bash and 'set -euxo pipefail' are a good approach to catch
>> errors
>> > early in shell scripts. Manually checking for $? is error prone and
>> things
>> > like pipes can hide them. Using undefined variables is another thing
>> > which should fail early before things go horribly wrong. And in build
>> automation
>> > I'd rather see output of -x since things can fail in mysterious ways.
>> > And then there's quoting....
>> >
>> > One useful guide is to use shellcheck as static analysis tool for shell
>> scripts.
>> >
>> > I guess internally bitbake uses at least set -e.
>> >
>> > Comments?
>>
>> all these are good suggestions, I would recommend to open bugs for all
>> of these some of them might need
>> to cite the scripts which need fixing.
>
>
> Agreed, it's important to check the behavior of shell scripts is kosher
> and standards-compliant. shellcheck is extremely useful, I recommend it as
> well, but combined with the vim syntastic plugin, so every time you save
> the file all the errors and warnings are identified :) Makes it harder to
> ignore or forget about.


That reminds me, there's a shell portability issue / standards-complaince
issue that's not identified by shellcheck. Typically, to negate a  bracket
expression in a regular expression, one uses ^, e.g. [^a-z] is everything
that isn't in the range a to z, but that's not the case in shell, e.g. at a
prompt or in a ${foo#<pattern>}:

    "[...]a bracket expression as in XBD *RE Bracket Expression* , except
that the <exclamation-mark> character ( '!' ) shall replace the
<circumflex> character ( '^' ) in its role in a non-matching list in the
regular expression notation"

So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells handle
both, but the behavior of the latter is actually unspecified according to
the standard:

    "A bracket expression starting with an unquoted <circumflex> character
produces unspecified results."

I recently got bitten by this with one of my shell scripts on a system
running dash.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 3705 bytes --]

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Mikko.Rapeli]

On Wed, Aug 12, 2015 at 10:51:26AM -0700, Christopher Larson wrote:
> That reminds me, there's a shell portability issue / standards-complaince
> issue that's not identified by shellcheck. Typically, to negate a  bracket
> expression in a regular expression, one uses ^, e.g. [^a-z] is everything
> that isn't in the range a to z, but that's not the case in shell, e.g. at a
> prompt or in a ${foo#<pattern>}:
> 
>     "[...]a bracket expression as in XBD *RE Bracket Expression* , except
> that the <exclamation-mark> character ( '!' ) shall replace the
> <circumflex> character ( '^' ) in its role in a non-matching list in the
> regular expression notation"
> 
> So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells handle
> both, but the behavior of the latter is actually unspecified according to
> the standard:
> 
>     "A bracket expression starting with an unquoted <circumflex> character
> produces unspecified results."
> 
> I recently got bitten by this with one of my shell scripts on a system
> running dash.

Does checkbashisms warn about this?

-Mikko

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On Wed, Aug 12, 2015 at 11:27 PM, <Mikko.Rapeli@bmw.de> wrote:

> On Wed, Aug 12, 2015 at 10:51:26AM -0700, Christopher Larson wrote:
> > That reminds me, there's a shell portability issue / standards-complaince
> > issue that's not identified by shellcheck. Typically, to negate a
> bracket
> > expression in a regular expression, one uses ^, e.g. [^a-z] is everything
> > that isn't in the range a to z, but that's not the case in shell, e.g.
> at a
> > prompt or in a ${foo#<pattern>}:
> >
> >     "[...]a bracket expression as in XBD *RE Bracket Expression* , except
> > that the <exclamation-mark> character ( '!' ) shall replace the
> > <circumflex> character ( '^' ) in its role in a non-matching list in the
> > regular expression notation"
> >
> > So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells
> handle
> > both, but the behavior of the latter is actually unspecified according to
> > the standard:
> >
> >     "A bracket expression starting with an unquoted <circumflex>
> character
> > produces unspecified results."
> >
> > I recently got bitten by this with one of my shell scripts on a system
> > running dash.
>
> Does checkbashisms warn about this?


Finally got around to checking, and yeah, checkbashisms does spot this one,
it's just shellcheck that doesn't (yet, opened a bug upstream).

-Chris


-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2193 bytes --]

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

On Fri, Aug 14, 2015 at 3:01 PM, Christopher Larson <clarson@kergoth.com>
wrote:

> On Wed, Aug 12, 2015 at 11:27 PM, <Mikko.Rapeli@bmw.de> wrote:
>
>> On Wed, Aug 12, 2015 at 10:51:26AM -0700, Christopher Larson wrote:
>> > That reminds me, there's a shell portability issue /
>> standards-complaince
>> > issue that's not identified by shellcheck. Typically, to negate a
>> bracket
>> > expression in a regular expression, one uses ^, e.g. [^a-z] is
>> everything
>> > that isn't in the range a to z, but that's not the case in shell, e.g.
>> at a
>> > prompt or in a ${foo#<pattern>}:
>> >
>> >     "[...]a bracket expression as in XBD *RE Bracket Expression* ,
>> except
>> > that the <exclamation-mark> character ( '!' ) shall replace the
>> > <circumflex> character ( '^' ) in its role in a non-matching list in the
>> > regular expression notation"
>> >
>> > So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells
>> handle
>> > both, but the behavior of the latter is actually unspecified according
>> to
>> > the standard:
>> >
>> >     "A bracket expression starting with an unquoted <circumflex>
>> character
>> > produces unspecified results."
>> >
>> > I recently got bitten by this with one of my shell scripts on a system
>> > running dash.
>>
>> Does checkbashisms warn about this?
>
>
> Finally got around to checking, and yeah, checkbashisms does spot this
> one, it's just shellcheck that doesn't (yet, opened a bug upstream).


Update: checkbashisms and shellcheck both check for this now.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2475 bytes --]

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Mikko.Rapeli]

On Sat, Aug 15, 2015 at 06:38:18PM -0700, Christopher Larson wrote:
> Update: checkbashisms and shellcheck both check for this now.

This is great! How about formulating that oe-core shell scripts should
pass shellcheck and checkbashisms checks without warnings?

-Mikko

Re: shell script guidelines in oe-core? (was Re: [PATCH v4] create-pull-request: cleanup bashisms)

[Otavio Salvador]

On Mon, Aug 17, 2015 at 3:14 AM,  <Mikko.Rapeli@bmw.de> wrote:
> On Sat, Aug 15, 2015 at 06:38:18PM -0700, Christopher Larson wrote:
>> Update: checkbashisms and shellcheck both check for this now.
>
> This is great! How about formulating that oe-core shell scripts should
> pass shellcheck and checkbashisms checks without warnings?

I second this; the more picky we are now, less bashism we have to handle later.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750