From 4161552cf00dc2e0cccbb880dfe9eea882214a4a Mon Sep 17 00:00:00 2001 From: DeeDeeG Date: Tue, 6 Apr 2021 10:45:07 -0400 Subject: [PATCH 1/6] 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.) --- lib/build.js | 7 +++++++ lib/configure.js | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/build.js b/lib/build.js index c2388fb348..3baba4140c 100644 --- a/lib/build.js +++ b/lib/build.js @@ -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) } diff --git a/lib/configure.js b/lib/configure.js index 17a6487fa9..0b55c81ad3 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -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 @@ -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) } @@ -84,11 +87,22 @@ 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 () { + fs.symlink(python, symlinkDestination, function () {}) + }) + } + function createConfigFile (err, vsInfo) { if (err) { return callback(err) From 3ed345840accf356ca439598dd26af512a30c1b4 Mon Sep 17 00:00:00 2001 From: DeeDeeG Date: Wed, 31 Mar 2021 17:06:10 -0400 Subject: [PATCH 2/6] test: update mocked graceful-fs for configure test Add missing functions "unlink()" and "symlink()" to mocked module. --- test/test-configure-python.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test-configure-python.js b/test/test-configure-python.js index 4290e7af1b..aacd75f7c7 100644 --- a/test/test-configure-python.js +++ b/test/test-configure-python.js @@ -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() } } }) From b9367d31be1ced14519288879584a5e0045d2058 Mon Sep 17 00:00:00 2001 From: DeeDeeG Date: Thu, 8 Apr 2021 02:37:13 -0400 Subject: [PATCH 3/6] 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/configure.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/configure.js b/lib/configure.js index 0b55c81ad3..0bf585cb2a 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -98,8 +98,15 @@ function configure (gyp, argv, callback) { log.verbose('python symlink', `creating symlink to "${python}" at "${symlinkDestination}"`) - fs.unlink(symlinkDestination, function () { - fs.symlink(python, symlinkDestination, function () {}) + fs.unlink(symlinkDestination, function (err) { + if (err && err.code !== 'ENOENT') { + log.warn('python symlink', 'error when attempting to remove existing symlink\n', err) + } + fs.symlink(python, symlinkDestination, function (err) { + if (err) { + log.warn('python symlink', 'error when attempting to create Python symlink\n', err) + } + }) }) } From 5889ba086d9327ff7f88261f3e6e63a55c18bb5e Mon Sep 17 00:00:00 2001 From: DeeDeeG Date: Sun, 11 Apr 2021 10:16:19 -0400 Subject: [PATCH 4/6] 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/configure.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/configure.js b/lib/configure.js index 0bf585cb2a..482e545153 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -100,11 +100,13 @@ function configure (gyp, argv, callback) { fs.unlink(symlinkDestination, function (err) { if (err && err.code !== 'ENOENT') { - log.warn('python symlink', 'error when attempting to remove existing symlink\n', err) + log.warn('python symlink', 'error when attempting to remove existing symlink') + log.warn('python symlink', err.stack) } fs.symlink(python, symlinkDestination, function (err) { if (err) { - log.warn('python symlink', 'error when attempting to create Python symlink\n', err) + log.warn('python symlink', 'error when attempting to create Python symlink') + log.warn('python symlink', err.stack) } }) }) From 0b54241f0d2e9fca8f122326c694b411fafcd09b Mon Sep 17 00:00:00 2001 From: DeeDeeG Date: Sun, 11 Apr 2021 11:01:27 -0400 Subject: [PATCH 5/6] 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/configure.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/configure.js b/lib/configure.js index 482e545153..28746754d9 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -101,12 +101,12 @@ function configure (gyp, argv, callback) { fs.unlink(symlinkDestination, function (err) { if (err && err.code !== 'ENOENT') { log.warn('python symlink', 'error when attempting to remove existing symlink') - log.warn('python symlink', err.stack) + log.warn('python symlink', err.stack, 'errno: ' + err.errno) } fs.symlink(python, symlinkDestination, function (err) { if (err) { log.warn('python symlink', 'error when attempting to create Python symlink') - log.warn('python symlink', err.stack) + log.warn('python symlink', err.stack, 'errno: ' + err.errno) } }) }) From 54328e16ca9ed17a2f976a1a341352727bdf9280 Mon Sep 17 00:00:00 2001 From: DeeDeeG Date: Sun, 4 Jul 2021 19:04:55 -0400 Subject: [PATCH 6/6] 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. --- lib/configure.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/configure.js b/lib/configure.js index 28746754d9..c7010385b5 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -100,13 +100,13 @@ function configure (gyp, argv, callback) { fs.unlink(symlinkDestination, function (err) { if (err && err.code !== 'ENOENT') { - log.warn('python symlink', 'error when attempting to remove existing symlink') - log.warn('python symlink', err.stack, 'errno: ' + err.errno) + 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.warn('python symlink', 'error when attempting to create Python symlink') - log.warn('python symlink', err.stack, 'errno: ' + err.errno) + log.verbose('python symlink', 'error when attempting to create Python symlink') + log.verbose('python symlink', err.stack, 'errno: ' + err.errno) } }) })