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 b9ddcd5 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 <[email protected]>

Author: pimterry

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))
       }

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 => {

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) {

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() }
+    }
   }
 })