-
Notifications
You must be signed in to change notification settings - Fork 55
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
Support compiling to wasm32 architectures #221
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's cool. Would it be possible to add an example of this kind of usage?
Cargo.toml
Outdated
openidconnect = { version = "2.3", default-features = false, features = [ "reqwest" ], optional = true} | ||
getrandom = { version = "0.2.8", features = ["js"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is something we have to do, quoting the documentation of the getrandom crate:
This feature should only be enabled for binary, test, or benchmark crates. Library crates should generally not enable this feature, leaving such a decision to users of their library. Also, libraries should not introduce their own js features just to enable getrandom’s js feature.
I think a better approach would be to:
- Extend our documentation explaining how to use this crate when one of the WebAssembly targets is used
- Move the
getrandom
dependency to the dev-dependencies. This will ensure the unit tests can be run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add a 'wasm' feature (and document it) to the crate that enabled it instead? I think it's a bit strange to require 'user' crates to set features on transient dependencies in order for things to compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already ran into some crates that require that, so it doesn't feel so strange to me. However, I think adding a wasm
feature would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've update to enable the js flag only if the wasm feature is enabled, and also added some info to the readme.
d93bfb4
to
5344ae9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can be merged once all the tests are green
Enable support for compiling sigstore-rs to WASM, making it usable from browser applications and on wasm32 VMs. Signed-off-by: Ulf Lilleengen <lulf@redhat.com>
Clippy and integration tests compile now, and I squashed the commits. I think the GHA should be correct as well. |
Summary
Enable support for compiling sigstore-rs to WASM, making it usable from browser applications and on wasm32 VMs.
Example application using the Yew framework: https://github.com/trustification/sigstore-search
Closes #220