| Message ID | 20181109150710.31085-2-crosa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Record Python version and misc test/CI fixes | expand |
On Fri, Nov 09, 2018 at 10:07:07AM -0500, Cleber Rosa 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\ //') What about: $($python -c 'import sys;print(sys.version)') ? It is very verbose, but I think that's a good thing. > + > # Suppress writing compiled files > python="$python -B" > > @@ -5918,7 +5921,7 @@ echo "LDFLAGS $LDFLAGS" > echo "QEMU_LDFLAGS $QEMU_LDFLAGS" > echo "make $make" > echo "install $install" > -echo "python $python" > +echo "python $python ($python_version)" > if test "$slirp" = "yes" ; then > echo "smbd $smbd" > fi > @@ -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 The output of "python -V" and "sys.version" seems to be meant for humans, not software. If we really want something to be used in conditional makefile rules, I'd prefer to use sys.version_info. e.g.: python_major_version="$($python -c 'import sys;print(sys.version_info[0])')" echo "PYTHON_MAJOR_VERSION=$python_major_version" > echo "CC=$cc" >> $config_host_mak > if $iasl -h > /dev/null 2>&1; then > echo "IASL=$iasl" >> $config_host_mak > -- > 2.19.1 >
On 11/9/18 10:49 AM, Eduardo Habkost wrote: > On Fri, Nov 09, 2018 at 10:07:07AM -0500, Cleber Rosa 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\ //') > > What about: > $($python -c 'import sys;print(sys.version)') > ? > > It is very verbose, but I think that's a good thing. > Well, something like: '3.7.1 (default, Oct 23 2018, 18:19:07) \n[GCC 8.2.1 20180801 (Red Hat 8.2.1-2)]' Doesn't qualify as simply the Python *version*. The documentation[1] puts it clearly: "A string containing the version number of the Python interpreter plus additional information on the build number and compiler used. This string is displayed when the interactive interpreter is started." I can't see how the compiler used on the Python build, or the build date, can be significant to someone debugging the type of Python code that will be on QEMU. >> + >> # Suppress writing compiled files >> python="$python -B" >> >> @@ -5918,7 +5921,7 @@ echo "LDFLAGS $LDFLAGS" >> echo "QEMU_LDFLAGS $QEMU_LDFLAGS" >> echo "make $make" >> echo "install $install" >> -echo "python $python" >> +echo "python $python ($python_version)" >> if test "$slirp" = "yes" ; then >> echo "smbd $smbd" >> fi >> @@ -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 > > The output of "python -V" and "sys.version" seems to be meant for > humans, not software. If we really want something to be used in > conditional makefile rules, I'd prefer to use sys.version_info. > e.g.: > "Python -V" is quite different from "sys.version". Indeed it includes the "Python " prefix, but that's all, everything else is the version number. > python_major_version="$($python -c 'import sys;print(sys.version_info[0])')" > echo "PYTHON_MAJOR_VERSION=$python_major_version" > > No, I'm actually interested in the other version components, not just the major version. As I tried to explain in another thread, differences from Python 3.0 to 3.4 are many, and from 3.4 to 3.6 and so on. Then, we can either introduce "PYTHON_MAJOR_VERSION", "PYTHON_MINOR_VERSION", "PYTHON_RELEASE" of sorts, or just have a single dot separated version string. - Cleber [1] - https://docs.python.org/3/library/sys.html#sys.version >> echo "CC=$cc" >> $config_host_mak >> if $iasl -h > /dev/null 2>&1; then >> echo "IASL=$iasl" >> $config_host_mak >> -- >> 2.19.1 >> >
On Fri, Nov 09, 2018 at 11:39:32AM -0500, Cleber Rosa wrote: > > > On 11/9/18 10:49 AM, Eduardo Habkost wrote: > > On Fri, Nov 09, 2018 at 10:07:07AM -0500, Cleber Rosa 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\ //') > > > > What about: > > $($python -c 'import sys;print(sys.version)') > > ? > > > > It is very verbose, but I think that's a good thing. > > > > Well, something like: > > '3.7.1 (default, Oct 23 2018, 18:19:07) \n[GCC 8.2.1 20180801 (Red Hat > 8.2.1-2)]' > > Doesn't qualify as simply the Python *version*. The documentation[1] > puts it clearly: "A string containing the version number of the Python > interpreter plus additional information on the build number and compiler > used. This string is displayed when the interactive interpreter is started." > > I can't see how the compiler used on the Python build, or the build > date, can be significant to someone debugging the type of Python code > that will be on QEMU. No problem to me. > > >> + > >> # Suppress writing compiled files > >> python="$python -B" > >> > >> @@ -5918,7 +5921,7 @@ echo "LDFLAGS $LDFLAGS" > >> echo "QEMU_LDFLAGS $QEMU_LDFLAGS" > >> echo "make $make" > >> echo "install $install" > >> -echo "python $python" > >> +echo "python $python ($python_version)" > >> if test "$slirp" = "yes" ; then > >> echo "smbd $smbd" > >> fi > >> @@ -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 > > > > The output of "python -V" and "sys.version" seems to be meant for > > humans, not software. If we really want something to be used in > > conditional makefile rules, I'd prefer to use sys.version_info. > > e.g.: > > > > "Python -V" is quite different from "sys.version". Indeed it includes > the "Python " prefix, but that's all, everything else is the version number. Is this a guarantee documented somewhere? > > > python_major_version="$($python -c 'import sys;print(sys.version_info[0])')" > > echo "PYTHON_MAJOR_VERSION=$python_major_version" > > > > > > No, I'm actually interested in the other version components, not just > the major version. As I tried to explain in another thread, differences > from Python 3.0 to 3.4 are many, and from 3.4 to 3.6 and so on. Do you have any examples in mind? I really doubt we will need to look at the Python minor version number inside shell scripts or makefiles. > > Then, we can either introduce "PYTHON_MAJOR_VERSION", > "PYTHON_MINOR_VERSION", "PYTHON_RELEASE" of sorts, or just have a single > dot separated version string. A dot separated version string would work for me, too. I don't mind the format that much, because I expect the new variable to become unnecessary in the next year. :)
On 11/9/18 1:25 PM, Eduardo Habkost wrote: > On Fri, Nov 09, 2018 at 11:39:32AM -0500, Cleber Rosa wrote: >> >> >> On 11/9/18 10:49 AM, Eduardo Habkost wrote: >>> On Fri, Nov 09, 2018 at 10:07:07AM -0500, Cleber Rosa 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\ //') >>> >>> What about: >>> $($python -c 'import sys;print(sys.version)') >>> ? >>> >>> It is very verbose, but I think that's a good thing. >>> >> >> Well, something like: >> >> '3.7.1 (default, Oct 23 2018, 18:19:07) \n[GCC 8.2.1 20180801 (Red Hat >> 8.2.1-2)]' >> >> Doesn't qualify as simply the Python *version*. The documentation[1] >> puts it clearly: "A string containing the version number of the Python >> interpreter plus additional information on the build number and compiler >> used. This string is displayed when the interactive interpreter is started." >> >> I can't see how the compiler used on the Python build, or the build >> date, can be significant to someone debugging the type of Python code >> that will be on QEMU. > > No problem to me. > >> >>>> + >>>> # Suppress writing compiled files >>>> python="$python -B" >>>> >>>> @@ -5918,7 +5921,7 @@ echo "LDFLAGS $LDFLAGS" >>>> echo "QEMU_LDFLAGS $QEMU_LDFLAGS" >>>> echo "make $make" >>>> echo "install $install" >>>> -echo "python $python" >>>> +echo "python $python ($python_version)" >>>> if test "$slirp" = "yes" ; then >>>> echo "smbd $smbd" >>>> fi >>>> @@ -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 >>> >>> The output of "python -V" and "sys.version" seems to be meant for >>> humans, not software. If we really want something to be used in >>> conditional makefile rules, I'd prefer to use sys.version_info. >>> e.g.: >>> >> >> "Python -V" is quite different from "sys.version". Indeed it includes >> the "Python " prefix, but that's all, everything else is the version number. > > Is this a guarantee documented somewhere? > >> >>> python_major_version="$($python -c 'import sys;print(sys.version_info[0])')" >>> echo "PYTHON_MAJOR_VERSION=$python_major_version" >>> >>> >> >> No, I'm actually interested in the other version components, not just >> the major version. As I tried to explain in another thread, differences >> from Python 3.0 to 3.4 are many, and from 3.4 to 3.6 and so on. > > Do you have any examples in mind? I really doubt we will need to > look at the Python minor version number inside shell scripts or > makefiles. > I guess I failed to make myself clear, because it looks like you're missing my point here. The situation that I envision is a developer writes a Python based test, runs it locally, gets a PASS, puts in a patch, sends it upstream. It gets executed on a number of different CI environments. It fails on one (or some). What's causing the failure? The Python *minor* version may be a significant difference. Like I said before, printing that is the most important functionality at this point. It is indeed debatable whether we'll need to keep it available for make/shell usage. >> >> Then, we can either introduce "PYTHON_MAJOR_VERSION", >> "PYTHON_MINOR_VERSION", "PYTHON_RELEASE" of sorts, or just have a single >> dot separated version string. > > A dot separated version string would work for me, too. I don't > mind the format that much, because I expect the new variable to > become unnecessary in the next year. :) > Cool. Thanks for the feedback. Just to get this right, what do I need to change here? - Cleber.
On 11/9/18 1:25 PM, Eduardo Habkost wrote: >> >> "Python -V" is quite different from "sys.version". Indeed it includes >> the "Python " prefix, but that's all, everything else is the version number. > > Is this a guarantee documented somewhere? > Oops, looks like I missed that comment, and failed to address it. Now, I must say I don't expect a documented guarantee about this will exist, but it looks like this is an acknowledged use case: https://bugs.python.org/issue18338 - Cleber.
On Fri, Nov 09, 2018 at 02:51:11PM -0500, Cleber Rosa wrote: > > > On 11/9/18 1:25 PM, Eduardo Habkost wrote: > >> > >> "Python -V" is quite different from "sys.version". Indeed it includes > >> the "Python " prefix, but that's all, everything else is the version number. > > > > Is this a guarantee documented somewhere? > > > > Oops, looks like I missed that comment, and failed to address it. Now, > I must say I don't expect a documented guarantee about this will exist, > but it looks like this is an acknowledged use case: > > https://bugs.python.org/issue18338 Well, considering that it even has a unit test checking for a specific format[1], I think the usage in this patch can be considered acceptable. [1] https://hg.python.org/cpython/rev/e6384b8b2325
On Fri, Nov 09, 2018 at 10:07:07AM -0500, Cleber Rosa 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> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
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: > @@ -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.) 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. 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" ? thanks -- PMM
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. > > 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])') 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. > > thanks > -- PMM Thanks! - Cleber.
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])') > > 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.
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. > > -- > 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.
On Fri, 23 Aug 2019 at 14:41, Cleber Rosa <crosa@redhat.com> wrote: > 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. I'd be happy with keeping it in the human-readable output that configure emits: if it's the wrong format there that's pretty harmless. But we shouldn't feed it into the makefiles unless we really need it, and we shouldn't let the format of whatever we do feed into the makefiles be driven by the desire to print something human-readable in configure's output -- there's no need for the two things to be the exact same string. thanks -- PMM
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? 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. > [...]
On Fri, Aug 23, 2019 at 02:44:15PM +0100, Peter Maydell wrote: > On Fri, 23 Aug 2019 at 14:41, Cleber Rosa <crosa@redhat.com> wrote: > > 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. > > I'd be happy with keeping it in the human-readable output > that configure emits: if it's the wrong format there that's > pretty harmless. But we shouldn't feed it into the makefiles > unless we really need it, and we shouldn't let the format > of whatever we do feed into the makefiles be driven by > the desire to print something human-readable in configure's > output -- there's no need for the two things to be the > exact same string. > > thanks > -- PMM I couldn't agree more. The shortcut taken to print both the human readable version and use that to check the version in the makefile was unfortunate. I'll send a fix proposal in a few. Best, - Cleber.
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.
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\ //') + # Suppress writing compiled files python="$python -B" @@ -5918,7 +5921,7 @@ echo "LDFLAGS $LDFLAGS" echo "QEMU_LDFLAGS $QEMU_LDFLAGS" echo "make $make" echo "install $install" -echo "python $python" +echo "python $python ($python_version)" if test "$slirp" = "yes" ; then echo "smbd $smbd" fi @@ -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
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(-)