Build native dependencies with spaces in apm's path

[smashwilson]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions

Description of the Change

Currently, Atom Beta is unable to install packages that directly or transitively include a native dependency, because the Makefile that node-gyp generate includes whitespace in a dependency list, causing the following failure:

make: *** No rule to make target `../../../../../../../../../../Applications/Atom', needed by `Makefile'.  Stop.
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/Applications/Atom Beta.app/Contents/Resources/app/apm/node_modules/node-gyp/lib/build.js:276:23)
gyp ERR! stack     at emitTwo (events.js:87:13)
gyp ERR! stack     at ChildProcess.emit (events.js:172:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

This PR works around the issue by providing a custom Makefile generator that escapes these paths.

Passing the custom generator to gyp is a bit... involved. node-gyp hardcodes an -fmake argument to the gyp invocation and doesn't provide a mechanism to pass a custom --format. To work around this, I've introduced a python-interceptor.sh script that's passed to npm as PYTHON, which node-gyp will exec to invoke gyp. python-interceptor.sh directly execs the real Python binary preserving all arguments as-is, except when it's running the gyp entrypoint, in which case it detects and removes any -f or --format arguments and replaces them with a --format argument that invokes the custom generator.

Because gyp assumes that paths don't contain a - character, the custom generator is provided to gyp by adding its containing directory to PYTHONPATH and specifying the generator as <modulename>.py. That triggers gyp's module file importing logic to import the correct module name, but by manipulating the PYTHONPATH ourselves we sidestep the - split earlier.

Alternate Designs

The best option here is to fix the problem at the source. It's a one-line fix upstream in node-gyp. I do intend to file a PR there, but I'd rather have this a temporary fix we can 🚢 independently until that works its way through the pipeline of getting feedback and getting merged 👉 being released in node-gyp 👉 being released in npm.

Benefits

The immediate benefit is that apm-beta install orupgrade commands will succeed for packages that include native dependencies, from the commandline or within the Atom UI.

It requires no action on the part of either npm package authors or apm users for builds to succeed.

Possible Drawbacks

The python-interceptor.sh script relies on the presence of a /bin/bash binary that understands arrays. There may be Linux distros where that is not true.

Applicable Issues

• Fixes apm-beta fails to install some packages if Atom Beta is installed but not Atom #499
• Build failure when builddir contains spaces. nodejs/node-gyp#65

[BinaryMuse]

@smashwilson what's the story here for Windows?

[smashwilson]

@BinaryMuse Windows should be unaffected, as Windows users will go in through apm.cmd and npm.cmd instead, which doesn't set PYTHON to the interceptor script.

I should verify that apm-beta can build on Windows, though.

[winstliu]

I have been installing packages with native dependencies fine on Windows, and I know @damieng has as well. Both of us have spaces in the directory path.

[smashwilson]

@50Wliu Perfect. I just confirmed that this works:

<C:\Users\smash>
] apm-beta install atom/github
Cloning https://github.com/atom/github.git done
Installing modules done
Moving github to C:\Users\smash\.atom\packages\github done
<C:\Users\smash>
]

[smashwilson]

This feels a bit weird to me. I poked around a bit trying to use @npm.config.get('node-gyp') but that's pointed to the node_modules/.bin/ symlink, not the node-gyp.js script, so @fork wasn't happy with it, and I didn't want to swap it out for a @spawn because it looked important to preserve the node executable.

[smashwilson]

@BinaryMuse I'd appreciate a look-over when you get the chance 😁