Use python not 'python2'

cjavad commented at 9:34 am on December 7, 2017: none

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 commented at 11:21 am on December 7, 2017: member

It might simply mean that the tool needs python 2.

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

laanwj added the label Scripts and tools on Dec 7, 2017

cjavad commented at 12:36 pm on December 7, 2017: none

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

kev009 commented at 7:51 pm on December 7, 2017: none

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

cjavad commented at 1:00 pm on December 11, 2017: none

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 commented at 5:06 pm on December 11, 2017: member

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

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

I’m ok with doing that instead.

hkjn commented at 5:15 pm on December 11, 2017: contributor

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.

MarcoFalke commented at 5:38 pm on December 11, 2017: member

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 commented at 10:20 am on December 12, 2017: none

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

laanwj commented at 12:10 pm on December 12, 2017: member

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 commented at 6:36 pm on December 12, 2017: member

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:

 0share/qt/extract_strings_qt.py|1 col 12| #!/usr/bin/env python
 1share/rpcauth/rpcauth.py|1 col 12| #!/usr/bin/env python
 2contrib/testgen/gen_base58_test_vectors.py|1 col 12| #!/usr/bin/env python
 3contrib/linearize/linearize-hashes.py|1 col 12| #!/usr/bin/env python3
 4contrib/linearize/linearize-data.py|1 col 12| #!/usr/bin/env python3
 5contrib/zmq/zmq_sub3.4.py|1 col 12| #!/usr/bin/env python3
 6contrib/zmq/zmq_sub.py|1 col 12| #!/usr/bin/env python3
 7contrib/macdeploy/custom_dsstore.py|1 col 12| #!/usr/bin/env python
 8contrib/macdeploy/macdeployqtplus|1 col 12| #!/usr/bin/env python
 9contrib/seeds/generate-seeds.py|1 col 12| #!/usr/bin/env python3
10contrib/seeds/makeseeds.py|1 col 12| #!/usr/bin/env python3
11contrib/filter-lcov.py|1 col 12| #!/usr/bin/env python3
12contrib/devtools/check-doc.py|1 col 12| #!/usr/bin/env python
13contrib/devtools/copyright_header.py|1 col 12| #!/usr/bin/env python3
14contrib/devtools/check-rpc-mappings.py|1 col 12| #!/usr/bin/env python3
15contrib/devtools/security-check.py|1 col 12| #!/usr/bin/env python
16contrib/devtools/optimize-pngs.py|1 col 12| #!/usr/bin/env python
17contrib/devtools/test-security-check.py|1 col 12| #!/usr/bin/env python2
18contrib/devtools/update-translations.py|1 col 12| #!/usr/bin/env python
19contrib/devtools/github-merge.py|1 col 12| #!/usr/bin/env python3
20contrib/devtools/symbol-check.py|1 col 12| #!/usr/bin/env python
21contrib/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 commented at 8:10 pm on December 12, 2017: member

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 commented at 6:23 am on December 13, 2017: member

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 commented at 12:40 pm on December 13, 2017: member

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

MarcoFalke commented at 7:04 pm on December 13, 2017: member

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.

MarcoFalke commented at 7:05 pm on December 13, 2017: member

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

cjavad commented at 9:58 am on December 14, 2017: none

So git does not seem to work here?

cjavad closed this on Dec 14, 2017

laanwj referenced this in commit 624bee9659 on Mar 28, 2018

codablock referenced this in commit 79f578ae17 on Aug 13, 2018

UdjinM6 referenced this in commit 7cf9572c26 on Aug 13, 2018

CryptoCentric referenced this in commit 7fd2c46693 on Apr 25, 2019

PastaPastaPasta referenced this in commit 2aee09c5ab on Dec 16, 2020

PastaPastaPasta referenced this in commit a2d0a288c4 on Dec 18, 2020

DrahtBot locked this on Sep 8, 2021

Use python not ‘python2’ #11843