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

Revert #11183 #11331

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Revert #11183 #11331

merged 1 commit into from
Nov 3, 2022

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Nov 3, 2022

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 --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 = unit_for.map_to_features_for(None));
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }

@rustbot
Copy link
Collaborator

rustbot commented Nov 3, 2022

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2022
@weihanglo weihanglo changed the title Revert "Auto merge of #11183 - weihanglo:issue/10526, r=ehuss" Revert #11183 Nov 3, 2022
@weihanglo
Copy link
Member Author

r? @ehuss

You may want to have a look. Thanks!

@rustbot rustbot assigned ehuss and unassigned epage Nov 3, 2022
@ehuss
Copy link
Contributor

ehuss commented Nov 3, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2022

📌 Commit 5fe2732 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2022
@bors
Copy link
Contributor

bors commented Nov 3, 2022

⌛ Testing commit 5fe2732 with merge 4111d42...

bors added a commit that referenced this pull request Nov 3, 2022
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
Copy link
Contributor

bors commented Nov 3, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 3, 2022
@weihanglo
Copy link
Member Author

#11332 fixes the failing semver-check

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2022
@bors
Copy link
Contributor

bors commented Nov 3, 2022

⌛ Testing commit 5fe2732 with merge d828b5b...

bors added a commit that referenced this pull request Nov 3, 2022
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
@weihanglo
Copy link
Member Author

@bors r-

@bors bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 3, 2022
@bors
Copy link
Contributor

bors commented Nov 3, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Nov 3, 2022
@weihanglo
Copy link
Member Author

@bors retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 3, 2022
@ehuss
Copy link
Contributor

ehuss commented Nov 3, 2022

Retry doesn't work after r-. Retry is only for retrying an already approved PR, but with r- it is no longer in the "approved" state.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2022

📌 Commit 5fe2732 has been approved by ehuss

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 3, 2022

⌛ Testing commit 5fe2732 with merge 7d8d028...

@bors
Copy link
Contributor

bors commented Nov 3, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 7d8d028 to master...

@bors bors merged commit 7d8d028 into rust-lang:master Nov 3, 2022
@weihanglo weihanglo deleted the revert-11183 branch November 4, 2022 07:29
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2022
20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b
2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000
- chore: Upgrade dependencies (rust-lang/cargo#11328)
- Clean more aggressively in CI (rust-lang/cargo#11335)
- Remove remove_dir_all (rust-lang/cargo#11333)
- test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327)
- Revert rust-lang/cargo#11183 (rust-lang/cargo#11331)
- fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332)
- Update curl-sys (rust-lang/cargo#11326)
- Mention fix on build script deadlock (rust-lang/cargo#11325)
- Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285)
- Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242)
- Update the outdated link for rust-semverver (rust-lang/cargo#11322)
- Fix broken link to compilation entry point (rust-lang/cargo#11317)
- Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621)
- Newer anyhow features are required (rust-lang/cargo#11316)
- Clean stale git temp files (rust-lang/cargo#11308)
- Report crate size on package and publish (rust-lang/cargo#11270)
- add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989)
- Update libcurl (rust-lang/cargo#11307)
- artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183)
- Fix singular verb in tests page (rust-lang/cargo#11300)
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2022
Update cargo

20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000
- chore: Upgrade dependencies (rust-lang/cargo#11328)
- Clean more aggressively in CI (rust-lang/cargo#11335)
- Remove remove_dir_all (rust-lang/cargo#11333)
- test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327)
- Revert rust-lang/cargo#11183 (rust-lang/cargo#11331)
- fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332)
- Update curl-sys (rust-lang/cargo#11326)
- Mention fix on build script deadlock (rust-lang/cargo#11325)
- Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285)
- Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242)
- Update the outdated link for rust-semverver (rust-lang/cargo#11322)
- Fix broken link to compilation entry point (rust-lang/cargo#11317)
- Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621)
- Newer anyhow features are required (rust-lang/cargo#11316)
- Clean stale git temp files (rust-lang/cargo#11308)
- Report crate size on package and publish (rust-lang/cargo#11270)
- add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989)
- Update libcurl (rust-lang/cargo#11307)
- artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183)
- Fix singular verb in tests page (rust-lang/cargo#11300)

r? `@ghost`
@ehuss ehuss added this to the 1.67.0 milestone Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
5 participants