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

Allow building for the web #69

Closed
wants to merge 3 commits into from
Closed

Allow building for the web #69

wants to merge 3 commits into from

Conversation

Mubelotix
Copy link

No description provided.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

👋 Thanks for the PR. I had a couple initial comments.

Comment on lines 24 to 26
[features]
default = []
web = ["pki-types/web"]
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a usecase for building this crate for a web target? It's primarily used for codegen for the data vendored into the webpki-roots crate.

webpki-roots/Cargo.toml Show resolved Hide resolved
@ctz
Copy link
Member

ctz commented Apr 26, 2024

I don't think this is necessary? There's no reason that a crate which takes a dependency must re-export or otherwise control the dependency's features.

If you want to set pki-types/web in your application, just write in your Cargo.toml: pki-types = { version = "*", features = ["web"] } -- job done, no need for any other crates to change.

(The version = "*" is only appropriate if you do not use any of the APIs from pki-types yourself.)

@Mubelotix
Copy link
Author

Mubelotix commented Apr 26, 2024

I don't think this is necessary? There's no reason that a crate which takes a dependency must re-export or otherwise control the dependency's features.

If you want to set pki-types/web in your application, just write in your Cargo.toml: pki-types = { version = "*", features = ["web"] } -- job done, no need for any other crates to change.

(The version = "*" is only appropriate if you do not use any of the APIs from pki-types yourself.)

This is what I have been doing before but it doesn't feel right having to dig into the dependency tree to fix all compile errors by adding features with no standardized name.

I removed the feature on webpki-ccadb as it wasn't enough to get it compiling and I only needed webpki-roots anyway

@Mubelotix
Copy link
Author

I made a similar PR on rustls#1921 btw

@djc
Copy link
Member

djc commented Apr 26, 2024

I don't think this is necessary? There's no reason that a crate which takes a dependency must re-export or otherwise control the dependency's features.

I agree it's not strictly necessary, but it usually makes things easier for the consumer at little cost to the maintainer?

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

I think this would be okay to have.

What we really want here is target-dependent features, which I think we can't do? I don't remember why we didn't go with that in the rustls-pki-types PR?

@ctz
Copy link
Member

ctz commented Apr 26, 2024

I agree it's not strictly necessary, but it usually makes things easier for the consumer at little cost to the maintainer?

I think a basic level of testing is that all combinations of a crate's features are tested. If we have features which do not vary the code or dependencies, that adds new dimensions to that testing for no benefit and eventually makes that style of testing implausible.

I think a crate should only have features that either change its dependencies or behaviour; but not just pass through to some other crate's features. I especially think that webpki-roots is an odd choice to do that.

@djc
Copy link
Member

djc commented Apr 26, 2024

I think a crate should only have features that either change its dependencies or behaviour; but not just pass through to some other crate's features. I especially think that webpki-roots is an odd choice to do that.

That is fair. And if we add a flag for this in rustls, we probably wouldn't strictly need it here?

@cpu
Copy link
Member

cpu commented Jun 3, 2024

Since a change on the Rustls side will probably require more thought/care are we OK with merging this or should it be closed pending that work? Since it sounds like it wouldn't be necessary if that work happened, and it likely isn't very valuable on its own, I was leaning towards close. WDYT?

@djc djc closed this Jun 3, 2024
@djc
Copy link
Member

djc commented Jun 3, 2024

Agreed, let's close this for now.

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.

4 participants