[PATCH] scripts: dev-needs.sh: Enforce bash usage

[PATCH] scripts: dev-needs.sh: Enforce bash usage

[Steffen Kothe]

Calling the script from a system which does not invoke bash
by default causes a return with a syntax error like:

	./dev-needs.sh: 6: Syntax error: "(" unexpected

/bin/sh invokes on most distributions a symbolic link to a
default shell like dash (Debian) or bash (Ubuntu).

Since the script depends on bash syntax, enforce the same by
default to prevent syntax errors caused by wrong shell type usage.

Signed-off-by: Steffen Kothe <steffen.kothe@skothe.de>
---
 scripts/dev-needs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/dev-needs.sh b/scripts/dev-needs.sh
index 454cc304fb448..46537859727bc 100755
--- a/scripts/dev-needs.sh
+++ b/scripts/dev-needs.sh
@@ -1,4 +1,4 @@
-#! /bin/sh
+#! /bin/bash
 # SPDX-License-Identifier: GPL-2.0
 # Copyright (c) 2020, Google LLC. All rights reserved.
 # Author: Saravana Kannan <saravanak@google.com>
--
2.30.2

Re: [PATCH] scripts: dev-needs.sh: Enforce bash usage

[Saravana Kannan]

On Wed, Nov 9, 2022 at 9:04 AM Steffen Kothe <steffen.kothe@skothe.de> wrote:
>
> Calling the script from a system which does not invoke bash
> by default causes a return with a syntax error like:
>
>         ./dev-needs.sh: 6: Syntax error: "(" unexpected
>
> /bin/sh invokes on most distributions a symbolic link to a
> default shell like dash (Debian) or bash (Ubuntu).
>
> Since the script depends on bash syntax, enforce the same by
> default to prevent syntax errors caused by wrong shell type usage.

I wrote this so that it can run on an Android target that runs toybox.
Sadly toybox doesn't like have /bin/bash. This will break my use case.
So I'll have to Nak this.

I'm open to other ideas though as I'd like to this to work in as many
cases as possible. Should we just add a wrapper that has /bin/bash and
then sources this file?

Also looks like multiple #! lines aren't supported by bash, so we
can't add multiple lines either.

-Saravana

>
> Signed-off-by: Steffen Kothe <steffen.kothe@skothe.de>
> ---
>  scripts/dev-needs.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/dev-needs.sh b/scripts/dev-needs.sh
> index 454cc304fb448..46537859727bc 100755
> --- a/scripts/dev-needs.sh
> +++ b/scripts/dev-needs.sh
> @@ -1,4 +1,4 @@
> -#! /bin/sh
> +#! /bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  # Copyright (c) 2020, Google LLC. All rights reserved.
>  # Author: Saravana Kannan <saravanak@google.com>
> --
> 2.30.2
>
>

Re: [PATCH] scripts: dev-needs.sh: Enforce bash usage

[Steffen Kothe]

Am Wed, Nov 09, 2022 at 02:58:15PM -0800 schrieb Saravana Kannan:
> On Wed, Nov 9, 2022 at 9:04 AM Steffen Kothe <steffen.kothe@skothe.de> wrote:
> >
> > Calling the script from a system which does not invoke bash
> > by default causes a return with a syntax error like:
> >
> >         ./dev-needs.sh: 6: Syntax error: "(" unexpected
> >
> > /bin/sh invokes on most distributions a symbolic link to a
> > default shell like dash (Debian) or bash (Ubuntu).
> >
> > Since the script depends on bash syntax, enforce the same by
> > default to prevent syntax errors caused by wrong shell type usage.
>
> I wrote this so that it can run on an Android target that runs toybox.
> Sadly toybox doesn't like have /bin/bash. This will break my use case.
> So I'll have to Nak this.

Ok, I see.

> I'm open to other ideas though as I'd like to this to work in as many
> cases as possible. Should we just add a wrapper that has /bin/bash and
> then sources this file?

I mean, we could leave at least a warning on top above the first
function via a simple echo.

Sth. like:

	echo "Warning: $0 is only tested for following shell variants
	      [toybox, bash]. Other shells might be not following the
	      specific syntax of this script."

	echo "Shell is: $(readlink /bin/sh)"

I would prevent to add another wrapper to it, because its fairly easy
to cp this file to a target. An embedded system with a pure POSIX compliant
shell will never be compatible to this kind of syntax.

What do you think about a more describing approach of the error instead
of handling it with some weird logic and wrapping?

Cheers,
	skothe

