Skip to content

Commit

Permalink
Add Python symlink to path (for non-Windows OSes only) (#2362)
Browse files Browse the repository at this point in the history
* lib: create a Python symlink and add it to PATH

Helps to ensure a version of Python validated by lib/find-python.js
is used to run various Python scripts generated by gyp.

Known to affect gyp-mac-tool, probably affects gyp-flock-tool as well.

These Python scripts (such as `gyp-mac-tool`) are invoked directly,
via the generated Makefile, so their shebang lines determine
which Python binary is used to run them.
The shebang lines of these scripts are all `#!/usr/bin/env python3`,
so the first `python3` on the user's PATH will be used.

By adding a symlink to the Python binary validated by find-python.js,
and putting this symlink first on the PATH, we can ensure we use
a compatible version of Python to run these scripts.

(Only on Unix/Unix-like OSes. Symlinks are tricky on Windows,
and Python isn't used at build-time anyhow on Windows,
so this intervention isn't useful or necessary on Windows.

A similar technique for Windows, no symlinks required,
would be to make batch scripts which execute the target binary,
much like what Node does for its bundled copy of npm on Windows.)

* test: update mocked graceful-fs for configure test

Add missing functions "unlink()" and "symlink()" to mocked module.

* lib: log any errors when creating Python symlink

Warn users about errors, but continue on in case the user does
happen to have new enough Python on their PATH.

(The symlinks are only meant to fix an issue in a corner case,
where the user told `node-gyp` where new enough Python is,
but it's not the first `python3` on their PATH.
We should not introduce a new potential failure mode to all users
when fixing this bug. So no hard errors during the symlink process.)

* lib: improve error formatting for Python symlink

Logging the entire error object shows the stack twice,
and all the other information is contained in the stack.

It also messes with the order of what is logged.

Rather than logging a bunch of redundant information in a messy way,
we can log only the stack. Logging it in a separate log.warn()
also gets rid of an extra space character at the beginning of the line.

* lib: restore err.errno to logs for symlink errors

This info (err.errno) is the only piece of information
in the error object that is not redundant to err.stack.

* lib: use log.verbose, not log.warn

These messages aren't important enough to be `log.warn`s.

Log as verbose only; they will also appear in full error output.
  • Loading branch information
DeeDeeG committed Jun 10, 2022
1 parent 6f74c76 commit b9ddcd5
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 2 deletions.
7 changes: 7 additions & 0 deletions lib/build.js
Expand Up @@ -185,6 +185,13 @@ function build (gyp, argv, callback) {
}
}

if (!win) {
// Add build-time dependency symlinks (such as Python) to PATH
const buildBinsDir = path.resolve('build', 'node_gyp_bins')
process.env.PATH = `${buildBinsDir}:${process.env.PATH}`
log.verbose('bin symlinks', `adding symlinks (such as Python), at "${buildBinsDir}", to PATH`)
}

var proc = gyp.spawn(command, argv)
proc.on('exit', onExit)
}
Expand Down
25 changes: 24 additions & 1 deletion lib/configure.js
Expand Up @@ -17,6 +17,7 @@ if (win) {
function configure (gyp, argv, callback) {
var python
var buildDir = path.resolve('build')
var buildBinsDir = path.join(buildDir, 'node_gyp_bins')
var configNames = ['config.gypi', 'common.gypi']
var configs = []
var nodeDir
Expand Down Expand Up @@ -73,7 +74,9 @@ function configure (gyp, argv, callback) {

function createBuildDir () {
log.verbose('build dir', 'attempting to create "build" dir: %s', buildDir)
fs.mkdir(buildDir, { recursive: true }, function (err, isNew) {

const deepestBuildDirSubdirectory = win ? buildDir : buildBinsDir
fs.mkdir(deepestBuildDirSubdirectory, { recursive: true }, function (err, isNew) {
if (err) {
return callback(err)
}
Expand All @@ -84,11 +87,31 @@ function configure (gyp, argv, callback) {
findVisualStudio(release.semver, gyp.opts.msvs_version,
createConfigFile)
} else {
createPythonSymlink()
createConfigFile()
}
})
}

function createPythonSymlink () {
const symlinkDestination = path.join(buildBinsDir, 'python3')

log.verbose('python symlink', `creating symlink to "${python}" at "${symlinkDestination}"`)

fs.unlink(symlinkDestination, function (err) {
if (err && err.code !== 'ENOENT') {
log.verbose('python symlink', 'error when attempting to remove existing symlink')
log.verbose('python symlink', err.stack, 'errno: ' + err.errno)
}
fs.symlink(python, symlinkDestination, function (err) {
if (err) {
log.verbose('python symlink', 'error when attempting to create Python symlink')
log.verbose('python symlink', err.stack, 'errno: ' + err.errno)
}
})
})
}

function createConfigFile (err, vsInfo) {
if (err) {
return callback(err)
Expand Down
4 changes: 3 additions & 1 deletion test/test-configure-python.js
Expand Up @@ -14,7 +14,9 @@ const configure = requireInject('../lib/configure', {
mkdir: function (dir, options, cb) { cb() },
promises: {
writeFile: function (file, data) { return Promise.resolve(null) }
}
},
unlink: function (path, cb) { cb() },
symlink: function (target, path, cb) { cb() }
}
})

Expand Down

1 comment on commit b9ddcd5

@DeeDeeG
Copy link
Contributor Author

@DeeDeeG DeeDeeG commented on b9ddcd5 Jun 10, 2022

Choose a reason for hiding this comment

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

This won't appear on the changelog, due to no prefix at the beginning of the commit message.

Could be fix: Add Python symlink to path (for non-Windows OSes only) (#2362) and would appear on the changelog.

Edit: Maybe it should be feat: (semver minor)?? Either way it should be in the changelogs IMO.

Please sign in to comment.