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

Accept Python 3 when NODE_GYP_PYTHON3 is defined #1815

Closed
wants to merge 5 commits into from

Conversation

joaocgreis
Copy link
Member

@joaocgreis joaocgreis commented Jul 8, 2019

Checklist
  • npm install && npm test passes
  • tests are included
  • commit message follows commit guidelines
Description of change

This depends on #1812, I'll rebase when that PR lands.

This allows for Python 3 to be tested without breaking node-gyp for users.

This uses two new environment variables: NODE_GYP_FORCE_PYTHON is the only Python binary checked when it is defined (so we can be sure of what version is being tested in Travis), and NODE_GYP_PYTHON3 conditionally enables Python 3.

When Python 3 is fully supported, enabling it unconditionally should be as simple as removing NODE_GYP_PYTHON3, which is only used on one line.

This includes two commits. One just moves Python detection to a new file. The other contains the rest of the changes. Separately, they should be easy to review.

The tests that check for a "Python version too new" were deleted. Hopefully we won't need them again.

@joaocgreis
Copy link
Member Author

@rvagg
Copy link
Member

rvagg commented Jul 9, 2019

When Python 3 is fully supported

I thought we were there already? Don't we have Travis tests for this? This python garbage is doing my head in and I wish we had more actual python people working on this or at least helping review. The sooner we get to gyp.js the better.

@joaocgreis
Copy link
Member Author

joaocgreis commented Jul 9, 2019 via email

@richardlau
Copy link
Member

Thia shows that when actually using Python 3 the tests don't pass, but I don't know how hard those issues are to fix.

At least https://travis-ci.com/nodejs/node-gyp/jobs/214237690#L2234-L2241

    +Traceback (most recent call last):                                         
    +  File "/home/travis/build/nodejs/node-gyp/gyp/gyp_main.py", line 13, in <module>
    +    import gyp                                                             
    +  File "/home/travis/build/nodejs/node-gyp/gyp/pylib/gyp/__init__.py", line 10, in <module>
    +    import gyp.input                                                       
    +  File "/home/travis/build/nodejs/node-gyp/gyp/pylib/gyp/input.py", line 7, in <module>
    +    from compiler.ast import Const                                         
    +ModuleNotFoundError: No module named 'compiler'  

looks like it would be fixed by refack/GYP3@f989ef9.

@@ -4,32 +4,61 @@ cache: pip
matrix:
include:
- name: "Python 2.7 on Linux"
env: NODE_GYP_FORCE_PYTHON=python2
Copy link
Contributor

@cclauss cclauss Jul 9, 2019

Choose a reason for hiding this comment

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

On Linux runs Travis will set the environment variable $TRAVIS_PYTHON_VERSION == 2.7 so perhaps that it would be better to leverage that rather than create a new environment variable.

Copy link
Contributor

@cclauss cclauss Jul 9, 2019

Choose a reason for hiding this comment

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

As I think about it more, I now understand that you are looking for a more general solution that is useful well beyond just Travis CI. What if we name the variable NODE_GYP_PYTHON2 and set it to True in legacy Python environments and set it to False or leave it undefined in Python 3 environments? In this way we would not need to carry this baggage into future years. With just 176 days left before Python 2 end of life we should be making execution on Python 3 the default behavior and making running on Python 2 be the special case.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks for doing this.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

NODE_GYP_PYTHON3 --> NODE_GYP_PYTHON2

@joaocgreis
Copy link
Member Author

@cclauss I don't understand which variable you'd like to change. Changing NODE_GYP_FORCE_PYTHON would break the point of having it in the first place, and changing NODE_GYP_PYTHON3 would break node-gyp for end users. My goal with these variables is:

  • NODE_GYP_FORCE_PYTHON: To have a way of forcing node-gyp to use a certain Python binary and not trying anything else. This is useful in Travis to specify a Python binary to use. I understand this adds some lines to .travis.yml, but when we're sure there is no other Python version in the machines we can remove this (hopefully next year Python 2 will start to disappear). I also plan to use this to add a test where python is a bat file, which is common in Windows. I think this is a good mechanism to have, even in the future when there is only Python 3.
  • NODE_GYP_PYTHON3: Python 3 needs to be opt-in for now, because it doesn't work. If we make this NODE_GYP_PYTHON2, then every user who wants to use node-gyp would have to set this before running! I purposefully used this variable only in one line:
    semverRange: process.env.NODE_GYP_PYTHON3 ? '>=2.7.0' : '>=2.6.0 <3.0.0',
    So, when Python 3 works we can simply change that line to semverRange: '>=2.7.0', remove the variable from .travis.yml and be done with it.

