Use python not 'python2' #11843

cjavad · 2017-12-07T09:34:33Z

addition to Merge #11830. I looked through the source files looking for other files with

#!/usr/bin/env python2

and found that contrib/devtools/test-security-check.py had the same error that @hkjn pointed out.

laanwj · 2017-12-07T11:21:10Z

It might simply mean that the tool needs python 2.

Did you verify and test that this code is compatible with python 3?

cjavad · 2017-12-07T12:36:37Z

I did, and it produced the same results with python, python2 and python3.

kev009 · 2017-12-07T19:51:40Z

Note that some packages don't install any bin/python' symlink (i.e. freebsd ports) to make the 2-3 transition less ambiguous.

cjavad · 2017-12-11T13:00:23Z

Yes but most unix based operating systems uses the bin/python symlink that is included with fx. macos and linux. And because the #!bin/env python is targeted for unix users and other POSIX based OSs where some of them dont have the #!bin/env python2 symlink like MacOS. So i would argue that using #!bin/env python is preferred over #!bin/env python2.

laanwj · 2017-12-11T17:06:55Z

It looks like we use #!/usr/bin/env python3 in most places:

git ls-files \*.py | xargs head -n 1|less

I'm ok with doing that instead.

hkjn · 2017-12-11T17:15:48Z

FWIW I also agree that setting python3 explicitly is the best path, assuming python2 support no longer is necessary. I went the other way in #11830 to try to be gentle to the long tail of python2-only folks that are still out there.

Any such users on python2 only could still call scripts with python2 somescript.py if they for some reason don't want to upgrade, assuming we keep the scripts compatible with both major versions.

maflcko · 2017-12-11T17:38:42Z

Even though it is not mentioned in doc/dependencies.md, we still target py2 support for user facing scripts. Functional tests are the only exception.

cjavad · 2017-12-12T10:20:02Z

Agreeing with @laanwj and @hkjn here, python3 would be a better choice for future compatibility.

laanwj · 2017-12-12T12:10:57Z

ACK after squash

Even though it is not mentioned in doc/dependencies.md, we still target py2 support for user facing scripts. Functional tests are the only exception.

Yes, agree it's somewhat weird now. I think it's about time to move away from that, and just settle on Python 3. But not be a discussion that we need to do here.

