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.
Strong concept ACK!
Pros:
Cons:
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?
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.
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.
@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.
36@@ -37,7 +37,7 @@ def content_hash(filename):
37 for file in os.listdir(absFolder):
38 extension = os.path.splitext(file)[1]
39 if extension.lower() == '.png':
40- print("optimizing "+file+"..."),
41+ print("optimizing "+file+"...")
Trailing comma in python2 outputs space instead of newline. I think equivalent in python3 would be
0print("optimizing {}...".format(file), end=' ')
69 sys.exit(1)
70
71 fileMetaMap['psize'] = os.path.getsize(file_path)
72 outputArray.append(fileMetaMap)
73- print("done\n"),
74+ print("done\n")
print("done")
724 retcode = process.poll()
725 if retcode:
726 cmd = kwargs.get("args")
727 if cmd is None:
728 cmd = popenargs[0]
729- raise CalledProcessError(retcode, cmd)
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.
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.
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).
check-doc.py, fixing newlines for prints with trailing commas, updating copyright years.
201@@ -203,7 +202,7 @@ def getFrameworks(binaryPath, verbose):
202 if verbose >= 3:
203 print("Inspecting with otool: " + binaryPath)
204 otoolbin=os.getenv("OTOOL", "otool")
205- otool = subprocess.Popen([otoolbin, "-L", binaryPath], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
206+ otool = subprocess.Popen([otoolbin, "-L", binaryPath], stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
207 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.
32@@ -33,12 +33,12 @@ def main():
33 args_need_doc = args_used.difference(args_docd)
One line above this:
You are switching this file to python3, so check_output will return byte-strings instead of text strings. Thus, you can not apply a regex on them.
b'pattern'
I’ve gone through and reviewed/tested my changes, and I think they’re good. Ready for wider review.
The only python file that is not now explicitly Python3 is gen_base58_test_vectors.py. It would require some substantial modifications to work in Python3 (since it’s using string/bytes manipulation which changed completely between Python2 and Python3). That file is not modified or used very much, so I’ve left it as Python2, adding an explicit note to the doc string saying that it’s Python2 only.
116@@ -117,6 +117,11 @@ deprecated in V0.15.1, and has now been removed. Miners should use the
117 `-blockmaxweight` option if they want to limit the weight of their blocks'
118 weights.
119
120+Python Support
121+--------------
122+
123+Support for Python 2 has been discontinued for all test files and tools.
I disagree. We should remove support for a legacy language which will soon not be maintained, and which may hide subtle bugs due to incompatibilities with the new language.
What do we gain by claiming to support python2?
Concept ACK on the diff (excluding change to doc/release-notes.md)
Will review after revert.
That file is not modified or used very much, so I’ve left it as Python2, adding an explicit note to the doc string saying that it’s Python2 only.
I might port it over some time - agree it’s not relevant to this PR (as it’s never called by the test framework, build system, nor any other script.)
gitian build fails on security-check.py
subprocess.Popen expect stdout to be bytes, but line 124 expects stdout to be a string. Will open a fixup PR.
@jnewbery After apply your commit
46- pngCrushOutput = ""
47 try:
48- pngCrushOutput = subprocess.check_output(
49- [pngcrush, "-brute", "-ow", "-rem", "gAMA", "-rem", "cHRM", "-rem", "iCCP", "-rem", "sRGB", "-rem", "alla", "-rem", "text", file_path],
50- stderr=subprocess.STDOUT).rstrip('\n')
51+ subprocess.call([pngcrush, "-brute", "-ow", "-rem", "gAMA", "-rem", "cHRM", "-rem", "iCCP", "-rem", "sRGB", "-rem", "alla", "-rem", "text", file_path],
check_call instead.
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 https://github.com/bitcoin/bitcoin/pull/11881#issuecomment-377034521, or mute the thread https://github.com/notifications/unsubscribe-auth/AKoyxmr-s3wbujdQem_HsInxNN1Agzk7ks5ti_nagaJpZM4Q_imm .
Labels
Scripts and tools
Milestone
0.17.0