search

dalek-cryptography / curve25519-dalek

A pure-Rust implementation of group operations on Ristretto and Curve25519
Other
884 stars 448 forks source link

Incorrect use of cfg to import dependency #632

Closed kotval closed 7 months ago

kotval commented 7 months ago

According to the cargo docs, in a Cargo.toml you cannot use 

target.'cfg(feature = "fancy-feature"'.dependencies]

to add dependencies based on optional features. It seems that doing this is the intent of https://github.com/dalek-cryptography/curve25519-dalek/blob/a62e4a5c573ca9a68503a6fbe47e3f189a4765b0/curve25519-dalek/Cargo.toml#L62C1-L64C1. The result is that even when selecting the "serial" backend with rustflags, a downstream project depending on this crate will have fiat-crypto as a dependency. The result seems to be that that line in the Cargo.toml equivalent to just being a regular dependency, and thus specifying it as such seem to be a mistake. I recommend adding a feature for each backend choice which needs separate dependencies in the Cargo.toml:

[dependencies]
fiat-crypto = { version = "0.2.1", default-features = false, optional = true}
[features]
fiat_backend = ["dep:fiat-crypto"]

and then adding something like to the build.rs 

if cfg!(feature = "fiat_backend") {
            println!("cargo:rustc-cfg=curve_dalek_backend=\"fiat\"");
       ...

This lets you to leave the cfg flags in the code unchanged, and have conditionally included dependencies. Checking the mutual exclusivity of certain features which could possibly correspond to inconsistent cfg flags can still be done in the build.rs.

Note: the same comment applies to the condition to select curve25519-dalek-derive in the same Cargo.toml.

tarcieri commented 7 months ago

We're not using target.'cfg(feature = ...), so at best the way this issue is filed is misframed.

We migrated from cargo features to cfg attributes very deliberately as part of the last release, so we have definitely considered that approach.

Cargo features are a poor fit for this use case because backends are 1-of-n, whereas features must be purely additive. They caused all sorts of problems because dependencies would activate mutually incompatible backends causing compilation failures.

cfg attributes make 1-of-n selection better but have their pros and cons. You have identified one of the cons.

An ideal solution to the problem might look like mutually exclusive, global features, but we have to work with what we have.

I would suggest reviewing the past issues where this has been discussed, namely #414

At any rate, we won't be switching back to feature-based backend selection since it's a poor fit for selecting a 1-of-n backend. Closing this.