-
Notifications
You must be signed in to change notification settings - Fork 595
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
Add feature to build for web #1921
Conversation
@@ -34,6 +34,7 @@ logging = ["log"] | |||
aws_lc_rs = ["dep:aws-lc-rs", "webpki/aws_lc_rs"] | |||
aws-lc-rs = ["aws_lc_rs"] # Alias because Cargo features commonly use `-` | |||
ring = ["dep:ring", "webpki/ring"] | |||
web = ["ring/wasm32_unknown_unknown_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.
Isn't this something that could be activated outside of rustls by a dependent crate that wanted to use *ring*
by taking its own direct dependency w/ the feature enabled?
If a feature for this were to be added here, should it also activate pki-types/web
?
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.
Also should this be ring?/wasm32_unknown_unknown_js
to avoid enabling the ring dependency in scenarios where the aws-lc-rs provider is actually selected?
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.
Isn't this something that could be activated outside of rustls by a dependent crate that wanted to use
*ring*
by taking its own direct dependency w/ the feature enabled?If a feature for this were to be added here, should it also activate
pki-types/web
?
Yes indeed, they don't have the feature yet but I made a PR, let's wait to see if they merge it
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.
Also should this be
ring?/wasm32_unknown_unknown_js
to avoid enabling the ring dependency in scenarios where the aws-lc-rs provider is actually selected?
I thought it would only enable ring if I wrote it that way web = ["dep:ring", "ring/wasm32_unknown_unknown_js"]
but I will check
I think we are probably overdue for regularising wasm support in this crate. What I mean by that is:
|
@Mubelotix Are you interested in trying to achieve the broader changeset Ctz described above? |
Sure. I only have experience with wasm in the browser though, so that's what the example would be about |
Personally my understanding of the general ecosystem here is lacking, but I think we have a light preference for WASI over an example fixed to the browser context. I don't know how far the two goals are from one another to gauge if it would be helpful to consider a browser example as a stepping stone towards a more complete WASI example. |
Ok I will look into WASI |
I think it's fair to close this for now while we wait on a reworked version closer to the goals Ctz outlined in his comment. Feel free to re-open once you have revisions ready! |
The build for wasm32-unknown-unknown with ring used to fail, but with this feature it works out of the box