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

[Cleber Rosa]

On Thu, Aug 22, 2019 at 06:54:20PM -0300, Eduardo Habkost wrote:
> 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])')
>

I meant to capture not only the major version, but the minor and release
as well.  My reasoning (may not appeal more people):

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

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

I see it being used by humans, so that brings a lot of subjetivity
into the matter.  IMO this is not out of place within the build
system, given that a lot of requirements detected by configure will
print out their versions (GTK, nettle, spice, etc).

But I'm certainly OK with dropping it if no value is perceived by
anyone else.

Cheers,
- Cleber.