(In any case, changing the default interpreter when this script is called separately does not affect compatibility of the build system, which calls python scripts with an explicit interpreter. I think defaulting to python 3 here makes sense going forward. But I don't care deeply.)

jnewbery · 2017-12-12T18:36:50Z

Yes, agree it's somewhat weird now. I think it's about time to move away from that, and just settle on Python 3. But not be a discussion that we need to do here.

I completely agree with @laanwj. Here are the hashbangs for python files not in the test directory:

share/qt/extract_strings_qt.py|1 col 12| #!/usr/bin/env python
share/rpcauth/rpcauth.py|1 col 12| #!/usr/bin/env python
contrib/testgen/gen_base58_test_vectors.py|1 col 12| #!/usr/bin/env python
contrib/linearize/linearize-hashes.py|1 col 12| #!/usr/bin/env python3
contrib/linearize/linearize-data.py|1 col 12| #!/usr/bin/env python3
contrib/zmq/zmq_sub3.4.py|1 col 12| #!/usr/bin/env python3
contrib/zmq/zmq_sub.py|1 col 12| #!/usr/bin/env python3
contrib/macdeploy/custom_dsstore.py|1 col 12| #!/usr/bin/env python
contrib/macdeploy/macdeployqtplus|1 col 12| #!/usr/bin/env python
contrib/seeds/generate-seeds.py|1 col 12| #!/usr/bin/env python3
contrib/seeds/makeseeds.py|1 col 12| #!/usr/bin/env python3
contrib/filter-lcov.py|1 col 12| #!/usr/bin/env python3
contrib/devtools/check-doc.py|1 col 12| #!/usr/bin/env python
contrib/devtools/copyright_header.py|1 col 12| #!/usr/bin/env python3
contrib/devtools/check-rpc-mappings.py|1 col 12| #!/usr/bin/env python3
contrib/devtools/security-check.py|1 col 12| #!/usr/bin/env python
contrib/devtools/optimize-pngs.py|1 col 12| #!/usr/bin/env python
contrib/devtools/test-security-check.py|1 col 12| #!/usr/bin/env python2
contrib/devtools/update-translations.py|1 col 12| #!/usr/bin/env python
contrib/devtools/github-merge.py|1 col 12| #!/usr/bin/env python3
contrib/devtools/symbol-check.py|1 col 12| #!/usr/bin/env python
contrib/devtools/clang-format-diff.py|1 col 12| #!/usr/bin/env python

Almost all specify python3. Having some which nominally support python2 is a bad idea, since I'd imagine no-one is even testing that they maintain compatibility with python2.

But not be a discussion that we need to do here.

I'll open a PR to completely remove support for python2. We can move the discussion there.

jnewbery · 2017-12-12T20:10:07Z

I think you need a couple more changes:

remove the no longer needed from __future__ import division,print_function line
add a universal_newlines=True argument to the subprocess.Popen() call

laanwj · 2017-12-13T06:23:19Z

I'll open a PR to completely remove support for python2. We can move the discussion there.

I think the only exception would be some of the python scripts for macdeploy that still require Python 2 (@cfields probably knows).

jnewbery · 2017-12-13T12:40:04Z

Please squash commits together (you can do this by running git rebase -i HEAD~3 and then selecting f for the second and third commits), and remove the @ jnewbery tag from the commit message (credit is not generally required for reviewers, and having @ tags in commit messages makes github spam the user).

maflcko · 2017-12-13T19:04:47Z

If you want to do this, you should bump security-check.py to py3-only as well. As I read the current code, it will run the test in py3 and the actual script in py2. Imo, this should be avoided.

maflcko · 2017-12-13T19:05:42Z

Then, squash everything into a single commit, fix the commit message, force push and fix the pull request title.

cjavad · 2017-12-14T09:58:00Z

So git does not seem to work here?

1874058 Make base58 python contrib code work with python3 (Evan Klitzke) bc6fdf2 Change all python files to use Python3 (John Newbery) Pull request description: Following discussion here: #11843 (comment) It's easier for maintainers if all python tools/scripts support only a single version of Python. There are only a few scripts that aren't explicitly python3 at this point, so this PR changes those remaining scripts to explicitly require python3. Tree-SHA512: 5d38eef6e0fc7d8515e23a1f4c75e8b4160fd0fe23cba52a1f41689b114e54a9e503e0724829e8b41982ef98f2d113df80d9e238213b74f09ceaed0344a19e24

1874058 Make base58 python contrib code work with python3 (Evan Klitzke) bc6fdf2 Change all python files to use Python3 (John Newbery) Pull request description: Following discussion here: bitcoin#11843 (comment) It's easier for maintainers if all python tools/scripts support only a single version of Python. There are only a few scripts that aren't explicitly python3 at this point, so this PR changes those remaining scripts to explicitly require python3. Tree-SHA512: 5d38eef6e0fc7d8515e23a1f4c75e8b4160fd0fe23cba52a1f41689b114e54a9e503e0724829e8b41982ef98f2d113df80d9e238213b74f09ceaed0344a19e24

* Merge bitcoin#11881: Remove Python2 support 1874058 Make base58 python contrib code work with python3 (Evan Klitzke) bc6fdf2 Change all python files to use Python3 (John Newbery) Pull request description: Following discussion here: bitcoin#11843 (comment) It's easier for maintainers if all python tools/scripts support only a single version of Python. There are only a few scripts that aren't explicitly python3 at this point, so this PR changes those remaining scripts to explicitly require python3. Tree-SHA512: 5d38eef6e0fc7d8515e23a1f4c75e8b4160fd0fe23cba52a1f41689b114e54a9e503e0724829e8b41982ef98f2d113df80d9e238213b74f09ceaed0344a19e24 * Merge bitcoin#12829: Python3 fixup f50975b [contrib] fixup symbol-check.py Python3 support (John Newbery) 5de2b18 [contrib] fixup security-check.py Python3 support (John Newbery) Pull request description: security-check.py and symbol-check.py were broken by bitcoin#11881. Fix them. Tree-SHA512: 86de3d6dc3292b1ae4cc04c2d7d7dbbf39c9270551d7b224b8d8b19e3184c30c897dbf823200403706d06bb405c0decad5cfd690cb2c0312992a235a4ffcf6bf

* Merge bitcoin#11881: Remove Python2 support 1874058 Make base58 python contrib code work with python3 (Evan Klitzke) bc6fdf2 Change all python files to use Python3 (John Newbery) Pull request description: Following discussion here: bitcoin#11843 (comment) It's easier for maintainers if all python tools/scripts support only a single version of Python. There are only a few scripts that aren't explicitly python3 at this point, so this PR changes those remaining scripts to explicitly require python3. Tree-SHA512: 5d38eef6e0fc7d8515e23a1f4c75e8b4160fd0fe23cba52a1f41689b114e54a9e503e0724829e8b41982ef98f2d113df80d9e238213b74f09ceaed0344a19e24 * Merge bitcoin#12829: Python3 fixup f50975b [contrib] fixup symbol-check.py Python3 support (John Newbery) 5de2b18 [contrib] fixup security-check.py Python3 support (John Newbery) Pull request description: security-check.py and symbol-check.py were broken by bitcoin#11881. Fix them.

1874058 Make base58 python contrib code work with python3 (Evan Klitzke) bc6fdf2 Change all python files to use Python3 (John Newbery) Pull request description: Following discussion here: bitcoin#11843 (comment) It's easier for maintainers if all python tools/scripts support only a single version of Python. There are only a few scripts that aren't explicitly python3 at this point, so this PR changes those remaining scripts to explicitly require python3. Tree-SHA512: 5d38eef6e0fc7d8515e23a1f4c75e8b4160fd0fe23cba52a1f41689b114e54a9e503e0724829e8b41982ef98f2d113df80d9e238213b74f09ceaed0344a19e24

laanwj added the Scripts and tools label Dec 7, 2017

jnewbery mentioned this pull request Dec 12, 2017

Remove Python2 support #11881

Merged

cjavad closed this Dec 14, 2017

cjavad mentioned this pull request Dec 14, 2017

Moving to python3 #11895

Closed

codablock mentioned this pull request Aug 13, 2018

Backport Bitcoin #11881: Remove Python2 support dashpay/dash#2224

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Use python not 'python2' #11843

Use python not 'python2' #11843

Uh oh!

Conversation

cjavad commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 7, 2017

Uh oh!

cjavad commented Dec 7, 2017

Uh oh!

kev009 commented Dec 7, 2017

Uh oh!

cjavad commented Dec 11, 2017

Uh oh!

laanwj commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkjn commented Dec 11, 2017

Uh oh!

maflcko commented Dec 11, 2017

Uh oh!

cjavad commented Dec 12, 2017

Uh oh!

laanwj commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Dec 12, 2017

Uh oh!

jnewbery commented Dec 12, 2017

Uh oh!

laanwj commented Dec 13, 2017

Uh oh!

jnewbery commented Dec 13, 2017

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

cjavad commented Dec 14, 2017

Uh oh!

Uh oh!

cjavad commented Dec 7, 2017 •

edited

Loading

laanwj commented Dec 11, 2017 •

edited

Loading

laanwj commented Dec 12, 2017 •

edited

Loading