Remove Python2 support #11881

jnewbery · 2017-12-12T19:53:09Z

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.

practicalswift · 2017-12-12T20:37:07Z

Strong concept ACK!

Pros:

Less work required to maintain the Python scripts
Avoids potential bugs due to subtle Python 2/3 incompatibilities

Cons:

?

maflcko · 2017-12-12T21:25:00Z

ACK for devtools. Not sure about unit tests and user facing scripts such as rpcauth.py, see also discussion in #11433

jnewbery · 2017-12-12T22:10:05Z

Marco, your comment from #11433:

I am not against deprecating py2, but we should do it properly such that ./configure fails early and warns about it. Also it should be mentioned in the release notes.

Would you support this PR for all Python files including user-facing scripts and tests if it met those conditions?

droark · 2017-12-12T23:10:23Z

Concept ACK. I'm not aware of any major projects still on Py2 and intending to stay there. AFAIK, just about everything that's still Py2 is due to legacy and/or manpower issues. Here's how I'd like to see things handled.

Make the decision to go Py3-only.
Add something in release notes for the next major version (0.16 or 0.17, I assume) indicating that Py2 will no longer be usable in the following version.
Make the changes, and force any config/make materials reliant on Python to ensure that Py3 is present.

maflcko · 2017-12-12T23:27:11Z

@jnewbery I'd be -0 on that. The reason that the functional tests run at 3.4+ only, is because it would be nearly impossible to have them compatible with both 2.7 and 3.4. However, the python code used for the unit tests and the few scripts meant for users is rather condensed and hopefully not much refactored in the future. So by not touching them, they should keep their compatibility.
Note that the loose scripts are not (regularly) tested on py2 nor py3, so just declaring that they no longer function with py2 isn't a huge improvement. (Or making them merly incompatible with py2 isn't a huge improvement either, imo). As long as they are not rewritten in a large-ish way, I don't see why we should change the dependency requirements...

laanwj · 2017-12-19T08:02:48Z

ACK on the idea to go Py3-only long-term. There's a burden on supporting multiple versions of python and that burden will only become larger as Python 3 progresses, by locking us out of new features and better ways to do things.

(this is a similar argument as that for going to C++11)

The only problematic case are scripts that depend on libraries that only support Python2. These are rare, but I vaguely remember some of the macdeploy scripts do, so please test those carefully.

As long as they are not rewritten in a large-ish way, I don't see why we should change the dependency requirements...

I also agree with this, though. There's no urgent reason to make this change at once, certainly for scripts that are hardly touched.

droark · 2017-12-19T08:14:15Z

@laanwj - I haven't tried the scripts yet but I think macdeploy's libraries all support Py3. It looks like there are three external packages: biplist, ds_store, and mac_alias. The commit lists seem to indicate that there's a reasonable chance all of them have Py3 support, or at least enough to get macdeploy to run. I'll give macdeploy a spin soon and submit upstream patches if necessary.

EDIT: Wrong biplist link.

fanquake · 2017-12-19T08:41:50Z

We are still patching biplist and mac_alias, so you could definitely send those patches upstream.

laanwj · 2017-12-19T10:07:21Z

OTOH the situation of having some things work in py2 but not others is confusing. It doesn't really benefit anyone, and there's also an overhead explaining it to contributors. That's one argument for doing this at once.

droark · 2017-12-19T21:05:48Z

@fanquake - Yep, mac_alias doesn't have those fixes. I'll submit upstream. I'll also try with biplist but I don't know how that'll go, since it's not a Py3 fix.

droark · 2017-12-19T22:49:23Z

Have submitted PRs for biplist (not Py3) and mac_alias. It's a start!

ryanofsky

utACK bbe67bdd82925cc72a868bb3de1896e61cae79db. Since this marked WIP, it might be good to update the PR description to say what other changes are underway or planned.

ryanofsky · 2017-12-20T16:03:47Z

contrib/devtools/optimize-pngs.py

@@ -37,7 +37,7 @@ def content_hash(filename):
    for file in os.listdir(absFolder):
        extension = os.path.splitext(file)[1]
        if extension.lower() == '.png':
