Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 14, 2019

Conversation

Projects
None yet
5 participants
@Sjors
Copy link
Member

commented Jan 29, 2019

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

Show resolved Hide resolved configure.ac Outdated

@Sjors Sjors force-pushed the Sjors:2019/01/bitcoin-util-test-python branch from d408089 to 5d0079c Jan 29, 2019

@Sjors Sjors changed the title 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 Jan 29, 2019

@Sjors Sjors force-pushed the Sjors:2019/01/bitcoin-util-test-python branch from 5d0079c to f927c8b Jan 29, 2019

Show resolved Hide resolved configure.ac Outdated
Show resolved Hide resolved configure.ac Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

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.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

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

@Sjors Sjors force-pushed the Sjors:2019/01/bitcoin-util-test-python branch from f927c8b to cebb2b2 Jan 30, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

@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

@Sjors Sjors force-pushed the Sjors:2019/01/bitcoin-util-test-python branch from cebb2b2 to 0890339 Jan 30, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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 0890339

@MarcoFalke MarcoFalke changed the title 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 Jan 30, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

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 MarcoFalke merged commit 0890339 into bitcoin:master Feb 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Feb 14, 2019

Merge #15285: build: Prefer Python 3.4 even if newer versions are pre…
…sent on the system

0890339 build: prefer python3.4 even if newer versions are present on the system (Sjors Provoost)

Pull request description:

  Python 3.4 is this mimimum supported version according to [doc/dependencies.md](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md)

  Systems with [PyEnv](https://github.com/pyenv/pyenv) ensure (via [.python-version](https://github.com/bitcoin/bitcoin/blob/master/.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

Tree-SHA512: b7487081a1ee7c2cb672a2e4bc1943ec8d23825fb941e567cb00fb123e6d59b1d8b7ddbf97d48aca770b9ddb9eacbfe73d8ac8cb1e1cdc34587ee1cee9929840

@Sjors Sjors deleted the Sjors:2019/01/bitcoin-util-test-python branch Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.