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

build: set Python semverRange: '>=2.7.0 <3.8.0' #1813

Closed
wants to merge 2 commits into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jul 7, 2019

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Based on the discussion #1811 let's try really running the tests on Python 3. This currently breaks that macOS builds.

lib/configure.js Outdated
@@ -362,7 +362,7 @@ PythonFinder.prototype = {
log: logWithPrefix(log, 'find Python'),
argsExecutable: ['-c', 'import sys; print(sys.executable);'],
argsVersion: ['-c', 'import sys; print("%s.%s.%s" % sys.version_info[:3]);'],
semverRange: '>=2.6.0 <3.0.0',
semverRange: '>=2.7.0 <3.8.0',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need an upper bound on the semver check anymore? It was there before to exclude Python 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it is semverRange: '>=2.7.0'

@cclauss
Copy link
Contributor Author

cclauss commented Jul 8, 2019

These tests would be more helpful if they printed argsVersion on failure but I am not sure how to add that functionality.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Needs a rebase after #1794 but that's purely stylistic. Thanks for doing this, @cclauss.

@cclauss
Copy link
Contributor Author

cclauss commented Jul 8, 2019

Given the "purely stylistic" changes in #1794, should I also whack our whole codebase thru python/black?

@cclauss cclauss requested a review from sam-github July 8, 2019 19:40
@@ -29,6 +29,7 @@ matrix:
python: 3.7
before_install: nvm install 12
allow_failures:
- os: osx
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

lib/configure.js Outdated
argsExecutable: [ '-c', 'import sys; print(sys.executable);' ],
argsVersion: [ '-c', 'import sys; print("%s.%s.%s" % sys.version_info[:3]);' ],
semverRange: '>=2.6.0 <3.0.0',
argsExecutable: [ '-c', 'import sys; print(sys.executable)' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

is dropping the ; cosmetic, or necessary? If cosmetic, I'd prefer to leave as is to not mix cosmetic and functional changes.

@sam-github
Copy link
Contributor

other than small comments, LGTM

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

This would break node-gyp for users that don't have Python 2 installed, because node-gyp can't yet be run with Python 3. I opened #1815 as an alternative to this, enabling Python 3 to be run on Travis.

@cclauss
Copy link
Contributor Author

cclauss commented Jul 9, 2019

Closing in favor of #1815

@cclauss cclauss closed this Jul 9, 2019
@cclauss cclauss deleted the really-python-3 branch July 9, 2019 07:11
@cclauss cclauss added the Python label Oct 4, 2019
rvagg added a commit that referenced this pull request Oct 4, 2019
Dropped accidentally in backport @
#1910

The change to >=2.7 was declared semver-major in
#1813
rvagg added a commit that referenced this pull request Oct 4, 2019
Dropped accidentally in backport @
#1910

The change to >=2.7 was declared semver-major in
#1813
rvagg added a commit that referenced this pull request Oct 4, 2019
Dropped accidentally in backport @
#1910

The change to >=2.7 was declared semver-major in
#1813
rvagg added a commit that referenced this pull request Oct 4, 2019
Dropped accidentally in backport @
#1910

The change to >=2.7 was declared semver-major in
#1813
rvagg added a commit that referenced this pull request Oct 4, 2019
Dropped accidentally in backport @
#1910

The change to >=2.7 was declared semver-major in
#1813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants