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.

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

feat: add --libc option to override platform specific install #6914

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Oct 18, 2023

Authored by: @Brooooooklyn

@wraithgar
Copy link
Member Author

It looks like arborist tests aren't passing still.

@klausbadelt
Copy link

Seems only an updated snapshot is needed to pass tests. Here it is, @wraithgar.

pr6914.patch

(please let me know if there's a better way to contribute)

The folks at lovell/sharp#3873 would love to see this merged :)

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 dcc3692a8..aebb6ac49 100644
--- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
+++ b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
@@ -3161,7 +3161,7 @@ ArboristNode {
 }
 `
 
-exports[`test/arborist/reify.js TAP fail to install optional deps with matched os and mismatched cpu with os and cpu options > expect resolving Promise 1`] = `
+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 {
   "edgesOut": Map {
     "platform-specifying-test-package" => EdgeOut {
@@ -3173,14 +3173,14 @@ ArboristNode {
   },
   "isProjectRoot": true,
   "location": "",
-  "name": "tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-mismatched-cpu-with-os-and-cpu-options",
+  "name": "tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-matched-cpu-and-mismatched-libc-with-os-and-cpu-and-libc-options",
   "packageName": "platform-test",
-  "path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-mismatched-cpu-with-os-and-cpu-options",
+  "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",
   "version": "1.0.0",
 }
 `
 
-exports[`test/arborist/reify.js TAP fail to install optional deps with mismatched os and matched cpu with os and cpu options > expect resolving Promise 1`] = `
+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 {
   "edgesOut": Map {
     "platform-specifying-test-package" => EdgeOut {
@@ -3192,9 +3192,28 @@ ArboristNode {
   },
   "isProjectRoot": true,
   "location": "",
-  "name": "tap-testdir-reify-fail-to-install-optional-deps-with-mismatched-os-and-matched-cpu-with-os-and-cpu-options",
+  "name": "tap-testdir-reify-fail-to-install-optional-deps-with-matched-os-and-mismatched-cpu-with-os-and-cpu-and-libc-options",
   "packageName": "platform-test",
-  "path": "{CWD}/test/arborist/tap-testdir-reify-fail-to-install-optional-deps-with-mismatched-os-and-matched-cpu-with-os-and-cpu-options",
+  "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",
+  "version": "1.0.0",
+}
+`
+
+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 {
+  "edgesOut": Map {
+    "platform-specifying-test-package" => EdgeOut {
+      "name": "platform-specifying-test-package",
+      "spec": "1.0.0",
+      "to": null,
+      "type": "optional",
+    },
+  },
+  "isProjectRoot": true,
+  "location": "",
+  "name": "tap-testdir-reify-fail-to-install-optional-deps-with-mismatched-os-and-matched-cpu-with-os-and-cpu-and-libc-options",
+  "packageName": "platform-test",
+  "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",
   "version": "1.0.0",
 }
 `
@@ -33031,7 +33050,7 @@ exports[`test/arborist/reify.js TAP store files with a custom indenting > must m
 
 `
 
-exports[`test/arborist/reify.js TAP success to install optional deps with matched platform specifications with os and cpu options > expect resolving Promise 1`] = `
+exports[`test/arborist/reify.js TAP success to install optional deps with matched platform specifications with os and cpu and libc options > expect resolving Promise 1`] = `
 ArboristNode {
   "children": Map {
     "platform-specifying-test-package" => ArboristNode {
@@ -33046,7 +33065,7 @@ ArboristNode {
       "location": "node_modules/platform-specifying-test-package",
       "name": "platform-specifying-test-package",
       "optional": true,
-      "path": "{CWD}/test/arborist/tap-testdir-reify-success-to-install-optional-deps-with-matched-platform-specifications-with-os-and-cpu-options/node_modules/platform-specifying-test-package",
+      "path": "{CWD}/test/arborist/tap-testdir-reify-success-to-install-optional-deps-with-matched-platform-specifications-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",
     },
@@ -33061,9 +33080,9 @@ ArboristNode {
   },
   "isProjectRoot": true,
   "location": "",
-  "name": "tap-testdir-reify-success-to-install-optional-deps-with-matched-platform-specifications-with-os-and-cpu-options",
+  "name": "tap-testdir-reify-success-to-install-optional-deps-with-matched-platform-specifications-with-os-and-cpu-and-libc-options",
   "packageName": "platform-test",
-  "path": "{CWD}/test/arborist/tap-testdir-reify-success-to-install-optional-deps-with-matched-platform-specifications-with-os-and-cpu-options",
+  "path": "{CWD}/test/arborist/tap-testdir-reify-success-to-install-optional-deps-with-matched-platform-specifications-with-os-and-cpu-and-libc-options",
   "version": "1.0.0",
 }
 `

@wraithgar
Copy link
Member Author

Thanks @klausbadelt that was all I really needed. This should go out w/ tomorrow's release as long as this PR goes green.

@wraithgar
Copy link
Member Author

Arborist failure looks like a timeout? Which is unexpected. Gonna have to look into that next.

@wraithgar wraithgar self-assigned this Jan 9, 2024
@wraithgar
Copy link
Member Author

Test failures are unrelated to this PR, and likely due to #7121

@wraithgar
Copy link
Member Author

npm/cacache#255 forced an issue we knew about and had a PR up to fix, but now we have to wait till it lands to move forward. If we have to we are going to inline the fixed code into npm till the PR lands. Thanks for your patience.

@wraithgar
Copy link
Member Author

It looks like one of the tests here still fails even w/ the older cacache. We'll need to solve both problems before we can land this, but the cacache one first since it's making debugging this further very difficult.

@klausbadelt
Copy link

For what it's worth - running tests locally pre-snapshot update also "timed out" - it didn't fail 'properly'. I agree that's odd. The above snapshot update fixed this though.

@wraithgar
Copy link
Member Author

Yeah it was the snapshots making it fail even w/ a rolled back cacache. Tests pass with the older version so I'm confident in this PR outside of that issue. Fixing the timeouts isn't part of this PR so I'll land this and work on the promise concurrency in a separate PR

@wraithgar wraithgar merged commit 6673c77 into latest Jan 10, 2024
51 of 52 checks passed
@wraithgar wraithgar deleted the gar/6817 branch January 10, 2024 16:05
@github-actions github-actions bot mentioned this pull request Jan 10, 2024
@klausbadelt
Copy link

Thank you @wraithgar - really appreciate this 🚀

@Tofandel
Copy link

Tofandel commented Nov 19, 2024

There is another checkPlatform in arborist idealTree, is it normal that that one is not being passed the os, cpu and libc options?

checkPlatform(node.package, this.options.force)

@wraithgar
Copy link
Member Author

@Tofandel that appears to be an oversight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants