build: Prefer Python 3.4 even if newer versions are present on the system #15285

Sjors commented at 6:12 PM on January 29, 2019: member

Python 3.4 is this mimimum supported version according to doc/dependencies.md

Systems with PyEnv ensure (via .python-version) that Python 3.4 is used for the functional tests. However make check calls bitcoin-util-test.py using the Python command found by configure.ac, which looks system wide.

On systems with multiple versions of Python this would cause make check to fail, as it tries to call a version of Python that PyEnv blocks.

This is solved by preferring python3.4 in configure.ac.

I missed this in #14884, so ideally this should be tagged 0.18

in configure.ac:88 in d408089597 outdated

  84 | @@ -85,7 +85,7 @@ AC_PATH_TOOL(STRIP, strip)
  85 |  AC_PATH_TOOL(GCOV, gcov)
  86 |  AC_PATH_PROG(LCOV, lcov)
  87 |  dnl Python 3.x is supported from 3.4 on (see https://github.com/bitcoin/bitcoin/issues/7893)
  88 | -AC_PATH_PROGS([PYTHON], [python3.7 python3.6 python3.5 python3.4 python3 python])
  89 | +AC_PATH_PROGS([PYTHON], [python3.4 python3 python])

MarcoFalke commented at 6:16 PM on January 29, 2019:

Could you reorder those, such that python3.4 is merely the first to look for? Unless you are certain that there is a python3 alias on all systems. (Otherwise it would fall back to python, which commonly means python2)

Sjors commented at 6:23 PM on January 29, 2019:

Done. That indeed seems safer, and still works with PyEnv.

Sjors force-pushed on Jan 29, 2019

Sjors renamed this:
~~Do not explicitly call Python >3.4 even if present on the system~~
Prefer Python 3.4 even if newer versions are present on the system
on Jan 29, 2019

Sjors force-pushed on Jan 29, 2019

in configure.ac:87 in f927c8baf6 outdated

  84 | @@ -85,7 +85,8 @@ AC_PATH_TOOL(STRIP, strip)
  85 |  AC_PATH_TOOL(GCOV, gcov)
  86 |  AC_PATH_PROG(LCOV, lcov)
  87 |  dnl Python 3.x is supported from 3.4 on (see https://github.com/bitcoin/bitcoin/issues/7893)

MarcoFalke commented at 6:39 PM on January 29, 2019:

I'd prefer not to link to github issues unless necessary to keep the source code "self contained" as possible.

I guess you could delete this comment altogether, since we added doc/dependencies.md in the meantime.

in configure.ac:89 in f927c8baf6 outdated

  84 | @@ -85,7 +85,8 @@ AC_PATH_TOOL(STRIP, strip)
  85 |  AC_PATH_TOOL(GCOV, gcov)
  86 |  AC_PATH_PROG(LCOV, lcov)
  87 |  dnl Python 3.x is supported from 3.4 on (see https://github.com/bitcoin/bitcoin/issues/7893)
  88 | -AC_PATH_PROGS([PYTHON], [python3.7 python3.6 python3.5 python3.4 python3 python])
  89 | +dnl Python 3.4 is specified in .python-version and should be used if available (see https://github.com/bitcoin/bitcoin/pull/15285)
  90 | +AC_PATH_PROGS([PYTHON], [python3.4 python3.7 python3.6 python3.5 python3 python])

MarcoFalke commented at 6:40 PM on January 29, 2019:

nit: Could do without the github link above and sort like 3.4 3.5 3.6 ... 3. Feel free to ignore

DrahtBot commented at 8:03 PM on January 29, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14954 (WIP: build: Require python 3.5 by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 10:04 PM on January 29, 2019: member

I'm a bit ambivalent on this, ideally it wouldn't matters so much what version of Python is used in the first place if it's more recent than the minimum.

fanquake added the label Build system on Jan 29, 2019

Sjors commented at 10:21 AM on January 30, 2019: member

@MarcoFalke I got rid of the Github links and switched to the more aesthetic version ordering.

Sjors force-pushed on Jan 30, 2019

Sjors commented at 10:28 AM on January 30, 2019: member

@laanwj I agree with that. The problem is that Python doesn't have a way to enforce backwards compatibility in any given script file. That's why I added PyEnv support in #14884 which prevents us from accidentally using Python > 3.4 syntax. The only reason for the change here is that otherwise PyEnv breaks.

Maybe someone knows another approach for configure.ac to find the correct python executable in a way compatible with PyEnv? The most obvious approach that comes to mind is to drop python3.7 python3.6 python3.5 python3.4 entirely, and only leave python3 python, which do play nicely with PyEnv.

I don't know if there are any systems where python3 doesn't point to the most recently installed version. I do know on some systems python doesn't always link to python3, e.g. on macOS the default is/was Python 2 and the homebrew installation of Python 3 would leave the python command alone, leading to headaches.

build: prefer python3.4 even if newer versions are present on the system

Python 3.4 is the mimimum supported version according to doc/dependencies.md

Systems with PyEnv ensure (via .python-version) that Python 3.4 is used
for the functional tests. However make check calls bitcoin-util-test.py
using the Python command found by configure.ac, which looks system wide.

On systems with multiple versions of Python this would cause make check
to fail, as it tries to call a version of Python that PyEnv blocks.

This is solved by preferring python3.4 in configure.ac

0890339fb3

Sjors force-pushed on Jan 30, 2019

MarcoFalke commented at 1:12 PM on January 30, 2019: member

There shouldn't be any difference in what python version make check runs, since all supported versions should work equally. Also, the gitian builds use the bionic python3 version, which wouldn't change with that patch. I don't see any risk in this patch.

utACK 0890339fb337de7690d501a5436657a9d996e5a7

MarcoFalke renamed this:
~~Prefer Python 3.4 even if newer versions are present on the system~~
build: Prefer Python 3.4 even if newer versions are present on the system
on Jan 30, 2019

laanwj commented at 11:20 AM on January 31, 2019: member

I don't know if there are any systems where python3 doesn't point to the most recently installed version.

IIRC there are some systems which don't have python3 nor python at all, and only versioned named (BSDs maybe?). Though I don't remember the exact history of that line I'm fairly sure it's like this for a good reason.

On systems with multiple versions of Python this would cause make check to fail, as it tries to call a version of Python that PyEnv blocks.

What about overriding the PYTHON variable instead of changing auto-detect? After all you're trying to make it use a certain version of Python.

Sjors commented at 1:01 PM on January 31, 2019: member

@laanwj override with what though? Another autodetect specifically for this purpose?

laanwj commented at 2:08 PM on January 31, 2019: member

I'd prefer not using auto-detect at all in that case, when you know what exact Python executable you want to use. You can pass what version you want to configure.

My real point here is that I don't like treating Python 3.4 specially in auto-detection just because it happens to be used in our tests right now. Say, we bump that Python version later and then this needs to be changed again, it's easy to forget.

(Another way would be to detect whether PyEnv is used and if so, choose that Python version. This is already much more generally applicable)

Sjors commented at 3:37 PM on January 31, 2019: member

We don't know which Python executable to use. It's either python or python3, somewhere in $PATH, when using PyEnv.

Another approach could be to not use python3.4 style executables, unless python3 and python return a version that's unacceptably old. I don't know how to configure that though.

MarcoFalke commented at 3:54 PM on January 31, 2019: member

Say, we bump that Python version later and then this needs to be changed again, it's easy to forget.

The python3.4 would need to be removed either way. At least with this change it is no longer so easy to forget.

MarcoFalke merged this on Feb 14, 2019

MarcoFalke closed this on Feb 14, 2019

MarcoFalke referenced this in commit 33480c6366 on Feb 14, 2019

Sjors deleted the branch on Feb 15, 2019

PastaPastaPasta referenced this in commit 2ee07e0da2 on Jun 27, 2021

PastaPastaPasta referenced this in commit 97e53da21d on Jun 28, 2021

PastaPastaPasta referenced this in commit 589bbc9166 on Jun 29, 2021

MarcoFalke locked this on Dec 16, 2021