From e9f27cc989c22f0e2971d2517ef3493dc3b4630d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Sun, 23 Feb 2025 09:27:35 +0100 Subject: [PATCH 1/8] fix for #4828: don't trash optional dependencies that fail to to an incompatible platform, to prevent issues when either the lockfile is removed --- workspaces/arborist/lib/arborist/reify.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 4083d79f4fa25..a8dae22e06673 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -815,11 +815,18 @@ 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) => { const set = optionalSet(node) for (node of set) { - log.verbose('reify', 'failed optional dependency', node.path) - this[_addNodeToTrashList](node) + // 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' + log.verbose('reify', 'failed optional dependency', node.path, trashOptionalDependency) + if (trashOptionalDependency) { + this[_addNodeToTrashList](node) + } } }) : p).then(() => node) } From e77fa1164a50935c512019bd7a7b2891e01aaa1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Sun, 23 Feb 2025 09:42:59 +0100 Subject: [PATCH 2/8] fix for #4828: move check to before loop --- workspaces/arborist/lib/arborist/reify.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index a8dae22e06673..e149c89b36fb8 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -816,13 +816,13 @@ module.exports = cls => class Reifier extends cls { // just add it and its optional set to the trash list. [_handleOptionalFailure] (node, p) { 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' const set = optionalSet(node) for (node of set) { - // 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' log.verbose('reify', 'failed optional dependency', node.path, trashOptionalDependency) if (trashOptionalDependency) { this[_addNodeToTrashList](node) From 61288248d9623c8d942484a067d4e15ea54009f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Sun, 23 Feb 2025 09:44:56 +0100 Subject: [PATCH 3/8] fix for #4828: logging improvement --- workspaces/arborist/lib/arborist/reify.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index e149c89b36fb8..b7ab26272a14b 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -823,7 +823,7 @@ module.exports = cls => class Reifier extends cls { const trashOptionalDependency = e?.code !== 'EBADPLATFORM' const set = optionalSet(node) for (node of set) { - log.verbose('reify', 'failed optional dependency', node.path, trashOptionalDependency) + log.verbose('reify', `failed optional dependency (trash: ${trashOptionalDependency}`, node.path) if (trashOptionalDependency) { this[_addNodeToTrashList](node) } From ba20f4d3528bdc6df6518499ec6124494f747fd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Mon, 24 Feb 2025 08:59:17 +0100 Subject: [PATCH 4/8] fix for #4828: apply the same fix to build-ideal-tree and log when pruning is skipped --- workspaces/arborist/lib/arborist/build-ideal-tree.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 54f86dea0f65c..679dabbf01fff 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -1489,6 +1489,13 @@ This is a one-time fix-up, please be patient... throw node.errors[0] } + // Optional dependencies that fail to install due to an incompatible platform + // should not be pruned to prevent rebuild issues when the lockfile is not present. + const pruneOptionalDependency = node.errors[0]?.code !== 'EBADPLATFORM' + if (!pruneOptionalDependency) { + log.verbose('build-ideal-tree', `skip pruning optional dependency due to platform mismatch`, node.path) + continue + } const set = optionalSet(node) for (const node of set) { node.parent = null From feba0f2f177f8ca32ae7c4a8839f506b9f060e3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Mon, 24 Feb 2025 09:08:48 +0100 Subject: [PATCH 5/8] fix for #4828: logging improvement --- workspaces/arborist/lib/arborist/reify.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index b7ab26272a14b..7b934de129093 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -823,7 +823,7 @@ module.exports = cls => class Reifier extends cls { const trashOptionalDependency = e?.code !== 'EBADPLATFORM' const set = optionalSet(node) for (node of set) { - log.verbose('reify', `failed optional dependency (trash: ${trashOptionalDependency}`, node.path) + log.verbose('reify', `failed optional dependency (trash: ${trashOptionalDependency})`, node.path) if (trashOptionalDependency) { this[_addNodeToTrashList](node) } From 49e7ffcc60759d52acfe873001f6de562c447d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Mon, 24 Feb 2025 09:13:14 +0100 Subject: [PATCH 6/8] fix for #4828: simplify code --- workspaces/arborist/lib/arborist/reify.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 7b934de129093..c7bf9f4e4755a 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -821,12 +821,14 @@ module.exports = cls => class Reifier extends cls { // 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 (trash: ${trashOptionalDependency})`, node.path) - if (trashOptionalDependency) { - this[_addNodeToTrashList](node) - } + log.verbose('reify', 'failed optional dependency', node.path) + this[_addNodeToTrashList](node) } }) : p).then(() => node) } From b590d479efc3cddf247558e0fbf7a835907184be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Tue, 25 Feb 2025 20:54:43 +0100 Subject: [PATCH 7/8] fix for #4828: - fix tests by adjusting snapshots for reify, now that it doesn't trash optional dependencies that fail due to a platform mismatch - fix tests for isolated-mode by adjusting the assertions to not expect failure, but expect an empty directory though in case of a platform mismatch --- .../test/arborist/reify.js.test.cjs | 100 +++++++++++++++++- workspaces/arborist/test/isolated-mode.js | 29 ++++- 2 files changed, 121 insertions(+), 8 deletions(-) diff --git a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs index cec3560033241..35cbe71f02d3a 100644 --- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs @@ -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", }, }, @@ -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", }, }, @@ -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", }, }, @@ -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", }, }, @@ -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", }, }, diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js index dcc293b82dc77..8699b5951d589 100644 --- a/workspaces/arborist/test/isolated-mode.js +++ b/workspaces/arborist/test/isolated-mode.js @@ -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') - 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 => { @@ -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 @@ -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) From c7e07a584960ef41ac0c046191d35e2d052176ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20R=C3=BCtter?= Date: Tue, 25 Feb 2025 21:17:19 +0100 Subject: [PATCH 8/8] fix for #4828: - Remove unneeded code in build-ideal-tree, as it currently doesn't seem to ever hit this code - Without this, the original issue is still fixed --- workspaces/arborist/lib/arborist/build-ideal-tree.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 679dabbf01fff..54f86dea0f65c 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -1489,13 +1489,6 @@ This is a one-time fix-up, please be patient... throw node.errors[0] } - // Optional dependencies that fail to install due to an incompatible platform - // should not be pruned to prevent rebuild issues when the lockfile is not present. - const pruneOptionalDependency = node.errors[0]?.code !== 'EBADPLATFORM' - if (!pruneOptionalDependency) { - log.verbose('build-ideal-tree', `skip pruning optional dependency due to platform mismatch`, node.path) - continue - } const set = optionalSet(node) for (const node of set) { node.parent = null