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.

Sign up for GitHub

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

Jump to bottom

configure: use sys.version_info to get python version #1504

Closed
wants to merge 1 commit into from
Closed

configure: use sys.version_info to get python version #1504

wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

Instead of parsing the string returned by platform.python_version(), it is easier to use the integers in sys.version_info. This avoids the need to pattern match and remove extra characters.

This is necessary because V8's infrastructure uses a Python that includes "chromium" in the version string, which would have needed to be filtered out too.

Checklist
Description of change

@hashseed
configure: use sys.version_info to get python version
50fba7f 
This is cleaner than filtering additional strings from
platform.python_version().
@hashseed hashseed mentioned this pull request Jul 17, 2018
Closed
2 tasks
maclover7
maclover7 reviewed Jul 18, 2018
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

SGTM but can you add a test case with one of the custom python version numbers?

@hashseed
Copy link
Member Author

SGTM but can you add a test case with one of the custom python version numbers?

The way existing tests work is to mock out the CLI invocation of python with something that returns a string to parse. The whole point of this PR is to make sure the CLI result no longer contains any custom version strings anymore.

Besides, there never was a test for filtering "rc" or "+" either.

@maclover7
Copy link
Contributor

@hashseed Ah sorry, misread the Git diff. Agree that since the extra bits are being pulled out, there's nothing to really check.

@maclover7
Copy link
Contributor

CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/60/

@hashseed
Copy link
Member Author

hashseed commented Aug 7, 2018

Can this be merged?

@maclover7
Copy link
Contributor

@hashseed I can merge this to master, but before that, would you be able to open a PR to backport this commit to the v3.x branch?

@rvagg rvagg closed this Aug 9, 2018
rvagg pushed a commit that referenced this pull request Aug 9, 2018
@hashseed @rvagg
configure: use sys.version_info to get python version
da5db40 
This is cleaner than filtering additional strings from
platform.python_version().

PR-URL: #1504
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Aug 9, 2018

pulled in to both master and v3.x

rvagg pushed a commit that referenced this pull request Aug 9, 2018
@hashseed @rvagg
configure: use sys.version_info to get python version
536759c 
This is cleaner than filtering additional strings from
platform.python_version().

PR-URL: #1504
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
@greenaddress
Copy link
Contributor

@hashseed @rvagg @maclover7 this broke python3, i created a PR with a small fix here #1534

@Allianzcortex
Copy link

Allianzcortex commented Oct 20, 2018 •
edited
Loading

@greenaddress @maclover7 I meet this bug too..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maclover7 maclover7 maclover7 left review comments

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hashseed @maclover7 @rvagg @greenaddress @Allianzcortex @addaleax @richardlau