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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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: signature.asc
Description: PGP signature