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

Feature flags: js, ~~rustix~~ #574

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jan 1, 2025

This adds two feature flags: "js" and "rustix". These both enable the corresponding backend and use it by default (still allowing override with getrandom_backend, except that wasm32 sans OS has no viable alternative).

Result: a minimal crate dependant on getrandom has its lockfile reduced from 33 to 16 entries. Going further than this would require disabling WASI and/or Windows targets by default (windows-targets in particular adds a lot of entries).

Compromise: a direct dependency on getrandom is needed to enable the required target. And since this also default-enables the target, it likely should only be added in the final binary, as with getrandom v0.2. I considered not default-enabling the backend (i.e. one needs to both use --features=js and getrandom_backend=wasm_js to use the backend) but this is likely confusing (besides being tedious).

I'm not sure whether this is actually a good idea, but I don't see a better alternative than simply choosing not to address the issue of a large lockfile brought up here.

@dhardy dhardy requested review from newpavlov and josephlr January 1, 2025 17:24
@newpavlov
Copy link
Member

As I wrote in the issue, I think it's better to introduce an opt-in feature which would allow users to enable optional backends using configuration flags. It would resolve the lock file bloat issue and would be universal for all opt-in backends.

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

dhardy commented Jan 3, 2025

@newpavlov features must be additive, so we can't use an opt-in feature to remove functionality which would otherwise be available by default. And what do you mean by "configuration flags"? Isn't that what getrandom_backend already is, which is already causing lockfile bloat due to not trimming the dependency tree according to platform or configuration flags?

The only real alternatives I see here are (1) something similar to this PR but don't enable the backend by default (i.e. require using the feature flag and setting getrandom_backend to use wasm_js) or (2) the status quo (decide not to fix the bloat; maybe Cargo will eventually but I have my doubts since I wouldn't expect a configuration flag to affect Cargo.lock; in fact this is what @hanna-kruppe said in the other thread).

@dhardy
Copy link
Member Author

dhardy commented Jan 3, 2025

Merge due to #573.

@dhardy dhardy changed the title Feature flags: js, rustix Feature flags: js, ~~rustix~~ Jan 3, 2025
@newpavlov newpavlov mentioned this pull request Jan 9, 2025
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