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

Travis: enforce Python 3.4 support through linter #14884

Merged
merged 2 commits into from Dec 13, 2018

Conversation

Projects
None yet
6 participants
@Sjors
Copy link
Member

commented Dec 6, 2018

The minimum supported version of Python is 3.4 according to dependencies.md. This PR makes the Travis linter use this version in order to catch accidental use of modern syntax.

@laanwj laanwj added the Tests label Dec 6, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Concept ACK

Good idea!

What about bumping the requirement to 3.5 to allow for Xenial?

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

utACK

@Sjors Sjors referenced this pull request Dec 6, 2018

Closed

WIP: bitcoind signer support #2

5 of 5 tasks complete
@hebasto

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Concept ACK

.travis.yml Outdated
@@ -104,6 +104,8 @@ jobs:
BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=thread --disable-hardening --disable-asm CC=clang CXX=clang++"
# x86_64 Linux (no depends, only system libs, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer)
- stage: test
language: python
python: '3.4' # Minimum supported version according to dependencies.md

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 6, 2018

Member

No idea how this is supposed to work. We run the tests in a docker, which is completely separate from the travis environment?

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 6, 2018

Author Member

In that case this probably won't work indeed... I thought we only used Docker on the Xenial host? I'm still waiting for Travis to build my test branch.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 6, 2018

Author Member

I see, trying a different approach...

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

I mean of course strong Concept ACK if you figure out a way to install older versions (specific versions) of packages in ubuntu.

@Sjors Sjors force-pushed the Sjors:2018/12/python-3-4 branch 2 times, most recently from 84049db Dec 6, 2018

@Sjors Sjors changed the title [WIP] Travis: use Python 3.4 on one instance to check support [WIP] bump Python min version to 3.5 and enforce through Travis linter Dec 6, 2018

@Sjors Sjors force-pushed the Sjors:2018/12/python-3-4 branch Dec 6, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

The only way to install older or newer versions of Python is through pyenv. I'd rather not add another dependency.

Instead, a more practical solution is to bump the minimum Python version to 3.5, because our linters use that. It's available on Ubuntu Xenial, but we would lose support for (almost EOL) Trusty (Python 3.4).

In fact, check-doc.py, optimize-pngs.py , gitian-build.py, and verify-commits.py rely on Python 3.6 via check_output(encoding='utf8'). However only a small subset of developers need those scripts. I made trivial workaround for check-doc.py so Travis can use it with Python 3.5 3.4, but this workaround is not safe for the other scripts, see #14128

Alternatively we could modify the linter to work with Python 3.4 syntax. I personally can't be bothered, but I'll happily cherry-pick a solution :-)

I also added .python-version for pyenv which will cause tests with too modern syntax to fail on a developer machine before Travis. When using a script that does require Python >=3.5, you'll have to do pyenv local 3.7.1.

@Sjors Sjors force-pushed the Sjors:2018/12/python-3-4 branch Dec 6, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@Sjors Sjors changed the title [WIP] bump Python min version to 3.5 and enforce through Travis linter Bbump Python min version to 3.5 and enforce through Travis linter Dec 6, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

The python 3.4 requirement is only for the tests, which might be run by users that compile from source. Personally I don't care about the python version of developer tools and linters and I don't mind not supporting 3.4 for those.

@Sjors Sjors force-pushed the Sjors:2018/12/python-3-4 branch 2 times, most recently Dec 6, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@MarcoFalke I initially thought the linter itself needed Python 3.5, but that's not true anymore. So in that case we don't have to bump the version, nice!

Travis build failure demo when you use too modern syntax: https://travis-ci.org/bitcoin/bitcoin/jobs/464581145#L609

@Sjors Sjors changed the title Bbump Python min version to 3.5 and enforce through Travis linter Travis: enforce Python 3.4 support through linter Dec 6, 2018

@@ -43,7 +43,7 @@ jobs:
env:
cache: false
language: python
python: '3.6'
python: '3.4' # Oldest supported version according to doc/dependencies.md

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 6, 2018

Member

Hmm, that would set the minimum version of the linters to 3.4, but not of the functional test suite. (Note that only parsing code according to 3.4 syntax does not mean it can run 3.4)

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 6, 2018

Author Member

