Skip to content

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

Closed
cclauss wants to merge 1 commit into
masterfrom
Declare-Python-3-compatibility
Closed

doc: Declare that node-gyp is Python 3 compatible#1811
cclauss wants to merge 1 commit into
masterfrom
Declare-Python-3-compatibility

Conversation

@cclauss

@cclauss cclauss commented Jul 7, 2019

Copy link
Copy Markdown
Contributor

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)

@joaocgreis joaocgreis left a comment

Copy link
Copy Markdown
Member

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

cclauss commented Jul 7, 2019

Copy link
Copy Markdown
Contributor Author

2.7 should be the lower limit.

@joaocgreis

Copy link
Copy Markdown
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

cclauss commented Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

cclauss commented Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

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.

@rvagg rvagg left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

rvagg commented Sep 25, 2019

Copy link
Copy Markdown
Member

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

@cclauss

cclauss commented Sep 25, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

rvagg commented Sep 30, 2019

Copy link
Copy Markdown
Member

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

rvagg commented Sep 30, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

This should NOT go into 5.

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

@rvagg

rvagg commented Oct 1, 2019

Copy link
Copy Markdown
Member

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