> Also looks like multiple #! lines aren't supported by bash, so we
> can't add multiple lines either.
>
> -Saravana
>
> >
> > Signed-off-by: Steffen Kothe <steffen.kothe@skothe.de>
> > ---
> >  scripts/dev-needs.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/dev-needs.sh b/scripts/dev-needs.sh
> > index 454cc304fb448..46537859727bc 100755
> > --- a/scripts/dev-needs.sh
> > +++ b/scripts/dev-needs.sh
> > @@ -1,4 +1,4 @@
> > -#! /bin/sh
> > +#! /bin/bash
> >  # SPDX-License-Identifier: GPL-2.0
> >  # Copyright (c) 2020, Google LLC. All rights reserved.
> >  # Author: Saravana Kannan <saravanak@google.com>
> > --
> > 2.30.2
> >
> >

--
Cheers,
	Steffen

Re: [PATCH] scripts: dev-needs.sh: Enforce bash usage

[Saravana Kannan]

On Thu, Nov 10, 2022 at 9:48 AM Steffen Kothe <steffen.kothe@skothe.de> wrote:
>
> Am Wed, Nov 09, 2022 at 02:58:15PM -0800 schrieb Saravana Kannan:
> > On Wed, Nov 9, 2022 at 9:04 AM Steffen Kothe <steffen.kothe@skothe.de> wrote:
> > >
> > > Calling the script from a system which does not invoke bash
> > > by default causes a return with a syntax error like:
> > >
> > >         ./dev-needs.sh: 6: Syntax error: "(" unexpected
> > >
> > > /bin/sh invokes on most distributions a symbolic link to a
> > > default shell like dash (Debian) or bash (Ubuntu).
> > >
> > > Since the script depends on bash syntax, enforce the same by
> > > default to prevent syntax errors caused by wrong shell type usage.
> >
> > I wrote this so that it can run on an Android target that runs toybox.
> > Sadly toybox doesn't like have /bin/bash. This will break my use case.
> > So I'll have to Nak this.
>
> Ok, I see.
>
> > I'm open to other ideas though as I'd like to this to work in as many
> > cases as possible. Should we just add a wrapper that has /bin/bash and
> > then sources this file?
>
> I mean, we could leave at least a warning on top above the first
> function via a simple echo.
>
> Sth. like:
>
>         echo "Warning: $0 is only tested for following shell variants
>               [toybox, bash]. Other shells might be not following the
>               specific syntax of this script."
>
>         echo "Shell is: $(readlink /bin/sh)"
>
> I would prevent to add another wrapper to it, because its fairly easy
> to cp this file to a target. An embedded system with a pure POSIX compliant
> shell will never be compatible to this kind of syntax.
>
> What do you think about a more describing approach of the error instead
> of handling it with some weird logic and wrapping?

Are you suggesting we check for the shell being toybox/bash and then
printing this? Always printing it isn't nice because it'll mess up all
the script that expect the output to be just what it is today.

But if you want to add an error check, I'm okay with that.

-Saravana

>
> Cheers,
>         skothe
>
> > Also looks like multiple #! lines aren't supported by bash, so we
> > can't add multiple lines either.
> >
> > -Saravana
> >
> > >
> > > Signed-off-by: Steffen Kothe <steffen.kothe@skothe.de>
> > > ---
> > >  scripts/dev-needs.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/dev-needs.sh b/scripts/dev-needs.sh
> > > index 454cc304fb448..46537859727bc 100755
> > > --- a/scripts/dev-needs.sh
> > > +++ b/scripts/dev-needs.sh
> > > @@ -1,4 +1,4 @@
> > > -#! /bin/sh
> > > +#! /bin/bash
> > >  # SPDX-License-Identifier: GPL-2.0
> > >  # Copyright (c) 2020, Google LLC. All rights reserved.
> > >  # Author: Saravana Kannan <saravanak@google.com>
> > > --
> > > 2.30.2
> > >
> > >
>
> --
> Cheers,
>         Steffen
>

Re: [PATCH] scripts: dev-needs.sh: Enforce bash usage

[Steffen Kothe]

