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

doc: Declare that node-gyp is Python 3 compatible #1811

Closed
wants to merge 1 commit into from

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Jul 7, 2019

Edit: Blocked because Travis CI tests on Python 3 are failing...

NOTE: node-gyp is compatible with both Python 2.7 and 3.7 but Node.js itself is not yet compatible with Python 3. See #1337 (comment)

Careful review please because I am not a Windows user.

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

Declare that node-gyp is Python 3 compatible as discussed at #1337 (comment)

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.

Blocking this for now, node-gyp only uses Python 2:

semverRange: '>=2.6.0 <3.0.0',
, thus tests are not running in Python 3. I can open a PR to remove that restriction from configure.

@cclauss
Copy link
Contributor Author

cclauss commented Jul 7, 2019

2.7 should be the lower limit.

@joaocgreis
Copy link
Member

2.7 should be the lower limit.

@cclauss is there any node-gyp reason for that? Last time I checked (some time ago) it worked. I understand Python 2.6 is not recommendable at all, but is there anything on the node-gyp side forcing us to break users who can't upgrade?

@cclauss
Copy link
Contributor Author

cclauss commented Jul 8, 2019

After 5 years with no security patches, Python 2.6 represents a rather sizable attack surface area. Also the Python 3 compatibility changes that we have made across many files have not been tested on Python 2.6 and we should not be encouraging our users to run these untested changes on unsupported configurations in production. "Stop using Python 2.6", written by a Python core team member 4+ years ago, goes into more detail.

@joaocgreis
Copy link
Member

I don't think accepting it is the same as encouraging users to use it. Perhaps we should be clear in the docs that we only test on the latest version, others may or may not work. I still think that we don't have a reason to break 2.6, but I don't really have a strong opinion so I'll defer to other @nodejs/node-gyp members.

@cclauss
Copy link
Contributor Author

cclauss commented Jul 8, 2019

I have a strong opinion that we should not be encouraging/enabling our users to expose themselves to security risks. Also, we are tied to really out-of-date technologies and this long overdue backward compatibility promise is stunting our modernization.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

Seems good to me. Great in fact.
Our pace of progress doesn't have to be dictated by nodejs/node so I'm comfortable with moving forward even if we struggle a bit in nodejs/node.

@richardlau
Copy link
Member

I don't think accepting it is the same as encouraging users to use it. Perhaps we should be clear in the docs that we only test on the latest version, others may or may not work. I still think that we don't have a reason to break 2.6, but I don't really have a strong opinion so I'll defer to other @nodejs/node-gyp members.

I can get behind dropping support for Python 2.6 at the same time as dropping support for Node.js 6 in a a node-gyp semver major. Declaring Python 3 support (assuming it does actually work) would be a positive change for the semver major.

cclauss added a commit that referenced this pull request Jul 11, 2019
@cclauss cclauss added the Python label Jul 11, 2019
rvagg pushed a commit that referenced this pull request Jul 15, 2019
As discussed in #1811

PR-URL: #1818
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@rvagg rvagg mentioned this pull request Jul 16, 2019
rvagg pushed a commit that referenced this pull request Jul 17, 2019
As discussed in #1811

PR-URL: #1818
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: João Reis <reis@janeasystems.com>
@rvagg
Copy link
Member

rvagg commented Sep 25, 2019

@cclauss @joaocgreis can you update us on the status of this? can we merge it?

@cclauss
Copy link
Contributor Author

cclauss commented Sep 25, 2019

I think we are ready for this PR to land. Of the PRs with a Python label the only one that worries me is the macOS bit of #1844 but that is semver-major.

@joaocgreis
Copy link
Member

Can we "declare that node-gyp is Python 3 compatible" if it's broken on macOS?

I can't work on macOS support myself at the moment, so I won't block this if you think it makes sense. Node-gyp would be broken by default on macOS.

NOTE: node-gyp is compatible with both Python 2.7 and 3.7 but Node.js itself is not yet compatible with Python 3.

Careful review please because I am not a Windows user.
@cclauss cclauss force-pushed the Declare-Python-3-compatibility branch from e979d59 to 779483d Compare September 30, 2019 07:51
@rvagg
Copy link
Member

rvagg commented Sep 30, 2019

is this semver-major? would it be appropriate to put on 5.x?

rvagg pushed a commit that referenced this pull request Sep 30, 2019
NOTE: node-gyp is compatible with both Python 2.7 and 3.7 but Node.js itself is not yet compatible with Python 3.

PR-URL: #1811
Reviewed-By: João Reis <reis@janeasystems.com>
@rvagg
Copy link
Member

rvagg commented Sep 30, 2019

landed in c763ca1 but if this shouldn't be in 5.x, please label it semver-major so it only goes into 6.

@rvagg rvagg closed this Sep 30, 2019
@rvagg rvagg deleted the Declare-Python-3-compatibility branch September 30, 2019 11:36
@joaocgreis
Copy link
Member

This should NOT go into 5.

This depends on #1844, so this should only go into 6.

@rvagg
Copy link
Member

rvagg commented Oct 1, 2019

Maybe we should clarify in the readme that v5 is different to >v5 in this respect? A lot of people are going to be landing at this README regardless of their version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants