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

Include all builds as normal dependencies #449

Merged
merged 5 commits into from
May 15, 2024
Merged

Conversation

fvictorio
Copy link
Member

Hardhat users are frequently running into npm/cli#4828. As a blunt workaround, we are going to turn all the builds into dependencies of the entry-point package @nomicfoundation/edr, instead of being optional dependencies. For this to work, two things have to happen:

  1. The package.json of EDR has to use them as plain dependencies instead of optionalDependencies
  2. The package.jsons of the builds don't have to include things like os and arch, because the package manager might check them and complain if they don't match.

Since the optionalDependencies field of the entry-point package.json are added/modified by the NAPI-RS CLI, the only way to deal with 1 is to modify the package.json after running napi prepublish. This is done with jq and sponge (sponge is needed because you can't do jq '...' package.json > package.json and jq doesn't have a "modify in place" flag like sed's -i). sponge doesn't come pre-installed, so we need to install moreutils in the publish job.

For 2, it's enough to just modify the package.jsons we already have under crates/edr_napi/npm. The arch, os and libc fields are not re-added by napi.

Instead of using optional dependencies, make the `@NomicFoundation/edr`
package have all the NAPI-RS builds as dependencies.

This is a way to prevent npm/cli#4828 from
affecting users.
@fvictorio fvictorio requested a review from agostbiro May 14, 2024 15:09
Copy link

changeset-bot bot commented May 14, 2024

🦋 Changeset detected

Latest commit: 306594d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/edr Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@fvictorio fvictorio had a problem deploying to github-action-benchmark May 14, 2024 15:09 — with GitHub Actions Error
@fvictorio fvictorio temporarily deployed to github-action-benchmark May 14, 2024 15:10 — with GitHub Actions Inactive
@@ -17,11 +17,15 @@
"napi": {
"name": "edr",
"triples": {
"defaults": false,
Copy link
Member Author

Choose a reason for hiding this comment

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

This change isn't strictly necessary for this PR, but while working on this I noticed that slang does it and I think it makes sense. This way we don't rely on napi-rs's defaults that might change without us noticing it.

@fvictorio
Copy link
Member Author

Also: I tested this in the prerelease branch (job) and it seems to work. You can install @nomicfoundation/[email protected] to try it.

Copy link
Member

@agostbiro agostbiro left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! I tested out @nomicfoundation/[email protected] on MacOS ARM with a fresh install and it worked well 🎉

@fvictorio fvictorio merged commit caa6f85 into main May 15, 2024
32 checks passed
@fvictorio fvictorio deleted the publish-all-builds branch May 15, 2024 08:05
@Wodann Wodann added this to the EDR v0.3.9 milestone May 21, 2024
@Wodann Wodann modified the milestones: EDR v0.3.9, EDR v0.3.8 May 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants