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

argon2: only enable password-hash dependency if its explicit #521

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

BlackHoleFox
Copy link
Contributor

A missing weak feature specifier ? was causing password-hash to always become part of a crate's build tree if someone had specified default-features = false, features = ["alloc"] in their Cargo.toml.

Due to rust-lang/cargo#10801 password-hash will still always appear in the lockfile, but it won't be compiled anymore when a downstream crate has only enabled the alloc or std features.

@tarcieri
Copy link
Member

Looks like the documentation needs to be updated to be gated on password-hash and not just std: https://github.com/RustCrypto/password-hashes/blob/28dfc27/argon2/src/lib.rs#L33

(I guess when it isn't run under cargo hack, something else in the workspace is implicitly enabling the password-hash feature somehow)

@tarcieri
Copy link
Member

Needs rustfmt but looks like it's almost there

@BlackHoleFox
Copy link
Contributor Author

BlackHoleFox commented Aug 26, 2024

Oops, didn't realize the added features would line wrap. At least the failing test command passes locally now.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks!

@tarcieri tarcieri merged commit eefdb54 into RustCrypto:master Aug 26, 2024
17 checks passed
@BlackHoleFox BlackHoleFox deleted the argon2-weak-deps branch August 26, 2024 20:57
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.

2 participants