@cclauss
Copy link
Contributor

cclauss commented Jul 9, 2019

I like the idea behind NODE_GYP_FORCE_PYTHON. It will not be needed on Travis CI Linux because those jobs run in virtualenvs where by definition python points to the version of Python which is specified in the python: tag. On Travis CI Mac, NODE_GYP_FORCE_PYTHON plays a very useful role because both python2 and and python3 will be present and you know the Windows story better than I do.

I would like to resist the NODE_GYP_PYTHON3 variable unless we have an agreed upon method of deprecating it. My sense is that once we introduce new configuration vocabulary, it is nearly impossible to drop it. For example, our README.md clearly states that we no longer support Python 2.6 yet contributors continue to try to push it back into our scripts.

As I wrote above, Python 3 should the default and Python 2 should be the opt-in. Python 3 has been available for 8+ years and is the present and future of the language. Python 2 is legacy and is no longer supported at yearend. Therefore, I would much rather see us introduce a variable called NODE_GYP_PYTHON2 and force those who wish to remain on legacy Python to set that variable. I advocate that continuing to rely on a soon-to-be-EOLed platform should to be a deliberate, conscious, and documented choice.

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: process.env.NODE_GYP_PYTHON3 ? '>=2.7.0' : '>=2.6.0 <3.0.0',
Copy link
Contributor

@cclauss cclauss Jul 9, 2019

Choose a reason for hiding this comment

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

Please remove v2.6.0 to align with this project's README.md.

For this project:

  • Python2 is >=2.7.0 <3.0.0
  • Python3 is >=3.5.0

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only to extend the range of acceptable versions, not select which version to use.

@joaocgreis
Copy link
Member Author

@cclauss I might be missing something here, but I don't understand where you're trying to get to. Node-gyp does not work with Python 3, as Travis shows. By making Python 3 the default, node-gyp will be broken by default for every user. How is that acceptable?

@cclauss
Copy link
Contributor

cclauss commented Jul 9, 2019

This PR is supposed to make node-gyp Python 3 compatible. We all already thought that it was with all the tests passing until you demonstrated to us that find_python.js was actually silently preventing us from finding Python. That script was hardcoded to legacy which is exactly want I would like us to avoid continuing in this PR. I have been on this mission for a year now... I also worked on https://github.com/refack/GYP which was more advanced in Python 3 support but unfortunately was not adopted.

@joaocgreis
Copy link
Member Author

This PR does not make node-gyp Python 3 compatible, it only allows testing with it.

find-python.js is doing what it's supposed to do, produces a lot of logging at several levels and clearly indicates what version of Python it chose and why. If you run node-gyp locally on any project, you'll see that output. I also added it to Travis here.

The only test that actually runs node-gyp end-to-end is test-addon.js. Those tests are not supposed to have any output, so they're not suitable to test this kind of change. With this PR we can explicitly select an executable to run, which should allow us to reliably test different versions of Python from now on.

@cclauss it's great that you can put some effort into making node-gyp Python 3 compatible, thank you for moving that forward. I can't look into it myself, but I think this is a very important goal and that's why I'm trying to help where I can. With all changes that happened so far, I was not finding it surprising that node-gyp was already compatible until I started looking into the Windows failures. I hope that even if this is a bit disappointing, there won't be much left to do.

This allows us to start testing Python 3 without breaking node-gyp
for users.

This also adds support for NODE_GYP_FORCE_PYTHON, which will be the
only Python binary acceptable when defined.
@joaocgreis joaocgreis force-pushed the joaocgreis-J77-python3 branch 2 times, most recently from ea08a83 to e2c75b4 Compare July 9, 2019 20:17
@cclauss
Copy link
Contributor

cclauss commented Jul 9, 2019

The ModuleNotFoundError: No module named 'compiler' errors are caused by the fact that the compiler package was removed in Python 3 in favor of the ast module that exists in both Python 2 and Python 3.

refack/GYP3@f989ef9

@joaocgreis
Copy link
Member Author

I renamed NODE_GYP_PYTHON3 to EXPERIMENTAL_NODE_GYP_PYTHON3, I hope this helps it be clear that the variable is not to be used by end users, only for testing. Also excluded outdated versions 3.0-3.4 of Python if it is used.

@cclauss when this lands (or even before by depending on this), fixing that error should require only picking those changes and adapting for any possible difference.

@cclauss
Copy link
Contributor

cclauss commented Jul 10, 2019

Thanks for our WebEx on this topic and the resulting changes that you have made. If you can fix the Travis CI Py2 issues then I will give this PR a LGTM.

