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

[Request] Update documentation to mention potential no_std problem. #124

Closed
0ndorio opened this issue Oct 29, 2019 · 1 comment · Fixed by #125
Closed

[Request] Update documentation to mention potential no_std problem. #124

0ndorio opened this issue Oct 29, 2019 · 1 comment · Fixed by #125

Comments

@0ndorio
Copy link
Contributor

0ndorio commented Oct 29, 2019

Disclaimer: This is actually not an issue with hashbrown but with cargo.


Topic: no_std support

While hashbrown itself is able to compile successfully for no_std targets, and even uses the CI pipeline to verify its compatibility to the thumbv6m-none-eabi target, it might still trigger issues if used with other dependencies when the default ahash feature is enabled.

Issue

The dependency tree of ahash injects the getrandom crate if the default feature set is enabled. This crate is heavily platform dependent and does not support targets like the thumbv6 or thumbv7 families.

How does this happen?

The underlying issue is an a long standing and well known cargo bug (rust-lang/cargo#5760) which automatically calculates a required feature set for each (sub-)dependency inside the dependency tree regardless of the dependency origin.

If ahash - with default feature set - is used, it will include the const-random crate and therefore the proc macro crate const-random-macro into the dependency tree. This will request the getrandom feature for the crates rand v^0.7 and indirectly also rand_core v^0.5.1.

As dependencies of proc macros are compiled for the host and not the target architecture this works fine as long as the host architecture is contained in the following list and the no_std build should be successful (as you can see inside the CI).

But it is going to fail as soon as one of the following two conditions are true:

  • The host architecture is not supported.

  • The target architecture is not supported and the dependency tree contains at least one non proc macro dependency, which requires either rand v^0.7 or rand_core v^0.5.1, because than the underlying cargo issue kicks in. Cargo is going to inject the getrandom feature requirement for these crates, originating from the dependency list of const-random-macro, into the other dependency so that cargo is trying to compile getrandom not only for the host architecture but also for the target architecture.

Request

As long as rust-lang/cargo#5760 is still a thing it would be great if at least the hashbrown documentation could get a warning paragraph explaining this topic including how to work around this issue.

Potential workarounds I can see:

  • hashbrown could introduce a way to disable the compile-time-rng feature of ahash if default features are disabled.

  • The relying crate could disable the ahash feature of hashbrown and reintroduce another hasher or ahash without compile-time-rng feature.

Both might result in ahash losing the compile time random seed for the hash values.

@Amanieu
Copy link
Member

Amanieu commented Oct 30, 2019

I'm happy to allow disabling compile-time-rng via a feature. Would you be willing to submit a PR?

0ndorio added a commit to 0ndorio/hashbrown that referenced this issue Oct 30, 2019
Disables the default features of `ahash` and reintroduces them
through a new feature called `ahash-compile-time-rng`, which is
enabled by default.

The new feature makes it possible for depended crates to rely on
`hashbrown` with `ahash` as default hasher and to disable the
`compile-time-rng` at the same time.

This might be useful for any dependend crate targeting `no_std`,
which contains `rand` or `rand_core` somewhere inside the dependency
tree as a bug in cargo accidently enables the underlying `getrandom`
feature if `compile-time-rng` is enabled [1].

... fixes rust-lang#124

[1] rust-lang/cargo#5760
0ndorio added a commit to 0ndorio/hashbrown that referenced this issue Oct 30, 2019
Disables the default features of `ahash` and reintroduces them
through a new feature called `ahash-compile-time-rng`, which is
enabled by default.

The new feature makes it possible for depended crates to rely on
`hashbrown` with `ahash` as default hasher and to disable the
`compile-time-rng` at the same time.

This might be useful for any dependent crate targeting `no_std`,
which contains `rand` or `rand_core` somewhere inside the dependency
tree as a bug in cargo accidentally enables the underlying `getrandom`
feature if `compile-time-rng` is enabled [1].

... fixes rust-lang#124

[1] rust-lang/cargo#5760
bors added a commit that referenced this issue Oct 31, 2019
Introduce `ahash-compile-time-rng` feature.

**Content**

Disables the default features of `ahash` and reintroduces them
through a new feature called `ahash-compile-time-rng`, which is
enabled by default.

The new feature makes it possible for depended crates to rely on
`hashbrown` with `ahash` as default hasher and to disable the
`compile-time-rng` at the same time.

This might be useful for any depended crate targeting `no_std`,
which contains `rand` or `rand_core` somewhere inside the dependency
tree as a bug in cargo accidentally enables the underlying `getrandom`
feature if `compile-time-rng` is enabled [1].

... fixes #124

[1] rust-lang/cargo#5760

---

**Warnings**

 (1) Compiling `ahash` with disabled `compile-time-rng` feature is currently broken and requires tkaitchuck/aHash#25 to be merged in advance.

 (2) This introduces a hidden behavior change for all dependent crates, using hashbrown with `default-features = false` and `features = 'ahash'`. This happens as the `ahash` feature no longer implicitly enables the `compile-time-rng` feature of `ahash`.

---

Is the naming of the feature okay?  Do I need to add any additional  changes?
@bors bors closed this as completed in 2bed868 Oct 31, 2019
@bors bors closed this as completed in #125 Oct 31, 2019
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 a pull request may close this issue.

2 participants