Upgrade node gyp with fixes

[DeeDeeG]

Description of the Change

This branch contains the commits from #875, along with some fixes to make more of the tests pass, as described at length here: #875 (comment)

A couple of remaining tests need to be re-written, at which point all tests will pass with Python 2.

Note: About 12 tests are still not passing when using Python 3!!! [Edit: Fixed!]

Alternate Designs

This is just an attempt to iterate on #875, so I don't have an alternate design to describe here.

(Other than... "This, but with passing tests! Including with Python 3!")

Benefits

Passes several more tests than #875

Possible Drawbacks

Still needs a couple tests re-written for Python 2. and more debugging to pass all tests with Python 3
[Edit: Python 3 issues are resolved. Discussion of broken/invalidated Python 2 tests is ongoing.]

Travis CI config should probably be updated to test using Python 3, in addition to the current default python, Python 2.

Verification Process

Lots of testing with ./bin/npm test.

All but the still-broken tests should pass in Travis CI. (Expected to be: 4 failures in 2 tests.)

Applicable Issues

See also #875 for a bit of prior discussion.

[DeeDeeG]

There's some tests I don't know how to re-write:

apm/spec/install-spec.coffee

Lines 570 to 610 in 707cd2a

	describe "configurable Python binaries", ->
	[originalPython, originalNpmConfigPython] = []
	beforeEach ->
	originalPython = process.env.PYTHON
	originalNpmConfigPython = process.env.npm_config_python
	delete process.env.PYTHON
	delete process.env.npm_config_python
	afterEach ->
	process.env.PYTHON = originalPython
	process.env.npm_config_python = originalNpmConfigPython
	it 'respects ${PYTHON} if set', ->
	process.env.PYTHON = path.join __dirname, 'fixtures', 'fake-python-1.sh'
	callback = jasmine.createSpy('callback')
	apm.run(['install', 'native-package'], callback)
	waitsFor 'waiting for install to fail', 600000, ->
	callback.callCount is 1
	runs ->
	errorText = callback.mostRecentCall.args[0]
	expect(errorText).not.toBeNull()
	expect(errorText).toMatch('fake-python-1 called')
	it 'respects ${npm_config_python} if set', ->
	process.env.npm_config_python = path.join __dirname, 'fixtures', 'fake-python-2.sh'
	process.env.PYTHON = path.join __dirname, 'fixtures', 'fake-python-1.sh'
	callback = jasmine.createSpy('callback')
	apm.run(['install', 'native-package'], callback)
	waitsFor 'waiting for install to fail', 600000, ->
	callback.callCount is 1
	runs ->
	errorText = callback.mostRecentCall.args[0]
	expect(errorText).not.toBeNull()
	expect(errorText).toMatch('fake-python-2 called')

I'm not very fluent with the Node ecosystem, and I'm struggling to write the test in a way that checks if --python or npm_config_python works.

As far as I can tell, the old error behavior of node-gyp was easier to read programmatically. And also, newer node-gyp auto-detects python, python2 and python3 on the system... it can also tell if a program is a real Python executable, and prefers any real one it can find over an invalid Python program, even if the fake one is manually passed in with --python= or with npm_config_python=.

I do think the functionality works, I'm just unsure how to prove it within a test.

[DeeDeeG]

As for the additional failures on Python 3, I'm seeing a lot of this:

TypeError: '>=' not supported between instances of 'str' and 'int' while trying to load binding.gyp

[smashwilson]

I do think the functionality works, I'm just unsure how to prove it within a test.

Hmm. Maybe copy a real, valid Python interpreter to a test path, and try to check that... ? But I don't know how you'd verify that the correct binary was actually executed.

Honestly, it might be okay to just delete the test. As long as we're passing the environment variables along correctly, we're really testing node-gyp instead of apm at that point. 🤔

[DeeDeeG]

You may be interested to know that the commit I just pushed earlier resolves the failures unique to Python 3.

(Edit to explain: Node v0.10.3 + modern node-gyp + Python 3 = errors. A currently supported release of Node works just fine.)

i.e. the only things left to do here are the two tests I was complaining about/which your comment just now is referring to.

[DeeDeeG]

Whoops, I forgot to commit an updated BUNDLED_NODE_VERSION to this branch last time I pushed.

Tests should be passing now on Python 2 & 3, other than the "respects configurable python" tests as discussed above.

(There were many other things I tried on another branch that aren't needed, i.e. updating most dependencies and making code tweaks to match breaking changes in dependencies, so I left them out to keep this PR focused on one thing.)

[DeeDeeG]

I tried enabling the --verbose argument to pass on to npm install. See this commit: DeeDeeG@a353c7f

This gives a lot of output to the console, particularly this interesting stuff about finding/picking a Python executable:

$ export PYTHON="../spec/fixtures/fake-python-1.sh"
$ ../bin/apm install --verbose
[...snipped...]
gyp verb find Python Python is not set from command line or npm configuration
gyp verb find Python checking Python explicitly set from environment variable PYTHON
gyp verb find Python - process.env.PYTHON is "../spec/fixtures/fake-python-1.sh"
gyp verb find Python - executing "../spec/fixtures/fake-python-1.sh" to get executable path
gyp verb find Python - "../spec/fixtures/fake-python-1.sh" is not in PATH or produced an error
gyp verb find Python checking if "python" can be used
gyp verb find Python - executing "python" to get executable path
gyp verb find Python - executable path is "/usr/bin/python"
gyp verb find Python - executing "/usr/bin/python" to get version
gyp verb find Python - version is "2.7.18"
gyp info find Python using Python version 2.7.18 found at "/usr/bin/python"
[...snipped...]

I am only blocked at this point by not knowing how to redirect that text from stdout/console.log output into some variable. Or any way to compare the text to an expected value.

I think this (or similar) should work as the check/comparison logic: expect(output_python_text_here).toMatch('/executing\\ \"\.\./spec/fixtures/fake-python-1\.sh\"/')

[DeeDeeG]

In other words, this looks easily fixable, I just don't know how to capture stdout. 🙊 Seems like an easy thing to do?

(I'm not versed in the JavaScript/Node APIs though, so guessing it is taking me a lot of time/effort for an arguably small pay-off.)

[DeeDeeG]

Hi again,

• I deleted the failing tests. As you mentioned, they are essentially testing node-gyp anyhow. Tests should now be passing on Python 2 and Python 3.

• As an extra thing: I added a line to clean up after the with a space test. There was a renamed copy of the nod-gyp module left around. It was making tests fail when you ran the tests more than once. Since this is technically unrelated to upgrading npm/node-gyp, I can break that out into another PR, if you want.

• I also updated the CI config files to specify the same version of Node this PR updates to, from 10.2.1 to 10.20.1.

[smashwilson]

Excellent ✨

[smashwilson]

👍

[smashwilson]

Thanks again for taking the time to take care of this after I ran out of time to!

I'll publish and ⬆️ the version in atom/atom tomorrow when I'm back on the laptop.