Use a runtime-rng feature flag instead of using OS detection

[Amanieu]

The current OS detection is breaking my code which uses a custom linux target that doesn't have a libc.

ahash should have zero dependencies when built with default-features = false.

[tkaitchuck]

Wouldn't this be better handled at the getrandom level?
aHash is pulling in getRandom which as a detection that it is on a unix system and if so adds a dependency on libc. (Presumably this for the getrandom system call). However it also supports a fallback if this is not available. It seems like the solution for a linux that lacks libc would be to have getRandom compile to use the fallback and not pull in libc in the first place. This would be more generally useful because it would apply to not only aHash but anything else using getrandom.

[tkaitchuck]

Does target_env help here?

[Amanieu]

I am using a completely custom target specified using a json config. I don't think it is reasonable to modify upstream projects to specifically handle my custom target.

I would much prefer if this was handled in ahash: there's no reason to pull in getrandom when runtime rng is not required. This is the case for hashbrown's default hasher which uses ahash with no-default-features.

[tkaitchuck]

This is the case for hashbrown's default hasher which uses ahash with no-default-features.

If a source of Randomness is available, it absolutely should be used with Hash brown.
I am not clear how to express this sort of thing with features. As far as I know there isn't a way to specifically exclude a feature.
If that is true and If there is no way to detect libc's absence by the target or dynamically, I don't see any alternative besides adding another feature flag which disables it.

[Amanieu]

The proper way to express this would be for hashbrown to expose a feature to enable runtime rng which forwards to the ahash feature.

[tkaitchuck]

We want the best available source of Randomness to be used. If we reduce it to a flag, then it throws it on the user to set it for their platform. This likely means using a default which is the least common denominator, and trying to persuade people to lookup their platform and enable a better source.

I don't like that outcome because the use of aHash is very likely indirect. IE an app depends of a library which in turn uses it. If such a library is to be genetic it cannot make assumptions about the platform, so the app will be unaware and end up getting a poor source of Randomness even though a good one is available.

If you are in control of the custom target, you could specify the SYS or the ABI as "none": https://docs.rust-embedded.org/embedonomicon/custom-target.html
That would be easy to detect in the build.rs.

[Amanieu]

We want the best available source of Randomness to be used. If we reduce it to a flag, then it throws it on the user to set it for their platform. This likely means using a default which is the least common denominator, and trying to persuade people to lookup their platform and enable a better source.

Randomness for a hasher is only useful in the context of HashDoS. hashbrown specifically makes no promises that the default hasher is HashDoS-resistant. I am only using AHash as the default hasher because it is a generally good hash function and avoids the poor quality of FxHash in certain cases.

Anyone who specifically requires HashDoS-resistance should be using the HashMap types from the ahash crate directly, or use the standard library HashMap.

[tkaitchuck]

Randomness for a hasher is only useful in the context of HashDoS

Strong randomness, yes. Weak randomness is always required, but counters etc can provide that.

Anyone who specifically requires HashDoS-resistance should be using the HashMap types from the ahash crate directly, or use the standard library HashMap

Yes, agreed. I would like a way to support this for hashbrown without it affecting people using aHash directly unless they opt in.
Perhaps we could do this by splitting aHash into two crates. One which contains the build.rs and wraps another which has the actual code. Then Hashbrown could depend on the inner crate, but anyone just using aHash would get the auto-detection logic.

[Amanieu]

The runtime-rng flag added by this PR is enabled by default so anyone using the ahash crate directly will automatically have it enabled. However if they import it via hashbrown then it will not be enabled since hashbrown imports ahash with default-features = false.

[tkaitchuck]

Yes. But how is another library supposed to use it? If some other hashMap library does want randomness this PR forces those libraries to either be like hashbrown and not provide DOS protection or have their builds fail on platforms where random is not available.

I'm not even sure it really solves the original problem particularly well. The root of the issue is that aHash is using getrandom which declares:

[target.'cfg(unix)'.dependencies]
libc = { version = "0.2.64", default-features = false }

in it's config.toml and you are running on a unix without a libc. So if anything else ever caused getrandom to be pulled in then you're right back where you started. It's not implausible that some other crate in the build somewhere pulls in aHash, rand, or any of the other packages that depend on getrandom.

If instead we address the root issue and in getrandom change it's path in linux to instead call: https://man7.org/linux/man-pages/man2/getrandom.2.html
and not depend on using libc to read from a file. Then libc is never pulled in in the first place and aHash, rand, and anything else will work out of the box, and everyone's builds will be faster and binaries will be smaller.

There was a proposed PR to do exactly this: rust-random/getrandom#127
but was closed because it was not needed. I think re-opening this PR would be a better solution.

[Amanieu]

Yes. But how is another library supposed to use it? If some other hashMap library does want randomness this PR forces those libraries to either be like hashbrown and not provide DOS protection or have their builds fail on platforms where random is not available.

If another library actually guarantees DoS protection then it should require either runtime or compile-time RNG.

I'm not even sure it really solves the original problem particularly well.

It does solve the problem: it is common in no_std projects (e.g. embedded) to make sure that all of the crates you are using do not depend on std or libc. At the moment, ahash is the only crate which causes a problem for me. This is in fact a regression from #62 since previously the getrandom crate was not imported when the std feature was disabled.

If instead we address the root issue and in getrandom change it's path in linux to instead call

That won't work: you would need to use either libc::syscall (which requires libc) or inline assembly (which is unstable).

[tkaitchuck]

If another library actually guarantees DoS protection then it should require either runtime or compile-time RNG.

Libraries don't know how they are going to be used. Because something isn't available on a custom target, doesn't mean it should be disabled when it is available.
The Rust standard library does exactly the same thing. If you look at https://github.com/rust-lang/rust/blob/673d0db5e393e9c64897005b470bfeb6d5aec61b/library/std/src/sys/unix/rand.rs#L13
you can see it grabs the best source of randomness it can for it's maps but if it's not avalable:
https://github.com/rust-lang/rust/blob/5a4ab26459a1ccf17ef5bb4c841d3ae5517b2890/library/std/src/sys/unsupported/common.rs#L35
It just uses a fixed key. So there is no difference with the standard library's builder. If aHash's HashMap wrapper is truly going to be a drop-in replacement for the standard library HashMap it needs to support detection with at least as many targets. If the method hashmap_random_keys were exposed, I would call it. However it is not, so the best option for this is getrandom.

The only reason getrandom is not an unconditional hard dependency is because it creates the build issues you are seeing on unsupported targets. The problem is your custom target looks like a supported target but isn't.

Above I mentioned "target_env". Per the list here: https://doc.rust-lang.org/nightly/rustc/platform-support.html
it appears this is always set for linux to indicate which ABI is used. If you really don't have one, this being left empty, set to "none", or some other particular string should make this situation easy to distinguish. In which case all that is needed is adding it to the clauses https://github.com/tkaitchuck/aHash/blob/master/build.rs#L13 here and in the toml.

[xu-cheng]

FYI, you may be interested in an alternative solution in #97.

[tkaitchuck]

Changing merge target to simplification branch.