Am Thu, Nov 10, 2022 at 11:32:12AM -0800 schrieb Saravana Kannan:
> On Thu, Nov 10, 2022 at 9:48 AM Steffen Kothe <steffen.kothe@skothe.de> wrote:
> >
> > Am Wed, Nov 09, 2022 at 02:58:15PM -0800 schrieb Saravana Kannan:
> > > On Wed, Nov 9, 2022 at 9:04 AM Steffen Kothe <steffen.kothe@skothe.de> wrote:
> > > >
> > > > Calling the script from a system which does not invoke bash
> > > > by default causes a return with a syntax error like:
> > > >
> > > >         ./dev-needs.sh: 6: Syntax error: "(" unexpected
> > > >
> > > > /bin/sh invokes on most distributions a symbolic link to a
> > > > default shell like dash (Debian) or bash (Ubuntu).
> > > >
> > > > Since the script depends on bash syntax, enforce the same by
> > > > default to prevent syntax errors caused by wrong shell type usage.
> > >
> > > I wrote this so that it can run on an Android target that runs toybox.
> > > Sadly toybox doesn't like have /bin/bash. This will break my use case.
> > > So I'll have to Nak this.
> >
> > Ok, I see.
> >
> > > I'm open to other ideas though as I'd like to this to work in as many
> > > cases as possible. Should we just add a wrapper that has /bin/bash and
> > > then sources this file?
> >
> > I mean, we could leave at least a warning on top above the first
> > function via a simple echo.
> >
> > Sth. like:
> >
> >         echo "Warning: $0 is only tested for following shell variants
> >               [toybox, bash]. Other shells might be not following the
> >               specific syntax of this script."
> >
> >         echo "Shell is: $(readlink /bin/sh)"
> >
> > I would prevent to add another wrapper to it, because its fairly easy
> > to cp this file to a target. An embedded system with a pure POSIX compliant
> > shell will never be compatible to this kind of syntax.
> >
> > What do you think about a more describing approach of the error instead
> > of handling it with some weird logic and wrapping?
>
> Are you suggesting we check for the shell being toybox/bash and then
> printing this? Always printing it isn't nice because it'll mess up all
> the script that expect the output to be just what it is today.
>
> But if you want to add an error check, I'm okay with that.

What about a re-write of the script in POSIX compliant style? This
should work then in every shell derivate without weird checks.

This would remove for example function declarations, arrays and so on,
targeting to the same printable output but with different logic in the
background.

> -Saravana
>
> >
> > Cheers,
> >         skothe
> >
> > > Also looks like multiple #! lines aren't supported by bash, so we
> > > can't add multiple lines either.
> > >
> > > -Saravana
> > >
> > > >
> > > > Signed-off-by: Steffen Kothe <steffen.kothe@skothe.de>
> > > > ---
> > > >  scripts/dev-needs.sh | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/scripts/dev-needs.sh b/scripts/dev-needs.sh
> > > > index 454cc304fb448..46537859727bc 100755
> > > > --- a/scripts/dev-needs.sh
> > > > +++ b/scripts/dev-needs.sh
> > > > @@ -1,4 +1,4 @@
> > > > -#! /bin/sh
> > > > +#! /bin/bash
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > >  # Copyright (c) 2020, Google LLC. All rights reserved.
> > > >  # Author: Saravana Kannan <saravanak@google.com>
> > > > --
> > > > 2.30.2
> > > >
> > > >
> >
> > --
> > Cheers,
> >         Steffen
> >

--
Cheers,
	Steffen

RE: [PATCH] scripts: dev-needs.sh: Enforce bash usage

[David Laight]

From: Steffen Kothe
> Sent: 12 November 2022 09:41
...
> What about a re-write of the script in POSIX compliant style? This
> should work then in every shell derivate without weird checks.
> 
> This would remove for example function declarations, arrays and so on,
> targeting to the same printable output but with different logic in the
> background.

A quick read:

- remove the word 'function'.
- remove all 'local', ensure names are unique.
- use 'printf '%s' "....." instead of echo -n.

That should be pretty near portable.
Some shells that don't object to the 'local' keyword don't
actually implement local variables!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] scripts: dev-needs.sh: Enforce bash usage

[Saravana Kannan]

On Mon, Nov 14, 2022 at 2:30 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Steffen Kothe
> > Sent: 12 November 2022 09:41
> ...
> > What about a re-write of the script in POSIX compliant style? This
> > should work then in every shell derivate without weird checks.
> >
> > This would remove for example function declarations, arrays and so on,
> > targeting to the same printable output but with different logic in the
> > background.
>
> A quick read:
>
> - remove the word 'function'.
> - remove all 'local', ensure names are unique.
> - use 'printf '%s' "....." instead of echo -n.
>
> That should be pretty near portable.
> Some shells that don't object to the 'local' keyword don't
> actually implement local variables!
>

Steffen/David,

I'll happily test and take any updates that's at least as readable as
what's there now, as long as it continues working with toybox.

-Saravana