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 <cr...@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 <cr...@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.