It is a bummer that our Py3 tests now fail but at least with PR we are able to run the desired version of Python. We can leave the Py3 Travis tests in allow_failures mode in this PR and then we can work to make those tests green in future PRs.

@joaocgreis
Copy link
Member Author

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice work here! Thanks.

joaocgreis added a commit that referenced this pull request Jul 11, 2019
PR-URL: #1815
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
joaocgreis added a commit that referenced this pull request Jul 11, 2019
This allows us to start testing Python 3 without breaking node-gyp
for users.

This also adds support for NODE_GYP_FORCE_PYTHON, which will be the
only Python binary acceptable when defined.

PR-URL: #1815
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@joaocgreis
Copy link
Member Author

Landed in bb92c76...66ad305

@joaocgreis joaocgreis closed this Jul 11, 2019
@cclauss
Copy link
Contributor

cclauss commented Jul 11, 2019

Why have we created a new NODE_GYP_FORCE_PYTHON environment variable when there is already a well documented gyp --python option? Do they do the same thing or different things? Which one takes precedence?

@joaocgreis
Copy link
Member Author

The --python option is only a hint, it will be used if it can be used. If it can't be used, node-gyp will look for something else. I can elaborate on the rationale of how this works a bit:

  • Node-gyp also checks the PYTHON environment variable. As I understand it, this variable is quite common and might be defined for some other project that users are working on, or might even be a forgotten variable from some past project that was never undefined. Therefore, I don't think it makes sense for node-gyp to fail immediately if it points to an invalid version of Python, it should keep looking for a Python of the right version.
  • The --python option is the same as defining NPM_CONFIG_PYTHON or setting python in npm config. This is a harder case to make, but I think the same logic should (and currently does) apply here. We have plenty of users who are not familiar with node-gyp, and at some point in the past might have set the npm configuration for some reason (actually, we still recommend this in the README, and shouldn't remove until v5.0.0 becomes the default). If this Python is of the versions that we accept, we use it. If not, just look for something else. I think most users want node-gyp to work and not have to worry about this.
  • The NODE_GYP_FORCE_PYTHON is not a hint. It's either this or nothing. This variable is only used for node-gyp.

Note that the verbose log output shows clearly what is used, what can't be used, by what order and why.

Precedence works like this:

  • If NODE_GYP_FORCE_PYTHON is defined, check it's version and use it, or fail. Stop here, don't look for anything else.
  • Check --python/NPM_CONFIG_PYTHON.
  • Check PYTHON.
  • Look for a python in the PATH.
  • Look for a python2 in the PATH.
  • Try to use the py launcher on Windows.
  • Try the default locations on Windows.

The code is here:

function getChecks () {
if (this.env.NODE_GYP_FORCE_PYTHON) {
return [ {
before: () => {
this.addLog(
'checking Python explicitly set from NODE_GYP_FORCE_PYTHON')
this.addLog('- process.env.NODE_GYP_FORCE_PYTHON is ' +
`"${this.env.NODE_GYP_FORCE_PYTHON}"`)
},
check: this.checkCommand,
arg: this.env.NODE_GYP_FORCE_PYTHON
} ]
}
var checks = [
{
before: () => {
if (!this.configPython) {
this.addLog(
'Python is not set from command line or npm configuration')
return SKIP
}
this.addLog('checking Python explicitly set from command line or ' +
'npm configuration')
this.addLog('- "--python=" or "npm config get python" is ' +
`"${this.configPython}"`)
},
check: this.checkCommand,
arg: this.configPython
},
{
before: () => {
if (!this.env.PYTHON) {
this.addLog('Python is not set from environment variable ' +
'PYTHON')
return SKIP
}
this.addLog('checking Python explicitly set from environment ' +
'variable PYTHON')
this.addLog(`- process.env.PYTHON is "${this.env.PYTHON}"`)
},
check: this.checkCommand,
arg: this.env.PYTHON
},
{
before: () => { this.addLog('checking if "python" can be used') },
check: this.checkCommand,
arg: 'python'
},
{
before: () => { this.addLog('checking if "python2" can be used') },
check: this.checkCommand,
arg: 'python2'
}
]
if (this.win) {
checks.push({
before: () => {
this.addLog(
'checking if the py launcher can be used to find Python 2')
},
check: this.checkPyLauncher
})
for (var i = 0; i < this.winDefaultLocations.length; ++i) {
const location = this.winDefaultLocations[i]
checks.push({
before: () => {
this.addLog('checking if Python is ' +
`${location}`)
},
check: this.checkExecPath,
arg: location
})
}
}
return checks
}
I wrote that hoping that at least that part would be easy to read and extend.

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.

4 participants