gyp: pull Python 3 changes from node/node-gyp by cclauss · Pull Request #28573 · nodejs/node

cclauss · 2019-07-06T12:29:29Z

#28563 is too complicated for easy review. It touches 32 files some of which contain complex changes. This PR is a subset of #28563 which brings across 16 files (the easy half) that only contain Python 3 compatibility modifications. Once this PR is merged, we can rebase #28563 and focus our attention on the complex half.

The commands used to create this PR are below...

Order of execution: This PR (#28573), then #28563, then #28537, then #25878

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

cd node-gyp
git checkout master
git pull --rebase upstream master
git push origin master

cd ../node
git checkout master
git pull --rebase upstream master
git push origin master

git checkout -b gyp-pull-python3-changes
cd ..
SRC_DIR="node-gyp/gyp/"
DST_DIR="node/tools/gyp/"
ls ${SRC_DIR}
ls ${DST_DIR}

cp -r ${SRC_DIR}PRESUBMIT.py ${DST_DIR}
cp -r ${SRC_DIR}tools        ${DST_DIR}
cp -r ${SRC_DIR}pylib/gyp/MSVSNew.py      ${DST_DIR}pylib/gyp/
cp -r ${SRC_DIR}pylib/gyp/MSVSUserFile.py ${DST_DIR}pylib/gyp/
cp -r ${SRC_DIR}pylib/gyp/__init__.py     ${DST_DIR}pylib/gyp/
cp -r ${SRC_DIR}pylib/gyp/flock_tool.py   ${DST_DIR}pylib/gyp/
cp -r ${SRC_DIR}pylib/gyp/generator/analyzer.py              ${DST_DIR}pylib/gyp/generator/
cp -r ${SRC_DIR}pylib/gyp/generator/dump_dependency_json.py  ${DST_DIR}pylib/gyp/generator/
cp -r ${SRC_DIR}pylib/gyp/generator/eclipse.py               ${DST_DIR}pylib/gyp/generator/
cp -r ${SRC_DIR}pylib/gyp/generator/gypd.py                  ${DST_DIR}pylib/gyp/generator/
cp -r ${SRC_DIR}pylib/gyp/input_test.py   ${DST_DIR}pylib/gyp/
cp -r ${SRC_DIR}pylib/gyp/ordered_dict.py ${DST_DIR}pylib/gyp/
cp -r ${SRC_DIR}pylib/gyp/simple_copy.py  ${DST_DIR}pylib/gyp/

cd node
code tools/gyp/tools/pretty_vcproj.py  # fix print(("Usage:")) --> print("Usage:")
git commit -am"gyp: pull Python 3 changes from node/node-gyp"
git push --set-upstream origin gyp-pull-python3-changes
echo "Done."

nodejs-github-bot · 2019-07-06T12:43:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24315/

Trott · 2019-07-06T17:14:44Z

@nodejs/python @nodejs/gyp

cclauss · 2019-07-06T23:08:39Z

@MattIPv4 You review of these changes please.

MattIPv4 · 2019-07-07T09:40:17Z

@cclauss What is the aim with this? Is support for 2.7 intended still, or is this a complete move to 3.x?

cclauss · 2019-07-07T09:48:27Z

Both Python and legacy Python must be supported.

MattIPv4 · 2019-07-07T09:53:16Z

@cclauss Is legacy Python simply regarded as 2.7 now and not older versions that were supported before?

cclauss · 2019-07-07T10:07:57Z

Yes. See BUILDING.md.

tools/gyp/PRESUBMIT.py

tools/gyp/pylib/gyp/__init__.py

MattIPv4

Lgtm.

sam-github

RSLGTM

Since this drops support Python 2.6 and below I labelled it dont-land 10.x and 8.x, which document support for 2.6 in their BUILDING.md. Someone can correct that if I misunderstand.

AFAICT, there is a bug in core-validate-commit, see nodejs/core-validate-commit#65

You could use tools: as a sub-system to make Travis happy, it can be patched if necessary when this is landed.

sam-github · 2019-07-08T18:35:26Z

@nodejs/gyp Needs one more approval to land (or will have to wait 114 more hours...).

@cclauss FYI, your github profile and your commit lack identifying information. Its not absolutely required, but it is helpful to be able to attribute commits to specific humans.

----------------------------------- PR info ------------------------------------
Title      gyp: pull Python 3 changes from node/node-gyp (#28573)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     cclauss:gyp-pull-python3-changes -> nodejs:master
Labels     build, dont-land-on-v10.x, dont-land-on-v8.x, gyp, python, tools
Committers 1
 - cclauss <cclauss@me.com>

cclauss · 2019-07-10T09:18:20Z

How do we make progress on this? Are there any Pythonistas in the project that we should add as reviewers? @nodejs/python

sam-github · 2019-07-10T21:24:35Z

@bnoordhuis You may be the sole member of https://github.com/orgs/nodejs/teams/gyp that is still active on the project, PTAL

BridgeAR · 2019-07-11T14:40:58Z

tools/gyp/tools/pretty_gyp.py

-      brace_diff = 0
-      if not COMMENT_RE.match(line):
+    if COMMENT_RE.match(line):
+      print(line)


Does this not change the behavior? Before, it used to strip '\r\n\t ' from the line?

It looks like this is still doing that at 126 - This just now prints the line if it matches the regex, otherwise strips - Unlike before where it stripped it if failed the regex immediately.

PR-URL: nodejs#28573 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

sam-github · 2019-07-11T16:35:03Z

Landed in 5ebaf70

PR-URL: #28573 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#28573 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> (cherry picked from commit 5ebaf70)

gyp: pull Python 3 changes from node/node-gyp

eb662da

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Jul 6, 2019

cclauss requested review from Trott, bzoz, refack, richardlau, rvagg and sam-github July 6, 2019 12:30

cclauss added gyp Issues and PRs related to the GYP tool and .gyp build files python PRs and issues that require attention from people who are familiar with Python. labels Jul 6, 2019

cclauss requested a review from thefourtheye July 7, 2019 06:33

MattIPv4 reviewed Jul 7, 2019

View reviewed changes

tools/gyp/PRESUBMIT.py Show resolved Hide resolved

tools/gyp/pylib/gyp/__init__.py Show resolved Hide resolved

MattIPv4 approved these changes Jul 7, 2019

View reviewed changes

sam-github added dont-land-on-v8.x labels Jul 8, 2019

sam-github mentioned this pull request Jul 8, 2019

subsystem: allow gyp as a subsystem nodejs/core-validate-commit#65

Merged

sam-github approved these changes Jul 8, 2019

View reviewed changes

MattIPv4 mentioned this pull request Jul 8, 2019

Node 12 support MagicCap/MagicCap#182

Closed

cclauss requested review from addaleax and joaocgreis July 9, 2019 19:52

BridgeAR approved these changes Jul 11, 2019

View reviewed changes

pull bot pushed a commit to SimenB/node that referenced this pull request Jul 11, 2019

gyp: pull Python 3 changes from node/node-gyp

5ebaf70

PR-URL: nodejs#28573 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

sam-github closed this Jul 11, 2019

cclauss deleted the gyp-pull-python3-changes branch July 11, 2019 16:37

MattIPv4 mentioned this pull request Jul 11, 2019

gyp: cherrypick more Python3 changes from node-gyp #28563

Closed

4 tasks

targos pushed a commit that referenced this pull request Jul 20, 2019

gyp: pull Python 3 changes from node/node-gyp

b1db810

PR-URL: #28573 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This was referenced Jul 23, 2019

v12.7.0 release proposal #28816

Closed

v12.7.0 release proposal #28817

Merged

Uh oh!

Conversation

cclauss commented Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jul 6, 2019

Uh oh!

Trott commented Jul 6, 2019

Uh oh!

cclauss commented Jul 6, 2019

Uh oh!

MattIPv4 commented Jul 7, 2019

Uh oh!

cclauss commented Jul 7, 2019

Uh oh!

MattIPv4 commented Jul 7, 2019

Uh oh!

cclauss commented Jul 7, 2019

Uh oh!

Uh oh!

Uh oh!

MattIPv4 left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jul 8, 2019

Uh oh!

cclauss commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Jul 10, 2019

Uh oh!

BridgeAR Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

MattIPv4 Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cclauss commented Jul 6, 2019 •

edited

Loading

sam-github left a comment •

edited

Loading

cclauss commented Jul 10, 2019 •

edited

Loading