| Message ID | 20210602033206.720860-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | autoconf: fix handling absolute $PYTHON path | expand |
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 >
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.
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...
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
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.
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])
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(-)