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

Drop python2 support #2215

Merged
merged 5 commits into from
Nov 20, 2020
Merged

Drop python2 support #2215

merged 5 commits into from
Nov 20, 2020

Conversation

Lestropie
Copy link
Member

Closes #2210.

Closes #2047.

Will eventually resolve #2190.

Should consider resolving #2191 here.

Made a bit of a list in 3a856ce as to what changed, but would appreciate some reflection on whether or not there's anywhere else in the code base that required gymnastics to support both Python 2 and 3 that I may have missed.

Note that these changes are not compatible with Python2.
@Lestropie Lestropie added this to the 3.1.0 updates milestone Oct 28, 2020
@Lestropie Lestropie self-assigned this Oct 28, 2020
- All executable Python scripts directly invoke /usr/bin/python3.
- Separate testing of configure and build scripts using Python2 and Python3 no longer required.
- Replace distutils.spawn.find_executable() with shutil.which().
- Activate more stringent pylint tests, with some limited corresponding code changes.
- configure: Test presence of shlex.quote() and shutil.which().
- Remove mrtrix3.utils.STRING_TYPES, which is no longer required for testing the types of provided function arguments.
- population_template: No longer need to bind Python2's raw_input() to input().
- mrtrix3.path: Remove quote() function: shlex.quote() should be used directly.
- run_pylint: Remove capability for forcing Python version via environment variable.
- Binary test data: Update vectorstats generation / testing scripts to use Python3.
@jdtournier
Copy link
Member

All looks sensible, other than the first thing: changing the interpreter on the shebang to python3. I don't think that's the recommended way of doing things...? It might work on Ubuntu, but it's not guaranteed to work on other standard distros. Are there recommendations around this from the python community?

@jdtournier
Copy link
Member

Ok, forget that last comment, it is indeed the recommendation on this PEP:
https://www.python.org/dev/peps/pep-0394/

@jdtournier
Copy link
Member

jdtournier commented Nov 17, 2020

CI is failing due to lots of these errors during pylint:

Bad option value 'import-outside-toplevel' (bad-option-value)

A quick look online reveals that this check is available as of pylint version 2.4. But the version dragged in by apt-get on the GitHub runners is 1.8.3:

Unpacking pylint3 (1.8.3-1) ...

which explains the errors.

Previously, bad-option-value was also disabled in the config, but that was removed in 3a856ce - which is why the issue only arises now.

So, two options:

  • reinstate bad-option-value in pylint.rc
  • modify all occurrences of #pylint: disable=import-outside-toplevel to additionally have bad-option-value, e.g. #pylint: disable=bad-option-value,import-outside-toplevel (as suggested here).

Lestropie added a commit that referenced this pull request Nov 17, 2020
See if this addresses outdated version of pylint pulled by GitHub runner as commented in #2215.
Lestropie added a commit that referenced this pull request Nov 17, 2020
See if this addresses outdated version of pylint pulled by GitHub runner as commented in #2215.
@Lestropie
Copy link
Member Author

Installing pylint via pip rather than apt-get brings in 2.6.0 rather than 1.8.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants