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

Publish wasm API to npm #12317

Merged
merged 16 commits into from
Jul 17, 2024
Merged

Publish wasm API to npm #12317

merged 16 commits into from
Jul 17, 2024

Conversation

mattrunyon
Copy link
Contributor

@mattrunyon mattrunyon commented Jul 14, 2024

Summary

Will need to add an actions secret NPM_TOKEN that is an npm token w/ publish rights to the @astral-sh org. It seems right now the "best" option for is a classic automation token which never expires (npm scoped tokens expire which would cause this to randomly fail after it expires).

Fixes #1385

Added ruff_wasm to the pyproject version_files.

Added a build and publish wasm job with cargo-dist. I am not familiar with this toolchain, but I regenerated the publish action. Note this does not add the wasm build to the GH release artifacts.

This will publish @astral-sh/ruff-wasm-{target} where target is web, bundler, or nodejs

Test Plan

I test published this in my fork with a few modifications

  • workflow_dispatch for the build/publish action so it could be run standalone
  • Published under my npm scope
  • Changed repo URL to my fork so npm provenance works
  • Bumped the version for the 2nd successful publish to check the LICENSE file copied properly

Workflows: https://github.com/mattrunyon/ruff/actions/workflows/publish-wasm.yml
npm: https://www.npmjs.com/package/@mattrunyon/ruff-api

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Wow this is neat.

I prefer to keep the old ruff_wasm crate name. ruff_api feels to generic as a crate name.

I see that this package now only publishes the web bindings. I wonder if we need multiple packages similar to what BiomeJS does (they actually have an even more complicated setup) where they have a web, node (possibly deno) and bundler package.

crates/ruff_wasm/Cargo.toml Outdated Show resolved Hide resolved
crates/ruff_wasm/README.md Show resolved Hide resolved
@mattrunyon
Copy link
Contributor Author

mattrunyon commented Jul 15, 2024

I've updated the warning and also adjusted the naming w/ a CI matrix job to publish 3 wasm-pack targets. I haven't used deno, but looks like it doesn't use package.json so it would need its own slightly different job probably

Test run on my fork here: https://github.com/mattrunyon/ruff/actions/runs/9935706011
Example should be published in a few minutes to https://www.npmjs.com/package/@mattrunyon/ruff-wasm-web

@MichaReiser
Copy link
Member

Nice thank you. We now need @charliermarsh to add the NPM token.

@charliermarsh
Copy link
Member

Thanks for the ping -- I'll get to this today.

@charliermarsh
Copy link
Member

@MichaReiser -- I added NPM_TOKEN to the repo with read/write access on @astral-sh.

Copy link
Member

Choose a reason for hiding this comment

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

We need to add the Cargo.toml to

ruff/pyproject.toml

Lines 105 to 112 in 7a7c601

version_files = [
"README.md",
"docs/integrations.md",
"crates/ruff/Cargo.toml",
"crates/ruff_linter/Cargo.toml",
"scripts/benchmarks/pyproject.toml",
]

or it won't get automatically updated when doing the next release.

@MichaReiser
Copy link
Member

Thanks @charliermarsh . Stupid question. How can I dry-run the workflow? And is the trigger for the npm workflow in the correct release step?

@mattrunyon
Copy link
Contributor Author

mattrunyon commented Jul 16, 2024

I saw something about a tag=dry-run in some other workflows. If that actually runs the publish jobs, then could add a check to instead run npm publish --dry-run if that's the case. It will just print out what would have happened, but doesn't actually check the validity of the token (it will warn if there was no token)

@mattrunyon
Copy link
Contributor Author

Nevermind, looks like dry-run probably doesn't publish based on this line in publish.yml

publishing: ${{ inputs.tag && inputs.tag != 'dry-run' }}

@MichaReiser
Copy link
Member

Hmm okay. Let's see if @charliermarsh has an idea on how to test it or we'll test on release day 😆

@mattrunyon
Copy link
Contributor Author

mattrunyon commented Jul 16, 2024

If anybody w/ more knowledge on the release system wants to adjust this in the future to separate build and publish, you would basically run everything except the setup-node and publish steps and upload the resulting crates/ruff_wasm/pkg to an artifact. Then download the artifacts, run setup-node and then npm publish for each.

Could also run setup-node in both build and publish and then run npm pack crates/ruff_wasm/pkg --pack-destination some_optional_dir_if_not_current_dir to create a tarball. Then upload the resulting <name>-<version>.tgz. Then npm publish <name>-<version.tgz those

Comment on lines +33 to +34
- uses: jetli/wasm-pack-action@v0.4.0
- uses: jetli/wasm-bindgen-action@v0.2.0
Copy link
Member

Choose a reason for hiding this comment

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

Most our workflows use taiki-e/install-action for fast installation of cargo dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stole this setup from the playground workflow, so might want to update both at some point

@charliermarsh
Copy link
Member

@MichaReiser - You should be able to release with dry-run as the tag (it should be the pre-populated default).

The check in GitHub Actions is ${{ inputs.plan != '' && !fromJson(inputs.plan).announcement_tag_is_implicit }}. You can see an example in build-docker.yml.

@mattrunyon
Copy link
Contributor Author

There is currently nothing handling dry-run in the action and build/publish are in the same workflow. So fair warning if the action runs at all it will try to publish. I mostly wasn't sure about how the different release steps worked w/ dry-run and I thought the publish jobs were skipped entirely w/ dry-run

@charliermarsh
Copy link
Member

@mattrunyon - I will check...

@charliermarsh
Copy link
Member

Confirmed that publish jobs do not run with dry-run.

@charliermarsh
Copy link
Member

I think I would make just make the new job workflow_dispatch too and then skip the publish step if ${{ inputs.plan == '' || fromJson(inputs.plan).announcement_tag_is_implicit }}, so we can test the rest manually without publishing.

@charliermarsh
Copy link
Member

LGTM! I’ll let @MichaReiser merge.

@mattrunyon
Copy link
Contributor Author

Here's a run from my fork: https://github.com/mattrunyon/ruff/actions/runs/9963353823

If there's no token, the 2nd or 3rd to last line of the dry-run publish output will say something like "warning: You need to login to npm"

Due to how Github actions work, there's no way to dry-run this until it merges (since there's no workflow w/ the same name already on main)

@MichaReiser MichaReiser merged commit fe04f2b into astral-sh:main Jul 17, 2024
38 checks passed
@MichaReiser
Copy link
Member

MichaReiser commented Jul 17, 2024

Thanks again @mattrunyon for this amazing contribution. The dry run was completed successfully.

https://github.com/astral-sh/ruff/actions/runs/9969546556/job/27546697354

@mattrunyon mattrunyon deleted the npm-publish branch July 17, 2024 07:00
@MichaReiser
Copy link
Member

Yes, the packages should be published once we do the next release.

@dhruvmanila

This comment was marked as duplicate.

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.

Upload WASM packages for each release
4 participants