The linter uses the available Python executable to lint the functional tests. It will throw an error if sees a syntax that it doesn't recognise, see above demo. So it's not necessary to run the functional tests themselves in Python 3.4, which would require adding pyenv as a dependency inside the Docker container and slow things down.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 6, 2018

Author Member

Also, though hopefully not needed, any developer who uses pyenv on their machine will detect any mistake that slips through the cracks once they run the functional test suite on master.

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

I would prefer having all tools work with the supported version of Python (3.4), for consistency for developers, but agree it's not urgent—having the tests pass is most important.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

@laanwj That's a good point. Agree.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2018

I agree with that too, but it's non-trivial because #14128 had to introduce Python 3.6 syntax in those tools to deal with some sort of encoding hell.

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

I agree with that too, but it's non-trivial because #14128 had to introduce Python 3.6 syntax in those tools to deal with some sort of encoding hell.

Yes, that particular functionality looks hard to replace. I didn't know that was Python 3.6 specific.
(edit: whoops accidental close)

Edit: One way to handle this would be to wrap the appropriate subprocess calls and do the encoding magic only for 3.6, fall back to the system encoding in 3.4 so that it'd at least work.
But I don't know it's worth it. At some point in the future we'll be able to bump the minimum to 3.6 and any work there will be a waste of time.

@laanwj laanwj closed this Dec 7, 2018

@laanwj laanwj reopened this Dec 7, 2018

Sjors added some commits Dec 6, 2018

[test] Travis: enforce Python 3.4 support in functional tests
Make lint/check-doc.py Python 3.4 compatible.

Also add .python-version for pyenv which will cause tests with too
modern syntax to fail on developer machine rather than on Travis.
[test] functional framework: add CScript hex() for Python 3.4
test/functional/wallet_importmulti.py failed with:
AttributeError: 'CScript' object has no attribute 'hex'

@Sjors Sjors force-pushed the Sjors:2018/12/python-3-4 branch to 31926ee Dec 12, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2018

Rebased and added a workaround for calling .hex() on a CScript array in #14886 (cc @jnewbery), which doesn't work before Python 3.5.

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

utACK 31926ee

Going to merge this immediately to avoid any non-Python3.4isms from sneaking in.

@laanwj laanwj merged commit 31926ee into bitcoin:master Dec 13, 2018

2 checks passed

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

laanwj added a commit that referenced this pull request Dec 13, 2018

Merge #14884: Travis: enforce Python 3.4 support through linter
31926ee [test] functional framework: add CScript hex() for Python 3.4 (Sjors Provoost)
74ce326 [test] Travis: enforce Python 3.4 support in functional tests (Sjors Provoost)

Pull request description:

  The minimum supported version of Python is 3.4 according to [dependencies.md](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md). This PR makes the Travis linter use this version in order to catch accidental use of modern syntax.

Tree-SHA512: 71b2c102be72b135a8ba049378d66875760f20a04a657102a399240c5c2b2ddbdfa7d5ab4c0c0242ecc3259e0ee8eb2273f331bc5eb824f4ae4c3cc58aea37ac
@jnewbery
Copy link
Member

left a comment

utACK 31926ee. One request inline.

Thanks for catching my hex() mistake!

@@ -0,0 +1 @@
3.4.9

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 13, 2018

Member

Does pyenv allow you to comment this file? It's not immediately obvious that this is for pyenv, how it's supposed to work, or when we should update this.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 13, 2018

Author Member

I'll look into that. At least you can find out via git-blame :-P

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 15, 2019

Author Member

pyenv doesn't like it when you add a comment to .python-version, so I just updated the code style guidelines with an explanation in #15173.

@Sjors Sjors deleted the Sjors:2018/12/python-3-4 branch Dec 13, 2018

MarcoFalke added a commit that referenced this pull request Jan 15, 2019

Merge #15173: [doc] explain what .python-version does
04215eb [doc] explain what .python-version does (Sjors Provoost)

Pull request description:

  Documentation followup to #14884 (comment).

Tree-SHA512: 7b2b2718f998b7257273fab2e4f1d127f7af468cd26b2bf05b3dc7d6367e349e0a9c92e976a2760e83cf56feb68c37c89cb3a8d92e20be900bea3fb7f1f3d47b
@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

I missed a spot, see #15285

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