Add bitcoin_hashes features

[tcharding]

We re-export bitcoin_hashes so users do not have to depend directly on
it. This is Rust best practice as makes keeping versions in sync easier.

Currently, however, it is not possible to enable different features in
bitcoin_hashes and our feature enables it with no default features -
this makes the re-export basically useless.

Add a bunch of features that can be used to turn on the various
bitcoin_hashes features.

Note

The re-export/feature problem is not specific to this repo, I'd think there was a standard way to solve this. The solution presented here is made up by me so I'm inclined to think there is probably a better, more standard, solution. Hence this PR is draft for feedback please. We suffer the same problem all the way up our stack and its going to get worse when we start crate smashing.

Done while investigating: rust-bitcoin/rust-bitcoin#819 (comment)

[Kixunil]

If we're going to reexport things I think it only makes sense to reexport features that are actually used in conditions here. In this case bitcoin-hashes-std. However I suspect your approach may help with forward compatibility. Is it worth 2^n complexity, though?

[apoelstra]

I also am very uncomfortable with the 2^n complexity here. I've run into this problem many times when using re-exports from the rust-bitcoin dependency tree but I don't think it's reasonable to try and solve it this way. (This is only one dependency from one crate, and it brings in 5 new feature gates!)

[thomaseizinger]

I think we should activate this with our std feature.

[thomaseizinger]

Additionally, once we are in a higher MSRV, we can use "weak-dependency" features here then!

[thomaseizinger]

If we batch these together with our features, the list shouldn't explode as much.

[Kixunil]

We would have to mandate bitcoin_hashes in combination with std which doesn't seem right. Weak dependencies will take optimistically about a year and a half to be allowed here. (But once they are, hell yeah, let's use them.)

[thomaseizinger]

We would have to mandate bitcoin_hashes in combination with std which doesn't seem right.

Yes but also, the cost is negligible thought, isn't it? Esp. when weighted against the API complexity of all these features.

My vote would go down for either:

• Tie all bitcoin_hashes features to the respective bitcoin features
• Force users to depend on bitcoin_hashes separately if they want to activate features

[Kixunil]

I'm not sure what do you mean by "Tie all bitcoin_hashes features to the respective bitcoin features". Note that we're in secp256k1 repo.

Force users to depend on bitcoin_hashes separately if they want to activate features

We can't do this because we don't get to learn which features were activated in bitcoin_hashes. I already tried this in my own project and failed.

But now I also remembered one super-annoying issue:

• Crate A has std feature to impl std::error::Error
• Crate B has same std feature as A, optionally depends on A, and has an error type that has an error type from A as a source
• Crate C depends on B with std, without A
• Crate D depends on B without std with A

The compilation will fail because A/std is not enabled so the impl is missing but B tries to use it because std is enabled.
The only solution to this (other than future Rust version) I can think of is don't return Some from source when A is activated without a special feature - one exactly like in this PR.

Actually we should check for these cases in all the crates...

[thomaseizinger]

Note that we're in secp256k1 repo.

Whoops my bad, I meant secp256k1 obviously. With tie all features, I meant:

[features]
std = ["bitcoin_hashes/std"]

• Crate C depends on B with std, without A

• Crate D depends on B without std with A

Features get unified across a dependency-graph so we should end up with B/std and A, no?

• Crate B has same std feature as A, optionally depends on A, and has an error type that has an error type from A as a source

We shouldn't have cfg'd enum variants if that is what you are describing here. Those will cause headache across dependency graphs!

I think the Rust idiomatic way is to forward all features to your dependencies, i.e.

std feature in crate A enables an std feature (if present) in all its dependencies. Up until "weak-dependency features" land and/or can be used, this will automatically enable optional dependencies if you enable such a feature. That is a price we need to evaluate whether it is worth paying. Our dependency graph is pretty small anyway so this might not be a big deal on the whole. YMMV.

[Kixunil]

Features get unified across a dependency-graph so we should end up with B/std and A, no?

Only if we do it like you suggest which has the downside of potentially pulling in unneeded dependency.

We shouldn't have cfg'd enum variants if that is what you are describing here.

That's not the issue and there are several ways to deal with it even without #[non_exhaustive] (but we will bump MSRV so #[non_exhaustive] is the way to go. So no, that's not the issue. The issue is impl std::error::Error for MyError conditional on std feature.

forward all features

IMO there's no great solution today. There are multiple bad solutions, forwarding only required features seems the least bad.

[Kixunil]

Why was this closed?