[PATCH 1/2] bootstrap: Require patch

[PATCH 1/2] bootstrap: Require patch

[Petr Vorel]

bootstrap.conf uses patch, let's require it.

Better than multiple messages:
./bootstrap.conf: line 84: patch: command not found

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 bootstrap.conf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/bootstrap.conf b/bootstrap.conf
index 6b043fc35..0dd893c5c 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -70,6 +70,7 @@ autoconf   2.63
 automake   1.11
 gettext    0.18.3
 git        1.5.5
+patch      -
 tar        -
 "
 
-- 
2.32.0

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

[Petr Vorel]

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).

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
+		if command -v "$i" > /dev/null 2>&1; then
+			PYTHON="$i"
+			echo "Using $PYTHON" >&2
+			break
+		fi
+	done
+
+	if [ -z "$PYTHON" ]; then
+		echo "python not found" >&2
+		exit 1
+	fi
+fi
 
 export LC_COLLATE=C
 unset LC_ALL
-- 
2.32.0

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

[Daniel Kiper]

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).

It looks the bash dependency is not specified in the INSTALL file in
"The Requirements" section. May I ask you to add it?

> 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?

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

Ditto and below please...

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

Please drop ">&2" redirection here.
And I think it should be "Using $PYTHON...".

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

s/found/found./

> +		exit 1
> +	fi
> +fi

Daniel

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

[Daniel Kiper]

On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote:
> 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).
>
> It looks the bash dependency is not specified in the INSTALL file in
> "The Requirements" section. May I ask you to add it?

...and python requirement is missing from the list too. Additionally,
please update autogen.sh python usage in the INSTALL file.

Last but not least, patch tool requirement is missing from INSTALL file too.
Of course this requires update to patch #1 in this series.

Daniel

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

[Petr Vorel]

> On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote:
> > 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).

> > It looks the bash dependency is not specified in the INSTALL file in
> > "The Requirements" section. May I ask you to add it?

> ...and python requirement is missing from the list too. Additionally,
> please update autogen.sh python usage in the INSTALL file.
There is Python 2.6 or later, not sure if it still works on 2.6 (which is EOL)
or it requires now 2.7).

Also "If you use a development snapshot or want to hack on GRUB you may
need the following." is obsolete (from 2009), I'll move python, autoconf and
automake to the list on the top.

> Last but not least, patch tool requirement is missing from INSTALL file too.
> Of course this requires update to patch #1 in this series.
I first thought "Other standard GNU/Unix tools" would cover it, but it does not
harm to mention it (for those who compile on containers which have minimal
dependencies).

Kind regards,
Petr

> Daniel

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

[Bruce Dubbs]

On 8/18/21 1:19 AM, Petr Vorel wrote:
>> On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote:
>>> 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).
> 
>>> It looks the bash dependency is not specified in the INSTALL file in
>>> "The Requirements" section. May I ask you to add it?
> 
>> ...and python requirement is missing from the list too. Additionally,
>> please update autogen.sh python usage in the INSTALL file.
> There is Python 2.6 or later, not sure if it still works on 2.6 (which is EOL)
> or it requires now 2.7).

Actually all of Python 2 is EOL for the last year and a half.

https://www.python.org/doc/sunset-python-2/

   -- Bruce

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

[Daniel Kiper]

On Wed, Aug 18, 2021 at 11:04:50AM -0500, Bruce Dubbs wrote:
> On 8/18/21 1:19 AM, Petr Vorel wrote:
> > > On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote:
> > > > 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).
> >
> > > > It looks the bash dependency is not specified in the INSTALL file in
> > > > "The Requirements" section. May I ask you to add it?
> >
> > > ...and python requirement is missing from the list too. Additionally,
> > > please update autogen.sh python usage in the INSTALL file.
> > There is Python 2.6 or later, not sure if it still works on 2.6 (which is EOL)
> > or it requires now 2.7).
>
> Actually all of Python 2 is EOL for the last year and a half.
>
> https://www.python.org/doc/sunset-python-2/

Then we should drop python2 detection from the patch...

Daniel

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

[Daniel Kiper]

On Wed, Aug 18, 2021 at 09:19:23AM +0200, Petr Vorel wrote:
> > On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote:
> > > 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).
>
> > > It looks the bash dependency is not specified in the INSTALL file in
> > > "The Requirements" section. May I ask you to add it?
>
> > ...and python requirement is missing from the list too. Additionally,
> > please update autogen.sh python usage in the INSTALL file.
> There is Python 2.6 or later, not sure if it still works on 2.6 (which is EOL)
> or it requires now 2.7).

I think we should update this requirement to "Python 3.0.0 or later".

