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

[Eduardo Habkost]

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 <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.
[...]
> 
> 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?

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