-            print("optimizing "+file+"..."),
+            print("optimizing "+file+"...")


Trailing comma in python2 outputs space instead of newline. I think equivalent in python3 would be

print("optimizing {}...".format(file), end=' ')

thanks. Fixed

Was this fixed?

ryanofsky · 2017-12-20T16:09:42Z

contrib/devtools/optimize-pngs.py

                sys.exit(1)

            fileMetaMap['psize'] = os.path.getsize(file_path)
            outputArray.append(fileMetaMap)
-            print("done\n"),
+            print("done\n")


equivalent would be print("done")

thanks. Fixed

ryanofsky · 2017-12-20T16:13:52Z

contrib/macdeploy/macdeployqtplus

            output, unused_err = process.communicate()
            retcode = process.poll()
            if retcode:
                cmd = kwargs.get("args")
                if cmd is None:
                    cmd = popenargs[0]
-                raise CalledProcessError(retcode, cmd)


This was just broken before, I guess? I don't see how it could have worked.

laanwj · 2018-02-23T16:37:43Z

Needs rebase, I think we should do this for 0.17.

droark · 2018-02-23T17:19:47Z

Just for cross-referencing, the mac_alias patch made it into the mainline and was removed from Core here. I pinged the biplist maintainer. He said he'd been busy and would take a look at my PR eventually.

practicalswift · 2018-02-23T22:12:15Z

@laanwj Excellent! Let's get rid of Python 2!

droark · 2018-02-26T03:54:53Z

Hi. Andrew (the biplist maintainer) reviewed the Core patch and rejected it. Here's his rationale. Any thoughts? I admit that I'm not super familiar with the biplist codebase, so it would take a little time for me to come up with a coherent response.

