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

Add opt_in crate feature #580

Closed
wants to merge 1 commit into from
Closed

Add opt_in crate feature #580

wants to merge 1 commit into from

Conversation

newpavlov
Copy link
Member

This feature resolves the lock file bloat issue at the cost of less ergonomic use of opt-in backends.

TODO: update docs

@josephlr josephlr added this to the v0.3 milestone Jan 10, 2025
@dhardy
Copy link
Member

dhardy commented Jan 10, 2025

I think it would make more sense to call this feature wasm_backends. In case another backend were added / made optional, we'd then add another feature for that backend.

I also don't see much point in opt_in affecting other backends like rdrand which aren't adding dependencies.

@josephlr
Copy link
Member

I think the ideal situation would be to get @dhardy's idea in #433 (comment) to work somehow. That way an external crate (say getrandom-js) could have code like:

use getrandom::{register_backend, Error};

register_backend!("js", web_crypto_fill);

unsafe extern "C" fn web_crypto_fill(ptr: *mut u8, len: size) -> Option<Error> {
  // ...
}

And then passing --cfg getrandom_backend="js" would "just work". This would also remove the concept of a "custom" backend, as any custom backend could be declared and used in the same way. The "rdrand", "rndr", and "linux_getrandom" would stay as-is being "builtin" alternative backends.

@dhardy
Copy link
Member

dhardy commented Jan 10, 2025

@josephlr: I don't think register_backend! can work like that: IIUC the only real mechanism we have is to define a #[no_mangle] fn with a precise signature exactly once in the binary.

@newpavlov
Copy link
Member Author

newpavlov commented Jan 10, 2025

I think it would make more sense to call this feature wasm_backends.

The opt_in feature is not WASM-specific. It allows users to enable optional backends, including the ability to overwrite the default backends. The idea here is to have a unified approach which would be the same for any potential future opt-in backend which may bring additional dependencies (e.g. like linux_rustix) without proliferation of opt-in-specific features.

@josephlr
I am not sure how your idea would work. Can you explain it in a more detail?

As I wrote here, one alternative approach is to move WASM JS implementation into a separate crate and rely on the custom backend.

@dhardy
Copy link
Member

dhardy commented Jan 16, 2025

The opt_in feature is not WASM-specific. It allows users to enable optional backends, including the ability to overwrite the default backends. The idea here is to have a unified approach which would be the same for any potential future opt-in backend which may bring additional dependencies (e.g. like linux_rustix) without proliferation of opt-in-specific features.

Using a separate feature flag for each backend (or group of backends for some target platform) makes more sense IMO, so e.g. wasm_backends and rustix_backends (or just js and rustix).

Unless your intention is that --cfg getrandom_backend="X" only be usable when opt_in is enabled... but I'm not sure we can lint that properly (lints.rust.unexpected_cfgs can't be affected by feature flags I think).

@dhardy dhardy mentioned this pull request Jan 16, 2025
@newpavlov
Copy link
Member Author

newpavlov commented Jan 17, 2025

Using a separate feature flag for each backend (or group of backends for some target platform) makes more sense IMO, so e.g. wasm_backends and rustix_backends (or just js and rustix).

What about the custom and rdrand/rndr backends? Note the potential security implications of using crate features for those.

Unless your intention is that --cfg getrandom_backend="X" only be usable when opt_in is enabled...

Yes, it's exactly my intention.

but I'm not sure we can lint that properly (lints.rust.unexpected_cfgs can't be affected by feature flags I think).

Hm, I don't see why it would be a problem. IIUC unexpected_cfgs does not require for listed cfgs to be used in code.

UPD: I updated the code and, as you can see, CI passes without any lint warnings.

@newpavlov
Copy link
Member Author

Closing in favor of #574.

@newpavlov newpavlov closed this Jan 22, 2025
@newpavlov newpavlov deleted the optin branch January 22, 2025 17:05
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.

3 participants