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

[Cleber Rosa <crosa@redhat.com>]

On Fri, Aug 23, 2019 at 12:04:46PM -0300, Eduardo Habkost wrote:
> On Fri, Aug 23, 2019 at 09:40:54AM -0400, Cleber Rosa wrote:
> > 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 <crosa@redhat.com> 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 <crosa@redhat.com>
> > > > > > ---
> > > > > >  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.
> [...]
> > 
> > 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).
> 
> Absolutely, but are we talking about the output printed by
> ./configure, or about variables in config-host.mak?
>

Sure, that was a major oversight (or a ill planned shortcut).

> config-host.mak is not for humans, it's just input for our
> makefile code.  The output printed by ./configure on stdout is
> for humans, and I'd agree completely if ./configure keeps
> printing full Python version information on stdout.
> 
> > [...]
> 
> -- 
> Eduardo
> 

Yes, absolutely agree.  I'll send a fix proposal in a few.

Regards!
- Cleber.