rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.86k stars 2.43k forks source link

Allow cyclic dependencies between dependency graphs where possible (coming from the new resolver) #8734

Open mehcode opened 4 years ago

mehcode commented 4 years ago

Describe the problem you are trying to solve

Given two crates foo and foo-macro, where foo-macro has a dependency on foo, foo should be able to optionally depend on foo-macro via a cargo feature (as long as foo-macro does not activate the feature).

Describe the solution you'd like

See above.

Notes

With the new v2 dependency resolver (amazing stuff by the way, this solves a lot of other various wrinkles I've stumbled across), it feels like this should be fairly straight forward to solve for someone familiar with the cargo internals. My guess would be the checker for cyclic dependencies simply now has false positives and adjusting the check to recognize the above scenarios would simply work.


For a more concrete use case, SQLx currently has three primary crates: sqlx, sqlx-core, and sqlx-macros. All the sqlx crate does is forward everything from sqlx-core and sqlx-macros into a single crate. sqlx-macros needs to use sqlx-core. This feature request would remove the need to have the sqlx-core crate.

Eh2406 commented 4 years ago

I do remember discussing moving the cycle check to the "feature resolver" from the "dependency resolver" with @ehuss. I remember we thought it would cause problems, but at the moment I don't remember precisely what. One difficulty is that the output of the "dependency resolver" is currently a DAG, and much of Cargo therefore does not need checks for loops. If the only code that looks at the output of the "dependency resolver" is "feature resolver" then this is not much of a problem.

jhpratt commented 4 years ago

I would love to have this for the time crate. I have currently just stripped down the code from the main crate and reused it in the macro crate, but this is needlessly wasteful. Having this would also make it easier to implement the macro itself, for what it's worth.

lausek commented 3 years ago

I just want to summarize my research into this topic so far. The point of failure seems to be at check_cycles:1026. This algorithm simply does not recognize different feature configurations of packages even though it should (?). As far as I understand, the current implementation also evaluates optional dependencies regardless of specified features.

My fork of cargo was extended by a test case that should pass after this feature was implemented.

cargo build
     +
     |
     v
   parent +-> child +-> parent
     1)         2)        3)

1) Is the starting point of the process. parent includes child as default feature. 2) Child gets checked. It specifies parent as dependency, setting default-features to false therefore not including child. 3) parent should not have dependencies now.