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 --cpu and --os option to override platform specific install #6755

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

yukukotani
Copy link
Contributor

@yukukotani yukukotani commented Sep 1, 2023

Add --cpu option and ---os option which is proposed in npm/rfcs#612.

The change of npm-install-checks package is here: npm/npm-install-checks#65

Real Example

Let's think about the package.json below:

{
  "name": "demo",
  "version": "1.0.0",
  "dependencies": {
    "@node-rs/xxhash": "latest"
  }
}

Running npm i on my M1 mac machine will install two package @node-rs/xxhash and @node-rs/xxhash-darwin-arm64.

Running npm i --cpu x64 --os freebsd will install @node-rs/xxhash-freebsd-x64 instead of @node-rs/xxhash-darwin-arm64.

References

Closes npm/rfcs#612

@yukukotani yukukotani requested a review from a team as a code owner September 1, 2023 10:33
@yukukotani yukukotani changed the title Add --cpu and --os option to override platform specific install feat: Add --cpu and --os option to override platform specific install Sep 1, 2023
@wraithgar
Copy link
Member

Can you rebase your branch against latest? Testing matrix got updated for npm@10.

@wraithgar wraithgar mentioned this pull request Sep 1, 2023
@wraithgar
Copy link
Member

Once #6756 lands would you be willing to make a new PR to the release/v9 branch so that this new feature lands in npm 9?

@yukukotani
Copy link
Contributor Author

yukukotani commented Sep 2, 2023

@wraithgar It seems that already based on currnet latest branch. Backporting to v9 sounds nice!

$  git remote get-url origin 
https://github.com/npm/cli

$ git pull origin latest --rebase
From https://github.com/npm/cli
 * branch                latest     -> FETCH_HEAD
Current branch cpu-and-os is up to date.

$ git push
Everything up-to-date

@yukukotani
Copy link
Contributor Author

I've rebased again

@yukukotani
Copy link
Contributor Author

yukukotani commented Sep 8, 2023

Does the test fail because it's just flaky or the diff may contain something wrong? It passed on my local machine

@wraithgar wraithgar merged commit 1c93c44 into npm:latest Sep 8, 2023
44 of 52 checks passed
@github-actions github-actions bot mentioned this pull request Sep 8, 2023
@wraithgar
Copy link
Member

wraithgar commented Sep 8, 2023

The test failed cause of an engines mismatch, we need to bump the config module's CI and engines. Disregard, and thank you for seeing this through the multiple modules it took to implement. I know a lot of folks would appreciate this landing in release/v9 if you're willing to make that PR too.

@yukukotani
Copy link
Contributor Author

@wraithgar Sure, created #6776

@ryanblock
Copy link

ryanblock commented Jan 30, 2024

@yukukotani (cc @wraithgar) this is rad! A question about intended behavior on this feature:

  1. If an invalid arch and/or os flag is provided, no platform-specific dependencies are installed, and npm exits 0. I would consider that worthy of an error state; by exiting 0, we would lack the ability to know whether the command was malformed (perhaps due to typo, or by using x86_64 instead of x64, etc.). Was this intentional?

  2. If a valid arch and/or os flag is provided, but the platform-specific dependency does not exist, nothing is installed and npm exits 0. I would also consider this an error state. If I am specifying os / arch to obtain a specific build of a native module, it should be assumed that the application that uses this dependency would be broken if that dependency isn't present. Was this intentional?

Thank you!

Tested with npm 10.2.3

@wraithgar
Copy link
Member

Yes, this is intentional as all of these are optional dependencies, and missing an optional dependency is never an error state.

For 1, this is input validation which would need to happen in the config declaration. Off the top of my head I don't know what the valid set of parameters would be.

For 2, this is a job for npm query. We are planning on adding capabilities for it to exit 0 or 1 based on if there were or were not results, and that would address this kind of problem.

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.

[RRFC] Support --cpu and --os flag to specify platform specific install
3 participants