Re: [PATCH 2/2] autogen.sh: Detect python

[Petr Vorel <pvorel@suse.cz>]

Hi Daniel,

sorry for longer time to reply (vacation).

> On Fri, Aug 06, 2021 at 08:45:08AM +0200, Petr Vorel wrote:
> > It help to avoid error on distros which has only python3 binary:
> > ./autogen.sh: line 20: python: command not found

> > Using bash builtin 'command -v' to avoid requiring which as extra
> > dependency (usable on containers).
command -v is supported in other common shells (busybox sh and dash),
I'll add it to the commit message.
IMHO it's non-POSIX extension, but because it's support we use it in LTP shell
API, where we expect very minimal shell tools (i.e. no bash, no core utils).

> It looks the bash dependency is not specified in the INSTALL file in
> "The Requirements" section. May I ask you to add it?
Due previous and the fact shebang in various scripts (bootstrap, geninit.sh,
docs/mdate-sh, grub-core/genemuinit.sh, ...) is /bin/sh and checkbashisms does
not complain it would be wrong to list bash as a dependency. IMHO Debian build
tools which use dash do not need any special patch (Colin Watson would know),
also Alpine which uses busybox sh does need any patch either.


> > Keep the possibility to define PYTHON.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  autogen.sh | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)

> > diff --git a/autogen.sh b/autogen.sh
> > index 31b0ced7e..46f9e1a6d 100755
> > --- a/autogen.sh
> > +++ b/autogen.sh
> > @@ -7,8 +7,21 @@ if [ ! -e grub-core/lib/gnulib/stdlib.in.h ]; then
> >    exit 1
> >  fi

> > -# Set ${PYTHON} to plain 'python' if not set already
> > -: ${PYTHON:=python}
> > +# Detect python
> > +if [ -z "$PYTHON" ]; then
> > +	for i in python python3 python2; do

> May I ask you to use (multiple of) 2 space indention as it is done in
> most of this file?
Sure, sorry to overlook it.

> > +		if command -v "$i" > /dev/null 2>&1; then

> Ditto and below please...
Sure.

> > +			PYTHON="$i"
> > +			echo "Using $PYTHON" >&2

> Please drop ">&2" redirection here.
IMHO there is no useful value for user to see "python not found", but no problem
to drop it.

> And I think it should be "Using $PYTHON...".
Sure.

> > +			break
> > +		fi
> > +	done
> > +
> > +	if [ -z "$PYTHON" ]; then
> > +		echo "python not found" >&2

> s/found/found./
Sure.

Kind regards,
Petr

> > +		exit 1
> > +	fi
> > +fi

> Daniel