Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

@cclauss
Copy link
Contributor

commented Jul 6, 2019

#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

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@MattIPv4 You review of these changes please.

@cclauss cclauss requested a review from thefourtheye Jul 7, 2019

@MattIPv4

This comment has been minimized.

Copy link

commented Jul 7, 2019

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

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Both Python and legacy Python must be supported.

@MattIPv4

This comment has been minimized.

Copy link

commented Jul 7, 2019

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

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Yes. See BUILDING.md.

Show resolved Hide resolved tools/gyp/PRESUBMIT.py
Show resolved Hide resolved tools/gyp/pylib/gyp/__init__.py
@MattIPv4
Copy link

left a comment

Lgtm.

@sam-github
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

@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>

@MattIPv4 MattIPv4 referenced this pull request Jul 8, 2019

Closed

Node 12 support #182

@cclauss cclauss requested review from joaocgreis and addaleax Jul 9, 2019

@cclauss

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

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

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jul 11, 2019

Member

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

This comment has been minimized.

Copy link
@MattIPv4

MattIPv4 Jul 11, 2019

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.

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
PR-URL: nodejs#28573
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Landed in 5ebaf70

@sam-github sam-github closed this Jul 11, 2019

@cclauss cclauss deleted the cclauss:gyp-pull-python3-changes branch Jul 11, 2019

@MattIPv4 MattIPv4 referenced this pull request Jul 11, 2019

Closed

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

1 of 4 tasks complete

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

gyp: pull Python 3 changes from node/node-gyp
PR-URL: #28573
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.