[PATCH] autoconf: fix handling absolute $PYTHON path

[PATCH] autoconf: fix handling absolute $PYTHON path

[Marek Marczykowski-Górecki]

Don't strip full path from $PYTHON variable. This is especially
relevant, if it points outside of $PATH. This is the case
for RPM build on CentOS 8 (%{python3} macro points at
/usr/libexec/platform-python).

For this reason, adjust also python-config handling - AC_PATH_PROG
doesn't work on already absolute path, so make it conditional.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 m4/python_devel.m4 | 6 +++++-
 tools/configure.ac | 1 -
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/m4/python_devel.m4 b/m4/python_devel.m4
index bbf1e0354b2b..676489b8e978 100644
--- a/m4/python_devel.m4
+++ b/m4/python_devel.m4
@@ -2,7 +2,11 @@ AC_DEFUN([AX_CHECK_PYTHON_DEVEL], [
 ac_previous_cppflags=$CPPFLAGS
 ac_previous_ldflags=$LDFLAGS
 ac_previous_libs=$LIBS
-AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
+AS_IF([echo "$PYTHON" | grep -q "^/"], [
+    pyconfig="$PYTHON-config"
+], [
+    AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
+])
 AS_IF([test x"$pyconfig" = x"no"], [
     dnl For those that don't have python-config
     CPPFLAGS="$CFLAGS `$PYTHON -c 'import distutils.sysconfig; \
diff --git a/tools/configure.ac b/tools/configure.ac
index 6414fcbb445e..ebf1265643b3 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -368,7 +368,6 @@ AS_IF([test -z "$PYTHON"], [AC_CHECK_PROGS([PYTHON], [python3 python python2], e
 AS_IF([test "$PYTHON" = "err"], [AC_MSG_ERROR([No python interpreter found])])
 AS_IF([echo "$PYTHON" | grep -q "^/"], [], [AC_PATH_PROG([PYTHON], [$PYTHON])])
 PYTHONPATH=$PYTHON
-PYTHON=`basename $PYTHONPATH`
 
 AX_PATH_PROG_OR_FAIL([PYTHONPATH], [$PYTHON])
 AX_CHECK_PYTHON_VERSION([2], [6])
-- 
2.26.3

Re: [PATCH] autoconf: fix handling absolute $PYTHON path

[Marek Marczykowski-Górecki]

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

On Wed, Jun 02, 2021 at 05:32:06AM +0200, Marek Marczykowski-Górecki wrote:
> Don't strip full path from $PYTHON variable. This is especially
> relevant, if it points outside of $PATH. This is the case
> for RPM build on CentOS 8 (%{python3} macro points at
> /usr/libexec/platform-python).
> 
> For this reason, adjust also python-config handling - AC_PATH_PROG
> doesn't work on already absolute path, so make it conditional.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Ping?

> ---
>  m4/python_devel.m4 | 6 +++++-
>  tools/configure.ac | 1 -
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/m4/python_devel.m4 b/m4/python_devel.m4
> index bbf1e0354b2b..676489b8e978 100644
> --- a/m4/python_devel.m4
> +++ b/m4/python_devel.m4
> @@ -2,7 +2,11 @@ AC_DEFUN([AX_CHECK_PYTHON_DEVEL], [
>  ac_previous_cppflags=$CPPFLAGS
>  ac_previous_ldflags=$LDFLAGS
>  ac_previous_libs=$LIBS
> -AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
> +AS_IF([echo "$PYTHON" | grep -q "^/"], [
> +    pyconfig="$PYTHON-config"
> +], [
> +    AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
> +])
>  AS_IF([test x"$pyconfig" = x"no"], [
>      dnl For those that don't have python-config
>      CPPFLAGS="$CFLAGS `$PYTHON -c 'import distutils.sysconfig; \
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 6414fcbb445e..ebf1265643b3 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -368,7 +368,6 @@ AS_IF([test -z "$PYTHON"], [AC_CHECK_PROGS([PYTHON], [python3 python python2], e
>  AS_IF([test "$PYTHON" = "err"], [AC_MSG_ERROR([No python interpreter found])])
>  AS_IF([echo "$PYTHON" | grep -q "^/"], [], [AC_PATH_PROG([PYTHON], [$PYTHON])])
>  PYTHONPATH=$PYTHON
> -PYTHON=`basename $PYTHONPATH`
>  
>  AX_PATH_PROG_OR_FAIL([PYTHONPATH], [$PYTHON])
>  AX_CHECK_PYTHON_VERSION([2], [6])
> -- 
> 2.26.3
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] autoconf: fix handling absolute $PYTHON path

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[PATCH] autoconf: fix handling absolute $PYTHON path"):
> Don't strip full path from $PYTHON variable. This is especially
> relevant, if it points outside of $PATH. This is the case
> for RPM build on CentOS 8 (%{python3} macro points at
> /usr/libexec/platform-python).
> 
> For this reason, adjust also python-config handling - AC_PATH_PROG
> doesn't work on already absolute path, so make it conditional.

Sorry for the delay replying and thanks for trying to improve this
area.

> -AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
> +AS_IF([echo "$PYTHON" | grep -q "^/"], [
> +    pyconfig="$PYTHON-config"
> +], [
> +    AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
> +])

I'm not sure this logic is right.  I haven't looked at this area in
detail but it seems confusing to me.  I don't quite understand why the
preexisting code calls AC_CHECK_PROG followed by AC_PATH_PROG.

I also don't understand why we ever need an absolute path for
$PYTHON-config.  Why don't we just rely on PATH lookups when in invoke
it ?

> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -368,7 +368,6 @@ AS_IF([test -z "$PYTHON"], [AC_CHECK_PROGS([PYTHON], [python3 python python2], e
>  AS_IF([test "$PYTHON" = "err"], [AC_MSG_ERROR([No python interpreter found])])
>  AS_IF([echo "$PYTHON" | grep -q "^/"], [], [AC_PATH_PROG([PYTHON], [$PYTHON])])
>  PYTHONPATH=$PYTHON
> -PYTHON=`basename $PYTHONPATH`

I'm not sure this is right.  I think we sometimes try to look at
PTYHON to see if we should be doing python-3-like things or
python-2-like things, and maybe that logic depends on PYTHON just
being the basename.

Contrary to what I said about leaving $PYTHON-config unresolved and
expecting it to be looked up at the time of use, maybe the right fix
is simply to change python_devel.m4 to use $PYTHONPATH-config instead.

Also using echo | grep -q ^/ seems poor style when case is available.
I think we can rely on case.  But I see that's in the old code
already.

As you can tell, trying to write down what I think doesn't seem to
have unconfused me.  Maybe you can explain ?  Or maybe I need more
coffee.

Thanks,
Ian.

Re: [PATCH] autoconf: fix handling absolute $PYTHON path

[Marek Marczykowski-Górecki]

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

On Tue, Jul 27, 2021 at 02:56:01PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[PATCH] autoconf: fix handling absolute $PYTHON path"):
> > Don't strip full path from $PYTHON variable. This is especially
> > relevant, if it points outside of $PATH. This is the case
> > for RPM build on CentOS 8 (%{python3} macro points at
> > /usr/libexec/platform-python).
> > 
> > For this reason, adjust also python-config handling - AC_PATH_PROG
> > doesn't work on already absolute path, so make it conditional.
> 
> Sorry for the delay replying and thanks for trying to improve this
> area.
> 
> > -AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
> > +AS_IF([echo "$PYTHON" | grep -q "^/"], [
> > +    pyconfig="$PYTHON-config"
> > +], [
> > +    AC_PATH_PROG([pyconfig], [$PYTHON-config], [no])
> > +])
> 
> I'm not sure this logic is right.  I haven't looked at this area in
> detail but it seems confusing to me.  I don't quite understand why the
> preexisting code calls AC_CHECK_PROG followed by AC_PATH_PROG.

I think it tires to get absolute path into $PYTHON, also in case it was
autodetected (the AC_CHECK_PROGS call). Which I think it another place
that is too magic (see below). I'll try to simplify it further.

> I also don't understand why we ever need an absolute path for
> $PYTHON-config.  Why don't we just rely on PATH lookups when in invoke
> it ?

This is a good question. I tried to preserve AC_PATH_PROG to keep
existence check there, but that's rather misused.

> > --- a/tools/configure.ac
> > +++ b/tools/configure.ac
> > @@ -368,7 +368,6 @@ AS_IF([test -z "$PYTHON"], [AC_CHECK_PROGS([PYTHON], [python3 python python2], e
> >  AS_IF([test "$PYTHON" = "err"], [AC_MSG_ERROR([No python interpreter found])])
> >  AS_IF([echo "$PYTHON" | grep -q "^/"], [], [AC_PATH_PROG([PYTHON], [$PYTHON])])
> >  PYTHONPATH=$PYTHON
> > -PYTHON=`basename $PYTHONPATH`
> 
> I'm not sure this is right.  I think we sometimes try to look at
> PTYHON to see if we should be doing python-3-like things or
> python-2-like things, and maybe that logic depends on PYTHON just
> being the basename.

If that's the case, those should be fixed too. PYTHON variable can
accept way more possibilities than just "python" and "python3". And
furthermore "python", depending on distribution, may point at python2 or
python3.
That said, few test builds work with this change, so it's unlikely
something important relies on PYTHON being just the basename.

BTW, are patches sent to xen-devel automatically built on gitlab-ci now?
How can I find such test build results?

> Contrary to what I said about leaving $PYTHON-config unresolved and
> expecting it to be looked up at the time of use, maybe the right fix
> is simply to change python_devel.m4 to use $PYTHONPATH-config instead.

Actually, the only place that needs full path to python, is filling
shebang. Everything else can rely on $PATH and use whatever is given in
$PYTHON (either absolute or just the basename). Especially, there is no
place that needs absolute python-config path, if $PYTHON points just at
the base name.

> Also using echo | grep -q ^/ seems poor style when case is available.
> I think we can rely on case.  But I see that's in the old code
> already.

Yes, I've copied it from there. autoconf macros are not my strong
side...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] autoconf: fix handling absolute $PYTHON path

[Andrew Cooper]

On 27/07/2021 16:29, Marek Marczykowski-Górecki wrote:
> On Tue, Jul 27, 2021 at 02:56:01PM +0100, Ian Jackson wrote:
>> Marek Marczykowski-Górecki writes ("[PATCH] autoconf: fix handling absolute $PYTHON path"):
> BTW, are patches sent to xen-devel automatically built on gitlab-ci now?
> How can I find such test build results?

Emails still aren't being sent automatically yet.  CI is *still*
deterministically broken from the PV32 breakage, and intermittently from
an arm32 randconfig issue.

Everything:
https://gitlab.com/xen-project/patchew/xen/-/pipelines

Specific run for this patch:
https://gitlab.com/xen-project/patchew/xen/-/pipelines/313320712

Looks like it got hit by some of the networking errors which happened
around that time.

~Andrew

Re: [PATCH] autoconf: fix handling absolute $PYTHON path

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [PATCH] autoconf: fix handling absolute $PYTHON path"):
> On Tue, Jul 27, 2021 at 02:56:01PM +0100, Ian Jackson wrote:
> > I'm not sure this logic is right.  I haven't looked at this area in
> > detail but it seems confusing to me.  I don't quite understand why the
> > preexisting code calls AC_CHECK_PROG followed by AC_PATH_PROG.
> 
> I think it tires to get absolute path into $PYTHON, also in case it was
> autodetected (the AC_CHECK_PROGS call). Which I think it another place
> that is too magic (see below). I'll try to simplify it further.
> 
> > I also don't understand why we ever need an absolute path for
> > $PYTHON-config.  Why don't we just rely on PATH lookups when in invoke
> > it ?
> 
> This is a good question. I tried to preserve AC_PATH_PROG to keep
> existence check there, but that's rather misused.

The existence check is probably helpful to avoid a late failure.

> > > --- a/tools/configure.ac
> > > +++ b/tools/configure.ac
> > > @@ -368,7 +368,6 @@ AS_IF([test -z "$PYTHON"], [AC_CHECK_PROGS([PYTHON], [python3 python python2], e
> > >  AS_IF([test "$PYTHON" = "err"], [AC_MSG_ERROR([No python interpreter found])])
> > >  AS_IF([echo "$PYTHON" | grep -q "^/"], [], [AC_PATH_PROG([PYTHON], [$PYTHON])])
> > >  PYTHONPATH=$PYTHON
> > > -PYTHON=`basename $PYTHONPATH`
> > 
> > I'm not sure this is right.  I think we sometimes try to look at
> > PTYHON to see if we should be doing python-3-like things or
> > python-2-like things, and maybe that logic depends on PYTHON just
> > being the basename.

I remembered what I was thinking of and it isn't in xen.git, it's in a
personal project.  I grepped xen.git for PYTHON and python2 and
python3 and didn't find anything that behaves like I was suggesting.

> If that's the case, those should be fixed too. PYTHON variable can
> accept way more possibilities than just "python" and "python3". And
> furthermore "python", depending on distribution, may point at python2 or
> python3.
> That said, few test builds work with this change, so it's unlikely
> something important relies on PYTHON being just the basename.

I think we do, however, need to rely on it being suitable for having
`-config` appended, to find the dev build runes etc.

The last change to this area was by me in
  9caed751db9110c785fd6b1def89d808baa1d907
  tools/configure: Allow specifying python to be found from path

My previous self doesn't seem to have left any notes about why I felt
it necessary to still split the thing back into a "directory and
executable name".  Also that text is wrong since the directory does
not seem to be extracted at all.  I surmise that I just did that
because the existing code did it, and assumed that womething somewhere
else wanted it.

Perhaps this was some support for build arrangements which predate
python-config ?  Those are surely doomed by now.

> Actually, the only place that needs full path to python, is filling
> shebang. Everything else can rely on $PATH and use whatever is given in
> $PYTHON (either absolute or just the basename). Especially, there is no
> place that needs absolute python-config path, if $PYTHON points just at
> the base name.

Right.

Ian.