Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: create Python symlink only during builds, and clean it up after #2721

Merged
merged 3 commits into from
Aug 1, 2023
Merged

fix: create Python symlink only during builds, and clean it up after #2721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb50fe0
fix: create Python symlink only during builds, and clean it up after
pimterry Aug 22, 2022
6307d91
assert install_path == self.output, f"{install_path} != {self.output}"
cclauss Aug 1, 2023
bcf6611
Merge branch 'main' into cleanup-symlinks
cclauss Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions lib/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ async function build (gyp, argv) {
let arch
let nodeDir
let guessedSolution
let python
let buildBinsDir

await loadConfigGypi()

Expand All @@ -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'
Expand All @@ -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()
Expand Down Expand Up @@ -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))
}
Expand Down
26 changes: 2 additions & 24 deletions lib/configure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
Expand All @@ -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 => {
Expand Down
9 changes: 6 additions & 3 deletions lib/create-config-gypi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand Down Expand Up @@ -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
Copy link
Contributor

@DeeDeeG DeeDeeG Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My only concern is, if there are any existing packages which end up setting a variable variables.python in their config.gypi?

Maybe this should be something more specific where package authors would really have to try to collide with node-gyp's namespace, like variables.node_gyp.python_path or variables.node_gyp_python_path?

(I'm admittedly not a heavy node-gyp or gyp user, so maybe someone would know if that's a realistic concern or not?)

NOTE: I am not a maintainer for this repository, (just a past contributor who wrote the feature this PR is fixing unintended side effects of), so maintainers here have final say and this is just a personal opinion of someone technically outside the project. Either way, I'd like to see this get merged soon, if possible.


// disable -T "thin" static archives by default
variables.standalone_static_library = gyp.opts.thin ? 0 : 1

Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 1 addition & 3 deletions test/test-configure-python.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() }
}
}
})

Expand Down