From 0f1f667b737d21905e283df100a2cb639993562a Mon Sep 17 00:00:00 2001 From: Tim Perry <1526883+pimterry@users.noreply.github.com> Date: Tue, 1 Aug 2023 10:02:59 +0200 Subject: [PATCH] fix: create Python symlink only during builds, and clean it up after (#2721) * fix: create Python symlink only during builds, and clean it up after Previously in b9ddcd5bbd93b05b03674836b6ebdae2c2e74c8c this was created during configuration, and the symlink persisted indefinitely. This causes problems with many tools that do not expect a codebase to include symlinks to external absolute paths. This PR largely reverts that commit, and instead writes the path to link to into the config, and then creates the symlink only temporarily during the build process, always deleting it afterwards. * assert install_path == self.output, f"{install_path} != {self.output}" --------- Co-authored-by: Christian Clauss --- lib/build.js | 29 ++++++++++++++++++++++++++--- lib/configure.js | 26 ++------------------------ lib/create-config-gypi.js | 9 ++++++--- test/test-configure-python.js | 4 +--- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/lib/build.js b/lib/build.js index 3b95b76353..0bcc9bea19 100644 --- a/lib/build.js +++ b/lib/build.js @@ -30,6 +30,8 @@ async function build (gyp, argv) { let arch let nodeDir let guessedSolution + let python + let buildBinsDir await loadConfigGypi() @@ -56,6 +58,7 @@ async function build (gyp, argv) { buildType = config.target_defaults.default_configuration arch = config.variables.target_arch nodeDir = config.variables.nodedir + python = config.variables.python if ('debug' in gyp.opts) { buildType = gyp.opts.debug ? 'Debug' : 'Release' @@ -67,6 +70,7 @@ async function build (gyp, argv) { log.verbose('build type', buildType) log.verbose('architecture', arch) log.verbose('node dev dir', nodeDir) + log.verbose('python', python) if (win) { await findSolutionFile() @@ -182,13 +186,32 @@ async function build (gyp, argv) { if (!win) { // Add build-time dependency symlinks (such as Python) to PATH - const buildBinsDir = path.resolve('build', 'node_gyp_bins') + 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`) + await fs.mkdir(buildBinsDir, { recursive: true }) + const symlinkDestination = path.join(buildBinsDir, 'python3') + try { + await fs.unlink(symlinkDestination) + } catch (err) { + if (err.code !== 'ENOENT') throw err + } + await fs.symlink(python, symlinkDestination) + log.verbose('bin symlinks', `created symlink to "${python}" in "${buildBinsDir}" and added to PATH`) } const proc = gyp.spawn(command, argv) - await new Promise((resolve, reject) => proc.on('exit', (code, signal) => { + await new Promise((resolve, reject) => proc.on('exit', async (code, signal) => { + if (buildBinsDir) { + // Clean up the build-time dependency symlinks: + if (fs.rm) { + // fs.rm is only available in Node 14+ + await fs.rm(buildBinsDir, { recursive: true }) + } else { + // Only used for old node, as recursive rmdir is deprecated in Node 14+ + await fs.rmdir(buildBinsDir, { recursive: true }) + } + } + if (code !== 0) { return reject(new Error('`' + command + '` failed with exit code: ' + code)) } diff --git a/lib/configure.js b/lib/configure.js index bcd8bb23ea..01c3f497ad 100644 --- a/lib/configure.js +++ b/lib/configure.js @@ -18,7 +18,6 @@ if (win) { function configure (gyp, argv, callback) { let python const buildDir = path.resolve('build') - const buildBinsDir = path.join(buildDir, 'node_gyp_bins') const configNames = ['config.gypi', 'common.gypi'] const configs = [] let nodeDir @@ -76,8 +75,7 @@ function configure (gyp, argv, callback) { function createBuildDir () { log.verbose('build dir', 'attempting to create "build" dir: %s', buildDir) - const deepestBuildDirSubdirectory = win ? buildDir : buildBinsDir - fs.mkdir(deepestBuildDirSubdirectory, { recursive: true }, function (err, isNew) { + fs.mkdir(buildDir, { recursive: true }, function (err, isNew) { if (err) { return callback(err) } @@ -88,31 +86,11 @@ 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) @@ -121,7 +99,7 @@ function configure (gyp, argv, callback) { process.env.GYP_MSVS_VERSION = Math.min(vsInfo.versionYear, 2015) process.env.GYP_MSVS_OVERRIDE_PATH = vsInfo.path } - createConfigGypi({ gyp, buildDir, nodeDir, vsInfo }).then(configPath => { + createConfigGypi({ gyp, buildDir, nodeDir, vsInfo, python }).then(configPath => { configs.push(configPath) findConfigs() }).catch(err => { diff --git a/lib/create-config-gypi.js b/lib/create-config-gypi.js index 2eab6f3035..8687379385 100644 --- a/lib/create-config-gypi.js +++ b/lib/create-config-gypi.js @@ -35,7 +35,7 @@ async function getBaseConfigGypi ({ gyp, nodeDir }) { return JSON.parse(JSON.stringify(process.config)) } -async function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) { +async function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo, python }) { const config = await getBaseConfigGypi({ gyp, nodeDir }) if (!config.target_defaults) { config.target_defaults = {} @@ -75,6 +75,9 @@ async function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) { // set the node development directory variables.nodedir = nodeDir + // set the configured Python path + variables.python = python + // disable -T "thin" static archives by default variables.standalone_static_library = gyp.opts.thin ? 0 : 1 @@ -112,13 +115,13 @@ async function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) { return config } -async function createConfigGypi ({ gyp, buildDir, nodeDir, vsInfo }) { +async function createConfigGypi ({ gyp, buildDir, nodeDir, vsInfo, python }) { const configFilename = 'config.gypi' const configPath = path.resolve(buildDir, configFilename) log.verbose('build/' + configFilename, 'creating config file') - const config = await getCurrentConfigGypi({ gyp, nodeDir, vsInfo }) + const config = await getCurrentConfigGypi({ gyp, nodeDir, vsInfo, python }) // ensures that any boolean values in config.gypi get stringified function boolsToString (k, v) { diff --git a/test/test-configure-python.js b/test/test-configure-python.js index 9c042b18d5..dcce3e5de2 100644 --- a/test/test-configure-python.js +++ b/test/test-configure-python.js @@ -16,9 +16,7 @@ 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() } + } } })