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 support to install different architectures #7214

Merged
merged 43 commits into from Oct 24, 2023

Conversation

nachoaldamav
Copy link
Contributor

@nachoaldamav nachoaldamav commented Oct 17, 2023

This feature allows pnpm to force the install of dependencies for specific systems.

For example, with this package.json:

{
  "pnpm": {
    "supportedArchitectures": {
      "os": ["darwin", "win32"],
      "cpu": ["x64", "arm64"]
    }
  }
}

pnpm will install the following dependencies for esbuild:

  - @esbuild+darwin-arm64@0.18.20
  - @esbuild+darwin-x64@0.18.20
  - @esbuild+win32-arm64@0.18.20
  - @esbuild+win32-x64@0.18.20

TODO:

  • add tests for supportedArchitectures and hoisted node_modules
  • add test for the case when the optional deps are in subdeps, architectures are changed and a new dependency is installed.
  • verify that the old optional deps are removed when the value of supportedArchitectures is changed.

close #5965

cli/cli-utils/src/readProjectManifest.ts Outdated Show resolved Hide resolved
cli/cli-utils/src/readProjectManifest.ts Outdated Show resolved Hide resolved
config/package-is-installable/src/checkPlatform.ts Outdated Show resolved Hide resolved
config/package-is-installable/src/checkPlatform.ts Outdated Show resolved Hide resolved
exec/plugin-commands-script-runners/test/index.ts Outdated Show resolved Hide resolved
config/package-is-installable/test/checkPlatform.ts Outdated Show resolved Hide resolved
@zkochan
Copy link
Member

zkochan commented Oct 18, 2023

We should also ensure that if someone modifies the supportedArchitectures field, pnpm will install the new included artifacts and symlink them to the dependent packages.

Maybe we'll have to add supportedArchitectures to node_modules/.modules.yaml and check if they match with the supportedArchitectures in package.json. If they don't match, pnpm should do full resolution. I don't know if it will be required. First the tests should be added.

@nachoaldamav
Copy link
Contributor Author

nachoaldamav commented Oct 18, 2023

We should also ensure that if someone modifies the supportedArchitectures field, pnpm will install the new included artifacts and symlink them to the dependent packages.

I just tested something, install everything, change the field and install again (keep pnpm-lock.yaml and node_modules), the new ones will be added but not symlinked, it won't remove anything that was removed from the package.json.

# Install deps with all the fields as `current` (Darwin arm64)
Found 1 esbuild binaries:
  - @esbuild+darwin-arm64@0.18.20

# Add win32 to the OS list, keep current, install deps
win32 should be listed below
Found 2 esbuild binaries:
  - @esbuild+darwin-arm64@0.18.20
  - @esbuild+win32-arm64@0.18.20
 
# Remove current from the OS list, keep win32, install deps
Only win32 should be listed below
Found 2 esbuild binaries:
  - @esbuild+darwin-arm64@0.18.20
  - @esbuild+win32-arm64@0.18.20

@zkochan
Copy link
Member

zkochan commented Oct 18, 2023

I know, it won't remove anything from node_modules/.pnpm due to this setting: https://pnpm.io/npmrc#modules-cache-max-age

but it should update the symlinks.

@zkochan zkochan requested a review from gluxon as a code owner October 21, 2023 12:19
@zkochan zkochan requested a review from a team October 23, 2023 01:50
@segevfiner
Copy link

@nachoaldamav Why is this in the package.json though? Doesn't this make more sense as configuration that can also be set via CLI and config files?

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.

Cross platform / architecture install option
3 participants