build: compile module was removed in Python 3

[cclauss]

Checklist

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

Description of change

Our Travis CI 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.

These changes are from refack/GYP3@f989ef9

@nodejs/python

[bnoordhuis]

LGTM at a quick glance. CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/143/

[richardlau]

Tests still fail with Python 3 with:
https://travis-ci.com/nodejs/node-gyp/jobs/215440829#L3070-L3075

 +gyp: Variable node -e "require('nan')" must expand to a string or list of strings; found a bytes while trying to load binding.gyp
    +gyp ERR! configure error                                                   
    +gyp ERR! stack Error: `gyp` failed with exit code: 1                       
    +gyp ERR! stack     at ChildProcess.onCpExit (/home/travis/build/nodejs/node-gyp/lib/configure.js:344:16)
    +gyp ERR! stack     at ChildProcess.emit (events.js:198:13)                 
    +gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:248:12)

The error is coming from

node-gyp/gyp/pylib/gyp/input.py

Lines 947 to 962 in 5bddd22

	if type(replacement) is list:
	for item in replacement:
	if not contents[-1] == '/' and type(item) not in (str, int):
	raise GypError('Variable ' + contents +
	' must expand to a string or list of strings; ' +
	'list contains a ' +
	item.__class__.__name__)
	# Run through the list and handle variable expansions in it. Since
	# the list is guaranteed not to contain dicts, this won't do anything
	# with conditions sections.
	ProcessVariablesAndConditionsInList(replacement, phase, variables,
	build_file)
	elif type(replacement) not in (str, int):
	raise GypError('Variable ' + contents +
	' must expand to a string or list of strings; ' +
	'found a ' + replacement.__class__.__name__)

[cclauss]

https://travis-ci.com/nodejs/node-gyp/builds/118963575 is a bit opaque to me this time.

[richardlau]

https://travis-ci.com/nodejs/node-gyp/builds/118963575 is a bit opaque to me this time.

For reference:
https://travis-ci.com/nodejs/node-gyp/jobs/215572608#L1954-L1979

test/test-find-python.js ............................ 24/26
  find python
  not ok should be equal
    --- wanted    
    +++ found     
    +Python 3.7.1 
    compare: ===
    at:
      line: 20
      column: 9
      file: test/test-find-python.js
    stack: |
      test/test-find-python.js:20:9
    source: |
      t.strictEqual(stdout, '')
  
  find python
  not ok expect truthy value
    at:
      line: 21
      column: 9
      file: test/test-find-python.js
    stack: |
      test/test-find-python.js:21:9
    source: |
      t.ok(/Python 2/.test(stderr))

also failed in #1815 (https://travis-ci.com/nodejs/node-gyp/jobs/214789663#L4479-L4504) so isn't because of this PR and is unlikely to be because of anything in gyp. cc @joaocgreis

[rvagg]

this is more of a rubber stamp lgtm than anything

i would like to see those tests cleaned up first though, @joaocgreis they look like they might be coming from your changes?

[joaocgreis]

This PR can land, the Python failure is unrelated.

test-find-python starts by looking for python as node-gyp would and confirming it is Python 2. But Python 3 is 3 and prints the version to stdout instead of stderr. I'll try to open a PR to fix this if no one beats to to it.

There are still failures in the Windows job that are not related to this.

[cclauss]

You are correct. https://bugs.python.org/issue18338

• python2 --version writes to stderr
• python3 --version writes to stdout

[rvagg]

@cclauss you're good to land and you can do it manually as long as you avoid a merge commit. First I'd recommend changing the commit message because it describes why not what. But your metadata is 👍.

Because you're not quite up to date with master, this is what I'd do to get this landed:

git checkout master
git pull origin master # replace "origin" with whatever you've called this repo
git checkout compile-module-removed-in-Python3
git rebase master # now up to date with master
git checkout master
git merge compile-module-removed-in-Python3 # should land just one commit onto master, cleanly
git push origin master # replace "origin" appropriately

I don't think we can use git node land on this repo, at least it didn't seem to work with non-nodejs/node repos last time I checked. I've been doing manual metadata and landing on this repo but perhaps someone else in here has a more automatic suggestion?

[rvagg]

Oh, that git pull origin master might cause problems actually because I rewrote master and you may have an out of sync version of master so a pull might create some chaos. I think git pull --rebase origin master might sort that out.

I sometimes go a more explicit route when in this kind of situation just to be absolutely clear what I'm trying to achieve:

git fetch origin
git checkout origin master
git branch -D master
git checkout -b master

but I think git pull --rebase origin master will achieve the same thing in this scenario.

[rvagg]

Your metadata didn't make it to master so something went wrong along the way unfortunately. I've force pushed a new version of the commit with.

PR-URL: https://github.com/nodejs/node-gyp/pull/1820
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Richard Lau <[email protected]>

and renamed it to build: more Python 3 compat, replace compile with ast, bacf53d

[richardlau]

Your metadata didn't make it to master so something went wrong along the way unfortunately. I've force pushed a new version of the commit with.

PR-URL: https://github.com/nodejs/node-gyp/pull/1820
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Richard Lau <[email protected]>

and renamed it to build: more Python 3 compat, replace compile with ast, bacf53d

I'm not in front of a computer at the moment, but it looks like this landed as a merge commit (both ef81bdb and bacf53d have two parents)?

[rvagg]

OK, I've fixed it up, reversed to the commit before and cherry-picked ef81bdb onto it then force pushed master. Thanks for picking that up @richardlau!