> Also "If you use a development snapshot or want to hack on GRUB you may
> need the following." is obsolete (from 2009), I'll move python, autoconf and
> automake to the list on the top.

OK...

There is also:

  3. Type `./bootstrap'.

     * autogen.sh (called by bootstrap) uses python. By default the
       invocation is "python", but it can be overridden by setting the
       variable $PYTHON.

I think this should be updated too.

> > Last but not least, patch tool requirement is missing from INSTALL file too.
> > Of course this requires update to patch #1 in this series.
> I first thought "Other standard GNU/Unix tools" would cover it, but it does not
> harm to mention it (for those who compile on containers which have minimal
> dependencies).

Yep!

Daniel

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

[Petr Vorel]

Hi Daniel,

> On Wed, Aug 18, 2021 at 09:19:23AM +0200, Petr Vorel wrote:
> > > On Mon, Aug 09, 2021 at 01:52:29PM +0200, Daniel Kiper wrote:
> > > > 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).

> > > > It looks the bash dependency is not specified in the INSTALL file in
> > > > "The Requirements" section. May I ask you to add it?

> > > ...and python requirement is missing from the list too. Additionally,
> > > please update autogen.sh python usage in the INSTALL file.
> > There is Python 2.6 or later, not sure if it still works on 2.6 (which is EOL)
> > or it requires now 2.7).

> I think we should update this requirement to "Python 3.0.0 or later".
I'll just note Python 3, because 1) IMHO it would not work on 3.0, first
reasonable python 3 version which allowed 2 and 3 reasonably easily coexist
together was IMHO 3.3, 2) no distro uses 3.0, they either use match higher 3
version (3.6 at least) or 2.7 (or 2.6 the oldest ones).

> > Also "If you use a development snapshot or want to hack on GRUB you may
> > need the following." is obsolete (from 2009), I'll move python, autoconf and
> > automake to the list on the top.

> OK...

> There is also:

>   3. Type `./bootstrap'.

>      * autogen.sh (called by bootstrap) uses python. By default the
>        invocation is "python", but it can be overridden by setting the
>        variable $PYTHON.

> I think this should be updated too.

+1.

Kind regards,
Petr

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

[Petr Vorel]

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

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

[Petr Vorel]

> 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).
Actually "command -v" [1] and "type" [2] are POSIX.

Kind regards,
Petr

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html
[2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/type.html

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

[Daniel Kiper]

On Wed, Aug 18, 2021 at 09:38:11AM +0200, Petr Vorel wrote:
> > 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).
> Actually "command -v" [1] and "type" [2] are POSIX.

OK, then ignore my comment WRT adding bash to the requirements
into the INSTALL file.

Daniel

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

[Dimitri John Ledkov]

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

Personally I would just change it to "set PYTHON to python3 if not set" and
that's it.

Python2 is irrelevant.

On Fri, 6 Aug 2021, 07:45 Petr Vorel, <pvorel@suse.cz> 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).
>
> 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
> +               if command -v "$i" > /dev/null 2>&1; then
> +                       PYTHON="$i"
> +                       echo "Using $PYTHON" >&2
> +                       break
> +               fi
> +       done
> +
> +       if [ -z "$PYTHON" ]; then
> +               echo "python not found" >&2
> +               exit 1
> +       fi
> +fi
>
>  export LC_COLLATE=C
>  unset LC_ALL
> --
> 2.32.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

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

[Petr Vorel]

Hi,

> Personally I would just change it to "set PYTHON to python3 if not set" and
> that's it.
Why bothering user to set environment variable when autodetection is possible?

> Python2 is irrelevant.
Fair enough for me to drop python 2 from autodetection. There probably aren't
many people who would like to use new grub in old distros. But again, when the
support is there, why to drop it?

Kind regards,
Petr

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

[Dimitri John Ledkov]

On Wed, Aug 18, 2021 at 6:51 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi,
>
> > Personally I would just change it to "set PYTHON to python3 if not set" and
> > that's it.
> Why bothering user to set environment variable when autodetection is possible?
>
> > Python2 is irrelevant.
> Fair enough for me to drop python 2 from autodetection. There probably aren't
> many people who would like to use new grub in old distros. But again, when the
> support is there, why to drop it?

10 year old distributions have python3.



--
Regards,

Dimitri.

Re: [PATCH 1/2] bootstrap: Require patch

[Daniel Kiper]

On Fri, Aug 06, 2021 at 08:45:07AM +0200, Petr Vorel wrote:
> bootstrap.conf uses patch, let's require it.
>
> Better than multiple messages:
> ./bootstrap.conf: line 84: patch: command not found
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel