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

fix: don't trash optional dependencies that fail due to incompatible platform #8127

Open
wants to merge 9 commits into
base: latest
Choose a base branch
from
+131 −9
Open

fix: don't trash optional dependencies that fail due to incompatible platform #8127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e9f27cc
fix for #4828: don't trash optional dependencies that fail to to an i…
paulrutter Feb 23, 2025
e77fa11
fix for #4828: move check to before loop
paulrutter Feb 23, 2025
6128824
fix for #4828: logging improvement
paulrutter Feb 23, 2025
ba20f4d
fix for #4828: apply the same fix to build-ideal-tree and log when pr…
paulrutter Feb 24, 2025
feba0f2
fix for #4828: logging improvement
paulrutter Feb 24, 2025
49e7ffc
fix for #4828: simplify code
paulrutter Feb 24, 2025
b590d47
fix for #4828:
paulrutter Feb 25, 2025
c7e07a5
fix for #4828:
paulrutter Feb 25, 2025
03d2938
Merge branch 'npm:latest' into maintenance/fix-4828-dont-thrash-optio…
paulrutter Mar 7, 2025
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
11 changes: 10 additions & 1 deletion workspaces/arborist/lib/arborist/reify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,16 @@ module.exports = cls => class Reifier extends cls {
// if the node is optional, then the failure of the promise is nonfatal
// just add it and its optional set to the trash list.
[_handleOptionalFailure] (node, p) {
return (node.optional ? p.catch(() => {
return (node.optional ? p.catch((e) => {
// Optional dependencies that fail to install due to an incompatible platform
// should not be added to the trash list to prevent rebuild issues when the lockfile is not present.
// This does cause optional dependencies to be present on all platforms in the node_modules directory,
// even though these aren't really used
const trashOptionalDependency = e?.code !== 'EBADPLATFORM'
if (!trashOptionalDependency) {
log.verbose('reify', `skip trashing optional dependency due to platform mismatch`, node.path)
return
}
const set = optionalSet(node)
for (node of set) {
log.verbose('reify', 'failed optional dependency', node.path)
Expand Down
100 changes: 95 additions & 5 deletions workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2842,11 +2842,29 @@ ArboristNode {

exports[`test/arborist/reify.js TAP do not install optional deps with mismatched platform specifications > expect resolving Promise 1`] = `
ArboristNode {
"children": Map {
"platform-specifying-test-package" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"type": "optional",
},
},
"location": "node_modules/platform-specifying-test-package",
"name": "platform-specifying-test-package",
"optional": true,
"path": "{CWD}/test/arborist/tap-testdir-reify-do-not-install-optional-deps-with-mismatched-platform-specifications/node_modules/platform-specifying-test-package",
"resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"platform-specifying-test-package" => EdgeOut {
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"to": null,
"to": "node_modules/platform-specifying-test-package",
"type": "optional",
},
},
Expand Down Expand Up @@ -3320,11 +3338,29 @@ ArboristNode {

exports[`test/arborist/reify.js TAP fail to install optional deps with matched os and matched cpu and mismatched libc with os and cpu and libc options > expect resolving Promise 1`] = `
ArboristNode {
"children": Map {
"platform-specifying-test-package" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"type": "optional",
},
},
"location": "node_modules/platform-specifying-test-package",
"name": "platform-specifying-test-package",
"optional": true,
"path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-matched-cpu-and-mismatched-libc-with-os-and-cpu-and-libc-options/node_modules/platform-specifying-test-package",
"resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"platform-specifying-test-package" => EdgeOut {
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"to": null,
"to": "node_modules/platform-specifying-test-package",
"type": "optional",
},
},
Expand All @@ -3339,11 +3375,29 @@ ArboristNode {

exports[`test/arborist/reify.js TAP fail to install optional deps with matched os and mismatched cpu with os and cpu and libc options > expect resolving Promise 1`] = `
ArboristNode {
"children": Map {
"platform-specifying-test-package" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"type": "optional",
},
},
"location": "node_modules/platform-specifying-test-package",
"name": "platform-specifying-test-package",
"optional": true,
"path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-mismatched-cpu-with-os-and-cpu-and-libc-options/node_modules/platform-specifying-test-package",
"resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"platform-specifying-test-package" => EdgeOut {
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"to": null,
"to": "node_modules/platform-specifying-test-package",
"type": "optional",
},
},
Expand All @@ -3358,11 +3412,29 @@ ArboristNode {

exports[`test/arborist/reify.js TAP fail to install optional deps with mismatched os and matched cpu with os and cpu and libc options > expect resolving Promise 1`] = `
ArboristNode {
"children": Map {
"platform-specifying-test-package" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"type": "optional",
},
},
"location": "node_modules/platform-specifying-test-package",
"name": "platform-specifying-test-package",
"optional": true,
"path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-mismatched-os-and-matched-cpu-with-os-and-cpu-and-libc-options/node_modules/platform-specifying-test-package",
"resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"platform-specifying-test-package" => EdgeOut {
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"to": null,
"to": "node_modules/platform-specifying-test-package",
"type": "optional",
},
},
Expand Down Expand Up @@ -33177,11 +33249,29 @@ exports[`test/arborist/reify.js TAP scoped registries > should preserve original

exports[`test/arborist/reify.js TAP still do not install optional deps with mismatched platform specifications even when forced > expect resolving Promise 1`] = `
ArboristNode {
"children": Map {
"platform-specifying-test-package" => ArboristNode {
"edgesIn": Set {
EdgeIn {
"from": "",
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"type": "optional",
},
},
"location": "node_modules/platform-specifying-test-package",
"name": "platform-specifying-test-package",
"optional": true,
"path": "{CWD}/test/arborist/tap-testdir-reify-still-do-not-install-optional-deps-with-mismatched-platform-specifications-even-when-forced/node_modules/platform-specifying-test-package",
"resolved": "https://registry.npmjs.org/platform-specifying-test-package/-/platform-specifying-test-package-1.0.0.tgz",
"version": "1.0.0",
},
},
"edgesOut": Map {
"platform-specifying-test-package" => EdgeOut {
"name": "platform-specifying-test-package",
"spec": "1.0.0",
"to": null,
"to": "node_modules/platform-specifying-test-package",
"type": "optional",
},
},
Expand Down
29 changes: 26 additions & 3 deletions workspaces/arborist/test/isolated-mode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,9 +552,10 @@ tap.test('failing optional deps are not installed', async t => {
const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
await arborist.reify({ installStrategy: 'linked' })

t.notOk(setupRequire(dir)('which'), 'Failing optional deps should not be installed')
t.ok(setupRequire(dir)('which'), 'Failing optional deps should not be installed, but the empty directory is there')
Copy link

Choose a reason for hiding this comment

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

Am I right to say that the new behavior would be to leave empty directories for all of the optional deps that did not succeed? Hopefully there's a way to avoid this, since that would leave a load of empty dirs for a lot of native packages, and potentially confuse code which is scanning for these dirs to know which one to load for the current platform...

Copy link
Author

@paulrutter paulrutter Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That is correct indeed. This is used to rebuild the proper package-json.lock file based on the node_modules directory.
I don't know enough of NPM to surface a better solution, but maybe others have ideas on how this can prevented?

Instead of the empty directory, it could also keep a list of failed optionals in a separate file or something, but that would require far more changes i would assume.

Copy link

Choose a reason for hiding this comment

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

That's really unfortunate; I thought that npm stored a file inside node_modules that described the layout? It seems like a bad idea to rely on an empty dir existing, and I really do think it'll cause something to break.

Copy link
Author

@paulrutter paulrutter Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, i can imagine breakage if packages rely on directory scanning without verifying the contents.

If this is not the desired approach, i would need guidance from the maintainers on what would be the best way forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we'll have @wraithgar weigh in

Copy link
Author

@paulrutter paulrutter Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For what it's worth, i checked out bindings which i use myself in https://github.com/blueconic/node-oom-heapdump, and that library handles this just fine. Haven't checked how nodejs itself handles it, as it's able to load native modules out of the box nowadays.

https://github.com/TooTallNate/node-bindings/blob/c8033dcfc04c34397384e23f7399a30e6c13830d/bindings.js#L111

Also, the rollup package doesn't rely on directory scanning: https://github.com/rollup/rollup/blob/d3e79bd79e6b53b01ef7d6535d6ea9ad9be714dd/native.js#L77

sharp seems safe as well: https://github.com/lovell/sharp/blob/1533bf995acda779313fc178d2b9d46791349961/lib/sharp.js#L23

Copy link

Choose a reason for hiding this comment

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

The main ones to check out are rollup and esbuild (the bulk of the people in the issue having this problem), but there's also swc, oxc, dprint, who have this same problem too.

Copy link
Author

@paulrutter paulrutter Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

eslint https://github.com/evanw/esbuild/blob/b914dd30294346aa15fcc04278f4b4b51b8b43b5/lib/npm/node-platform.ts#L150 is safe for the happy path, but the fallback for fixing the platform issue does use directory scanning.

swc is safe: https://github.com/swc-project/swc/blob/b65387ac570c4bbb3b776a69bda810be862b434e/packages/core/binding.js#L4

oxc is safe: https://github.com/oxc-project/oxc/blob/bff83c96a21d582775b65d97123d3a2ffabf30be/napi/parser/bindings.js#L4

dprint is safe: https://github.com/dprint/dprint/blob/00e8f5e9895147b20fe70a0e4e5437bd54d928e8/deployment/npm/install_api.js#L23


t.notOk(fs.existsSync(path.join(dir, 'node_modules', '.bin', 'which')))
t.ok(fs.existsSync(path.join(dir, 'node_modules', 'which')))
t.ok(isEmptyDir(t, path.join(dir, 'node_modules', 'which')))
})

tap.test('Optional deps are installed when possible', async t => {
Expand Down Expand Up @@ -1319,7 +1320,12 @@ tap.test('failing optional peer deps are not installed', async t => {
const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
await arborist.reify({ installStrategy: 'linked' })

t.notOk(setupRequire(dir)('bar', 'which'), 'Failing optional peer deps should not be installed')
t.ok(setupRequire(dir)('bar', 'which'), 'Failing optional peer deps should not be installed, but an empty directory is there')

t.ok(fs.existsSync(path.join(dir, 'node_modules', 'which')))
t.ok(isEmptyDir(t, path.join(dir, 'node_modules', 'which')))
t.ok(fs.existsSync(path.join(dir, 'node_modules', 'bar')))
t.ok(isEmptyDir(t, path.join(dir, 'node_modules', 'bar')))
})

// Virtual packages are 2 packages that have the same version but are
Expand Down Expand Up @@ -1452,6 +1458,23 @@ function setupRequire (cwd) {
}
}

/**
* Determine whether the given `path` points to an empty directory.
* @param {String} path
* @returns {boolean}
*/
async function isEmptyDir (path) {
try {
const directory = await fs.opendir(path)
const entry = await directory.read()
await directory.close()

return entry === null
} catch (error) {
return false
}
}

function pathExists (path) {
try {
fs.statSync(path)
Expand Down