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.

Sign up for GitHub

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

Jump to bottom

Cargo regression in dependency resolution #11330

Closed
Mark-Simulacrum opened this issue Nov 3, 2022 · 0 comments · Fixed by #11331
Closed

Cargo regression in dependency resolution #11330

Mark-Simulacrum opened this issue Nov 3, 2022 · 0 comments · Fixed by #11331
Assignees
@weihanglo

Comments

@Mark-Simulacrum
Copy link
Member

Caught by rustc-perf's testing after merging rust-lang/rust#103860 -- see https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/cargo.20and.20rustc.20benchmarks.20broken

Posting and self-approving a revert in rust-lang/rust#103922

I can reproduce this by building the master branch (810cbad) and then:

Previously

cargo 1.67.0-nightly (7e484fc 2022-10-27)

$ rm -rf testing && cargo new --bin testing && cd testing && cargo add serde --features derive && cargo tree
     Created binary (application) `testing` package
    Updating crates.io index
      Adding serde v1.0.147 to dependencies.
             Features:
             + derive
             + serde_derive
             + std
             - alloc
             - rc
             - unstable
testing v0.1.0 (/home/mark/Edit/testing)
└── serde v1.0.147
    └── serde_derive v1.0.147 (proc-macro)
        ├── proc-macro2 v1.0.47
        │   └── unicode-ident v1.0.5
        ├── quote v1.0.21
        │   └── proc-macro2 v1.0.47 (*)
        └── syn v1.0.103
            ├── proc-macro2 v1.0.47 (*)
            ├── quote v1.0.21 (*)
            └── unicode-ident v1.0.5

On cargo master:

rm -rf testing && cargo new --bin testing && cd testing && cargo add serde --features derive && cargo tree
     Created binary (application) `testing` package
    Updating crates.io index
      Adding serde v1.0.147 to dependencies.
             Features:
             + derive
             + serde_derive
             + std
             - alloc
             - rc
             - unstable
testing v0.1.0 (/home/mark/Edit/testing)
└── serde v1.0.147
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Nov 3, 2022
Merged
@weihanglo weihanglo self-assigned this Nov 3, 2022
@weihanglo weihanglo mentioned this issue Nov 3, 2022
Merged
bors added a commit that referenced this issue Nov 3, 2022
@bors
Auto merge of #11331 - weihanglo:revert-11183, r=ehuss
4111d42 
Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19e0..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102
bors added a commit that referenced this issue Nov 3, 2022
@bors
Auto merge of #11331 - weihanglo:revert-11183, r=ehuss
d828b5b 
Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19e0..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102
@bors bors closed this as completed in 7d8d028 Nov 3, 2022
This was referenced Dec 13, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@weihanglo weihanglo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Revert #11183 weihanglo/cargo
2 participants
@Mark-Simulacrum @weihanglo