Re: [Qemu-devel] [PATCH 1/4] configure: keep track of Python version

[Eduardo Habkost]

On Thu, Aug 22, 2019 at 05:19:26PM -0400, Cleber Rosa wrote:
> On Thu, Aug 22, 2019 at 05:48:46PM +0100, Peter Maydell wrote:
> > On Fri, 9 Nov 2018 at 15:09, Cleber Rosa <address@hidden> wrote:
> > >
> > > Some functionality is dependent on the Python version
> > > detected/configured on configure.  While it's possible to run the
> > > Python version later and check for the version, doing it once is
> > > preferable.  Also, it's a relevant information to keep in build logs,
> > > as the overall behavior of the build can be affected by it.
> > >
> > > Signed-off-by: Cleber Rosa <address@hidden>
> > > ---
> > >  configure | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/configure b/configure
> > > index 74e313a810..67fff0290d 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -1740,6 +1740,9 @@ if ! $python -c 'import sys; 
> > > sys.exit(sys.version_info < (2,7))'; then
> > >        "Use --python=/path/to/python to specify a supported Python."
> > >  fi
> > >
> > > +# Preserve python version since some functionality is dependent on it
> > > +python_version=$($python -V 2>&1 | sed -e 's/Python\ //')
> > > +
> > 
> > Hi. Somebody on IRC has just fallen over a problem where
> > their python's "-V" output prints multiple lines, which
> > means that "$python_version" here is multiple lines, which
> > means that the eventual config-host.mak has invalid syntax
> > because we assume here:
> >
> 
> We've tried a number of things, and just when I thought we wouldn't be
> able to make any sense out of it, I arrived at a still senseless but
> precise reproducer.  TL;DR: it has to do with interactive shells and
> that exact Python build.
> 
> Reproducer (docker may also do the trick here):
> 
>   $ podman run --rm -ti fedora:29 /bin/bash -c 'dnf -y install 
> http://mirror.siena.edu/fedora/linux/releases/29/Everything/x86_64/os/Packages/p/python3-3.7.0-9.fc29.x86_64.rpm;
>  python3 -V'
>   Python 3.7.0 (default, Aug 30 2018, 14:32:33) 
>   [GCC 8.2.1 20180801 (Red Hat 8.2.1-2)]
> 
> With an interactive shell instead:
> 
>   $ podman run --rm -ti fedora:29 /bin/bash -i -c 'dnf -y install 
> http://mirror.siena.edu/fedora/linux/releases/29/Everything/x86_64/os/Packages/p/python3-3.7.0-9.fc29.x86_64.rpm;
>  python3 -V'
>   Python 3.7.0
> 
> How this behavior came to be, baffles me.  But, it seems to be fixed
> on newer versions.
> 
> > > @@ -6823,6 +6826,7 @@ echo "INSTALL_DATA=$install -c -m 0644" >> 
> > > $config_host_mak
> > >  echo "INSTALL_PROG=$install -c -m 0755" >> $config_host_mak
> > >  echo "INSTALL_LIB=$install -c -m 0644" >> $config_host_mak
> > >  echo "PYTHON=$python" >> $config_host_mak
> > > +echo "PYTHON_VERSION=$python_version" >> $config_host_mak
> > >  echo "CC=$cc" >> $config_host_mak
> > >  if $iasl -h > /dev/null 2>&1; then
> > >    echo "IASL=$iasl" >> $config_host_mak
> > 
> > that it's only one line, and will generate bogus makefile
> > syntax if it's got an embedded newline. (Problem system
> > seems to be Fedora 29.)
> >
> 
> The assumption could be guaranteed by a "head -1", and while
> it's not a failproof solution, it would at least not corrupt
> the makefile and the whole build system.
> 
> > I've reread this thread, where there seems to have been
> > some discussion about just running Python itself to
> > get the sys.version value (which is how we check for
> > "is this python too old" earlier in the configure script).
> > But I'm not really clear why trying to parse -V output is better:
> > it's definitely less reliable, as demonstrated by this bug.

Agreed.

> >
> > Given that the only thing as far as I can tell that we
> > do with PYTHON_VERSION is use it in tests/Makefile.inc
> > to suppress a bit of test functionality if we don't have
> > Python 3, could we stop trying to parse -V output and run
> > python to print sys.version_info instead, and/or just
> > have the makefile variable track "is this python 2",
> > since that's what we really care about and would mean we
> > don't have to then search the string for "v2"  ?
> 
> Because I've been bitten way too many times with differences in Python
> minor versions, I see a lot of value in keeping the version
> information in the build system.  But, the same information can
> certainly be obtained in a more resilient way.  Would you object something
> like:
> 
>   python_version=$($python -c 'import sys; print(sys.version().split()[0])')

Sounds much better, but why sys.version().split() instead of
sys.version_info?

  python_version=$($python -c 'import sys; print(sys.version_info[0])')

> 
> Or an even more paranoid version?  On my side, I understand the
> fragility of the current approach, but I also appreciate the
> information it stores.

We have only one place where $(PYTHON_VERSION) is used, and that
code will be removed once we stop supporting Python 2.  I don't
see the point of trying to store extra information that is not
used anywhere in our makefiles.

-- 
Eduardo