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

Incorrect use of cfg to import dependency #632

Closed
kotval opened this issue Feb 18, 2024 · 1 comment
Closed

Incorrect use of cfg to import dependency #632

kotval opened this issue Feb 18, 2024 · 1 comment

Comments

@kotval
Copy link

kotval commented Feb 18, 2024

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
Copy link
Contributor

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.

@tarcieri tarcieri closed this as not planned Won't fix, can't repro, duplicate, stale Feb 18, 2024
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

No branches or pull requests

2 participants