(Technically, this isn't a Py3 issue, but since I brought it up here earlier....)

I looked into this, and it looks like v1.0.2 added sorting of sets and dictionary keys when writing, which is the change needed for reproducible builds. The patch from Bitcoin Core targets biplist v0.9, which didn't have this sorting.

The below two changes from the Bitcoin Core patch seem to have any effect when writing plists, AFAICT (please correct me if I am wrong). One is wrapping the root dict in a hashable wrapper, so sorting the keys doesn't matter. The other is for computing some values in the file trailer related to how many unique objects there are in the file and how many bytes it takes to represent the offsets to those objects within the file.

That said, this is under-tested, so I added a test for what I expect the output to be.

Thanks!

EDIT 1: I think the "two changes" comment had to do with this commit, which is the 1.0.2 commit that supposedly adds sorting.

EDIT 2: biplist has been sorted out.

jnewbery · 2018-02-27T15:36:34Z

Rebased and addressed @ryanofsky's review comments.

Since this marked WIP, it might be good to update the PR description to say what other changes are underway or planned.

This is marked as WIP since I was looking for initial feedback and haven't tested or carefully reviewed my suggested changes. Since there seems to be some enthusiasm for removing python2 support, I'll clean this PR up.

jtimon · 2018-02-27T21:25:29Z

Concept ACK

eklitzke · 2018-03-11T09:03:26Z

Concept ACK.

wrt the comments from @droark, it's easy to wrap iterator types using sorted() or types like collections.OrderedDict(). If reproducibility is an issue then we should handle that, but that's possible to achieve in Py3 (and if Py2 code relies on it, that code is broken anyway).

ryanofsky · 2018-03-12T22:22:20Z

utACK 4f3238250bc01eedee7c60b4d6681d3e21c6783c. Changes since last review were porting check-doc.py, fixing newlines for prints with trailing commas, updating copyright years.

practicalswift · 2018-03-12T22:27:38Z

utACK 4f3238250bc01eedee7c60b4d6681d3e21c6783c

maflcko · 2018-03-13T16:28:31Z

contrib/macdeploy/macdeployqtplus

@@ -203,7 +202,7 @@ def getFrameworks(binaryPath, verbose):
    if verbose >= 3:
        print("Inspecting with otool: " + binaryPath)
    otoolbin=os.getenv("OTOOL", "otool")
-    otool = subprocess.Popen([otoolbin, "-L", binaryPath], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    otool = subprocess.Popen([otoolbin, "-L", binaryPath], stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
    o_stdout, o_stderr = otool.communicate()


You changed this to universal_newlines, so the type should be str.

Thus, the decode a couple of lines down will fail.

Thanks. Fixed

jnewbery · 2018-03-28T17:36:14Z

huh. That's confusing. All the other calls to subprocess.Popen expect stdout to be bytes, but line 124 expects stdout to be a string. Will open a fixup PR.

jnewbery · 2018-03-28T17:38:34Z

@ken2812221 thanks for reporting this. Can you try again with https://github.com/jnewbery/bitcoin/tree/python3_fixup and let me know if it resolves your issue?

ken2812221 · 2018-03-28T18:17:17Z

@jnewbery After apply your commit

+ make -j4 -C src check-security
make: Entering directory `/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
Checking binary security...
Traceback (most recent call last):
  File "../contrib/devtools/security-check.py", line 201, in 
    if not func(filename):
  File "../contrib/devtools/security-check.py", line 144, in check_PE_DYNAMIC_BASE
    (arch,bits) = get_PE_dll_characteristics(executable)
  File "../contrib/devtools/security-check.py", line 133, in get_PE_dll_characteristics
    arch = tokens[1].rstrip(',')
TypeError: 'str' does not support the buffer interface
make: *** [check-security] Error 1
make: Leaving directory `/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'

ken2812221 · 2018-03-28T18:21:35Z

It also fails on building linux
https://gist.github.com/ken2812221/5ef0feec721f48867bbd23b92593e508

jnewbery · 2018-03-28T18:35:55Z

Thanks @ken2812221 . I've pushed a new commit. Can you retry?

ken2812221 · 2018-03-28T19:07:54Z

+ make -j4 -C src check-security
make: Entering directory `/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'
Checking binary security...
Traceback (most recent call last):
  File "../contrib/devtools/security-check.py", line 192, in 
    etype = identify_executable(filename)
  File "../contrib/devtools/security-check.py", line 182, in identify_executable
    if magic.startswith('MZ'):
TypeError: startswith first arg must be bytes or a tuple of bytes, not str
make: *** [check-security] Error 1
make: Leaving directory `/home/ubuntu/build/bitcoin/distsrc-i686-w64-mingw32/src'

jnewbery · 2018-03-28T19:51:01Z

New branch pushed. Can you try again?

maflcko · 2018-03-28T20:10:58Z

contrib/devtools/optimize-pngs.py

-                pngCrushOutput = subprocess.check_output(
-                        [pngcrush, "-brute", "-ow", "-rem", "gAMA", "-rem", "cHRM", "-rem", "iCCP", "-rem", "sRGB", "-rem", "alla", "-rem", "text", file_path],
-                        stderr=subprocess.STDOUT).rstrip('\n')
+                subprocess.call([pngcrush, "-brute", "-ow", "-rem", "gAMA", "-rem", "cHRM", "-rem", "iCCP", "-rem", "sRGB", "-rem", "alla", "-rem", "text", file_path],


Post-merge nit: Should probably use check_call instead.

ken2812221 · 2018-03-28T20:26:47Z

Building windows is OK, but buiding linux still fail at check-symbols as I post on gist

jnewbery · 2018-03-28T20:59:17Z

Please retry check-symbols.py with latest commit.

ken2812221 · 2018-03-29T02:21:44Z

This commit really works. Thanks. John Newbery <notifications@github.com> 於 2018年3月29日週四上午5:00 寫道：

…

Please retry check-symbols.py with latest commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11881 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKoyxmr-s3wbujdQem_HsInxNN1Agzk7ks5ti_nagaJpZM4Q_imm> .

05120ee contrib: Remove unused import string (MarcoFalke) Pull request description: Tiny oversight in #11881, that broke travis. Tree-SHA512: 805c31cbbd74cf0a6c71bf1a7989c3ff1d179e24d434b70de307f20ddf358f6db8c2e08aa46dcaf2c75a44ae571b1dcf5ba15184be81680e19a13ad5a23a0991

jnewbery · 2018-03-29T14:18:11Z

@ken2812221 , thanks so much for your patience and help here. I've opened a PR to fix symbol-check and security-check: #12829

practicalswift · 2018-03-29T14:31:30Z

Goodbye Python 2. Welcome Python 3!

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 #11881. Fix them. Tree-SHA512: 86de3d6dc3292b1ae4cc04c2d7d7dbbf39c9270551d7b224b8d8b19e3184c30c897dbf823200403706d06bb405c0decad5cfd690cb2c0312992a235a4ffcf6bf

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

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. Tree-SHA512: 86de3d6dc3292b1ae4cc04c2d7d7dbbf39c9270551d7b224b8d8b19e3184c30c897dbf823200403706d06bb405c0decad5cfd690cb2c0312992a235a4ffcf6bf

fa0f92a build: depends: Switch to python3 (MarcoFalke) Pull request description: Actually try to switch to python3 after the incomplete attempts: * Remove Python2 support bitcoin#11881 * build: Require python 3.5 bitcoin#14954 * ... Tree-SHA512: ba689c3788f2dd91c15d4ff5f6a73219c1a73893c18d3bb8e6da5c35acfef9897c7e100439ce5cac05624c66f7619d13528b60065c36545608fb387aab25e117

* 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

fanquake added the Brainstorming label Dec 12, 2017

ryanofsky reviewed Dec 20, 2017

View reviewed changes

laanwj added this to the 0.17.0 milestone Feb 23, 2018

jnewbery force-pushed the remove_python2 branch from bbe67bd to 3b1467c Compare February 27, 2018 15:33

jnewbery force-pushed the remove_python2 branch 2 times, most recently from 8096152 to 4f32382 Compare February 28, 2018 15:32

maflcko changed the title ~~[WIP] [concept] Remove Python2 support~~ [contrib] Remove Python2 support Mar 13, 2018

maflcko reviewed Mar 13, 2018

View reviewed changes

maflcko added Scripts and tools and removed Brainstorming labels Mar 13, 2018

maflcko mentioned this pull request Mar 28, 2018

contrib: Remove unused import string #12821

Merged

maflcko reviewed Mar 28, 2018

View reviewed changes

jnewbery mentioned this pull request Mar 29, 2018

Python3 fixup #12829

Merged

jnewbery mentioned this pull request Apr 13, 2018

Add python3 script shebang lint #12972

Merged

HashUnlimited mentioned this pull request Aug 2, 2018

Python3 fixup chaincoin/chaincoin#113

Merged

maflcko mentioned this pull request Feb 21, 2019

build: Require python 3.5 #14954

Merged

maflcko mentioned this pull request Mar 14, 2019

build: depends: Switch to python3 (take 3) #15601

Merged

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

Remove Python2 support #11881

Remove Python2 support #11881

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Dec 12, 2017

Uh oh!

jnewbery commented Dec 12, 2017

Uh oh!

droark commented Dec 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

laanwj commented Dec 19, 2017

Uh oh!

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

Uh oh!

Uh oh!

fanquake commented Dec 19, 2017

Uh oh!

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

Uh oh!

Uh oh!

droark commented Dec 19, 2017

Uh oh!

droark commented Dec 19, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 23, 2018

Uh oh!

droark commented Feb 23, 2018

Uh oh!

practicalswift commented Feb 23, 2018

Uh oh!

droark commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Feb 27, 2018

Uh oh!

jtimon commented Feb 27, 2018

Uh oh!

eklitzke commented Mar 11, 2018

Uh oh!

ryanofsky commented Mar 12, 2018

Uh oh!

practicalswift commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 28, 2018

Uh oh!

jnewbery commented Mar 28, 2018

Uh oh!

ken2812221 commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jnewbery commented Dec 12, 2017 •

edited

Loading

practicalswift commented Dec 12, 2017 •

edited

Loading

maflcko commented Dec 12, 2017 •

edited

Loading

droark commented Dec 19, 2017 •

edited

Loading

laanwj commented Dec 19, 2017 via email •

edited

Loading

droark commented Feb 26, 2018 •

edited

Loading

ken2812221 commented Mar 28, 2018 •

edited

Loading