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: cherrypick python3 changes to input.py from node-gyp #29140

Closed
wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Aug 15, 2019

Built on the lessons learned in #29130 if we bring all changes done to this file in node-gyp, that breaks our Travis CI tests so this PR cherrypicks only the Python 3 compatibility changes. These changes are somewhat complex because the complier module was removed in Python 3 in favor of ast which was introduced in Python 2.5.

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

@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Aug 15, 2019
@nodejs-github-bot 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 Aug 15, 2019
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubber-stamp

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

I doubt this will pass ci's linter check on the commit messages, the 2 last commits should be squashed into the first --- I'd do it, but @cclauss your branch is protected, you'll have to unprotect it, or do the squashing yourself, or perhaps it can be done on landing if ci is green, I'm not sure if we do that.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber-stamp LGTM

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

cclauss added a commit to cclauss/node that referenced this pull request Aug 19, 2019
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2019
@Trott
Copy link
Member

Trott commented Aug 20, 2019

What's the best way to land this? Squash everything into a single commit?

@gengjiawen
Copy link
Member

What's the best way to land this? Squash everything into a single commit?

+1 to squash the commit.

@cclauss
Copy link
Contributor Author

cclauss commented Aug 20, 2019

I usually squash like this combined with a well timed git rebase -i master.

However that creates merge commits and requires force push which does not work for this community. Can someone point me to the handful of commands that would allow me to squash without causing rework? A pointer here or I am @cclauss on Slack.

@bnoordhuis
Copy link
Member

@cclauss git pull --rebase && git rebase -i origin/master is what I use. That avoids the merge commit because it's basically a fast-forward.

@@ -709,6 +698,9 @@ def FixupPlatformCommand(cmd):

def ExpandVariables(input, phase, variables, build_file):
# Look for the pattern that gets expanded into variables
def to_utf8(s):
return s if isinstance(s, str) else s.decode("utf-8")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: single quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python Software Foundation is going in the opposite direction so the muscle memory is building up but will fix.

raise GypError("%s while executing command '%s' in %s" %
(e, contents, build_file))

p_stdout, p_stderr = p.communicate('')
p_stdout = to_utf8(p_stdout)
p_stderr = to_utf8(p_stderr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this might cause a runtime exception when the output contains e.g. latin1-encoded file paths?

Copy link
Contributor Author

@cclauss cclauss Aug 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. There is the old adage that you can never accurately guess encodings -- you have to be told. Should we leave as is or how should we resolve? We could catch the UnicodeDecodeError and then attempt to return s.decode('latin-1') and if we catch another UnicodeDecodeError just return s. Other approaches?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable, that probably covers 99% of everyday use.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 20, 2019

Landed in 3b92998

@Trott Trott closed this Aug 20, 2019
Trott pushed a commit that referenced this pull request Aug 20, 2019
PR-URL: #29140
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
PR-URL: #29140
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
PR-URL: nodejs#29140
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
(cherry picked from commit 3b92998)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants