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

Fix panic when artifact target is used for [target.'cfg(<target>)'.dependencies #10433

Merged
merged 11 commits into from
Mar 16, 2022

Conversation

Byron
Copy link
Member

@Byron Byron commented Feb 28, 2022

With an artifact dependency like this in package a

[dependencies.a]
path = "b"
artifact = "bin"
target = "$TARGET"

…and when using $TARGET like this in another package b

[target.'cfg(target_arch = "$ARCHOF_$TARGET")'.dependencies]
c = { path = "../c" }

…it panics with thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId <dbg-info>, but we would expect this to work normally.

Tasks

  • reproduce issue in new test
    • figure out why the test is fixed but the real-world example isn't
  • find a fix

Fixes #10431 and #10452.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2022
@Byron Byron marked this pull request as draft February 28, 2022 02:32
@Byron Byron changed the title [DRAFT] Fix #10431 Fix #10431 Feb 28, 2022
@Byron Byron force-pushed the fix-#10431 branch 2 times, most recently from 26d2f50 to 6e0dbb5 Compare February 28, 2022 02:38
@Byron
Copy link
Member Author

Byron commented Feb 28, 2022

The fix was exactly what @ehuss suggested in the parent issue, and I did my best to describe why that would be done. That description isn't based, however, on a deep understanding of the matter but merely describes what seems to be related to the line.

I wonder if the description could be improved to better describe what's going on.

@Byron Byron marked this pull request as ready for review February 28, 2022 03:37
@Jasper-Bekkers
Copy link

Thanks for taking a stab at this @Byron however, it looks like this doesn't resolve the issue I'm seeing?

@Byron
Copy link
Member Author

Byron commented Feb 28, 2022

Thanks so much for double-checking, @Jasper-Bekkers. I fell for an invalid assumption, thinking that just because I could reproduce the issue with a test that failed exactly at the same spot with a fix as suggested before, it would fix the actual issue.

Indeed, it doesn't work and still fails as before. and I will give it another shot tomorrow.

@Jasper-Bekkers
Copy link

Jasper-Bekkers commented Mar 1, 2022

@Byron I managed to make the test panic as well by changing the d2 dependency to jni = 0.19, so I'm suspecting that it breaks because of a crates.io dependency.

diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs
index 6d19a46f1..04e063a98 100644
--- a/tests/testsuite/artifact_dep.rs
+++ b/tests/testsuite/artifact_dep.rs
@@ -465,8 +465,8 @@ fn feature_resolution_works_for_cfg_target_specification() {
                 version = "0.0.1"
                 authors = []
 
-                [target.'cfg(target_arch = "$ARCH")'.dependencies.d2]
-                path = "../d2"
+                [target.'cfg(target_arch = "$ARCH")'.dependencies]
+                jni = "0.19"
             "#
             .replace("$ARCH", target_arch),
         )
running 1 test
test artifact_dep::feature_resolution_works_for_cfg_target_specification has been running for over 60 seconds
test artifact_dep::feature_resolution_works_for_cfg_target_specification ... FAILED

failures:

---- artifact_dep::feature_resolution_works_for_cfg_target_specification stdout ----
running `C:\Users\Jasper\cargo\target\debug\cargo.exe build -Z bindeps`
thread 'artifact_dep::feature_resolution_works_for_cfg_target_specification' panicked at '
test failed running `C:\Users\Jasper\cargo\target\debug\cargo.exe build -Z bindeps`
error: process exited with code 101 (expected 0)
--- stdout

--- stderr
    Updating crates.io index
 Downloading crates ...
  Downloaded jni v0.19.0
 Downloading crates ...
  Downloaded cesu8 v1.1.0
 Downloading crates ...
  Downloaded combine v4.6.3
 Downloading crates ...
  Downloaded jni-sys v0.3.0
 Downloading crates ...
  Downloaded log v0.4.14
 Downloading crates ...
  Downloaded thiserror v1.0.30
thread 'main' panicked at 'activated_features for invalid package: features did not find PackageId { name: "jni", version: "0.19.0", source: "registry `crates-io`" } NormalOrDevOrArtifactTarget(None)', src/cargo\core\resolver\features.rs:318:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
', tests\testsuite\artifact_dep.rs:502:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    artifact_dep::feature_resolution_works_for_cfg_target_specification

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2338 filtered out; finished in 131.12s

error: test failed, to rerun pass '--test testsuite'

@Jasper-Bekkers
Copy link

Note that I had originally posted this issue in #10405

@Byron Byron marked this pull request as draft March 1, 2022 01:29
They differ as these will be built for the host, so
`FeaturesFor` will not set a target specifically.
@Byron
Copy link
Member Author

Byron commented Mar 1, 2022

Thanks for your help! For a moment I thought it's about jni being a published crate, but that wasn't the case. What matters is that it has a build script, which reproduces the issue once again.

The feature key seems incorrect (being NormalOrDevOrArtifactTarget(None)) and should probably be signaling it is a HostDep instead. Debugging this is typically cumbersome, and I wonder if @ehuss has some tricks to share or any other advice 😅. What I'd really be missing is a visualization of the unit graph.

This is a hacky way of making changes while leaving everything else,
despite it being deactivated for now, in the hopes to not forget
to put additional difficulties back in once this particular issue
is fixed.
@Byron
Copy link
Member Author

Byron commented Mar 1, 2022

I managed to further simplify the example test, which now is just an artifact dependency that itself has a build script. The target.'cfg…'.dependency part isn't needed for reproduction. For now I left all the extras in the example but deactivated them to be able to increase the complexity should I be able to fix the simple version of the issue.

Byron added 4 commits March 1, 2022 11:04
In order not to give up and create a basis for discussion while ending
my 3h oddisey on finding a fix for today, I present something that
seems to work even though I hope there are better ways to solve this.
@Byron
Copy link
Member Author

Byron commented Mar 1, 2022

Alright, I managed to find a 'fix' which really is a cry for help. I hope that @Jasper-Bekkers can validate it on their machine and for the reviewers to guide this PR towards a more proper fix solution which I am unfortunately unable to achieve with my current knowledge level.

What's most difficult for me is the feature resolver being disconnected from the code that creates the unit graph for compilation while still having to provide the answers for all questions asked in a later traversal of the graph. It's hard for me to know for sure whether the one providing answers or the one asking needs adjustments.

@Byron Byron marked this pull request as ready for review March 1, 2022 07:57
@Jasper-Bekkers
Copy link

@Byron It looks like this fix resolves the panic I'm seeing! Thanks :-) Maybe @ehuss can give it a review to help address your concerns?

@phil-opp
Copy link
Contributor

phil-opp commented Mar 1, 2022

I also ran into #10431 and I can confirm that it seems to be fixed with commit 0bede73.

@ehuss
Copy link
Contributor

ehuss commented Mar 3, 2022

Can you update the PR title to be more descriptive? Usually the "fixes #xxxx" should be somewhere in the description, and the title should be a succinct summary of the purpose of the PR (like "Fix panic when …"). The description should have a more long-form description of the purpose of the PR and what it fixes.

Debugging this is typically cumbersome, and I wonder if @ehuss has some tricks to share or any other advice 😅. What I'd really be missing is a visualization of the unit graph.

There are some tips for debugging while writing tests here.

For visualizing the unit graph, there isn't anything built-in. What I normally do is run with --unit-graph, and pretty-format the output (with jq or whatever). I think manually look through that. It's a bit cumbersome because the connections are identified with numbers.

Often I will also just draw out the graph. If it is simple, I just write it down in a scratch buffer. If it is really complex, I'll break out a pencil and paper and draw the graph.

I also recommend temporarily adding copious log messages if you want to trace the execution. There are already log messages which will show the full set of all features, and will also show the entire unit graph (though I think --unit-graph is a little easier to read).


I'm looking through this, and I'm not feeling too comfortable as it is getting quite difficult to follow. I opened #10452 with what I think might be something related to this. Previously, the feature resolver was fairly basic with a simple bool tracking features "for_host" or not. Now, it has multiple states which can transition in various ways as it walks the dependency graph. I'm not feeling confident that all those state transitions are being handled correctly.

When I get a chance, I'll try to continue poking at this.

@Byron Byron changed the title Fix #10431 Fix panic when artifact target is used for [target.'cfg(<target>)'.dependencies (#10431) Mar 5, 2022
@Byron Byron changed the title Fix panic when artifact target is used for [target.'cfg(<target>)'.dependencies (#10431) Fix panic when artifact target is used for [target.'cfg(<target>)'.dependencies (fixes #10431) Mar 5, 2022
@Byron Byron changed the title Fix panic when artifact target is used for [target.'cfg(<target>)'.dependencies (fixes #10431) Fix panic when artifact target is used for [target.'cfg(<target>)'.dependencies Mar 5, 2022
@Byron
Copy link
Member Author

Byron commented Mar 5, 2022

Can you update the PR title to be more descriptive?

Sorry for that, the subject is changed as well as the description to be much more helpful without following links to issues.

Thanks for the summary on how you are debugging, it's very helpful even though it also painfully shows that there is no silver bullet (yet). If there is one takeaway to highlight then it's to use the built-in logging facilities - I never used it but think it's good to get a first idea of what's going on without stepping through the runtime with a debugger.

My intuition for what I am often lacking is a unit-graph visualization. Maybe some day I (or somebody else) will write a transformer that takes the --unit-graph output and turns it into an svg for display in the browser, maybe along with some javascript to help interacting with it.

I'm looking through this, and I'm not feeling too comfortable as it is getting quite difficult to follow.

Likewise :/. Even though it's less of an issue when I think that as I don't consider myself well-versed in the codebase anyway. Sometimes when stepping through with the debugger I thought the only way this is going to make sense to me is to rewrite it from scratch and learn everything that way. By now I am clearly in the territory of using tests to find least-intrusive special case fixes that do the job without actually understanding on how that affects everything else.

When I get a chance, I'll try to continue poking at this.

Thank you!

@ehuss
Copy link
Contributor

ehuss commented Mar 10, 2022

OK, I think the correct approach is something like the following:

diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 932c91a22..a69c2501f 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
@@ -464,10 +464,7 @@ fn compute_deps_custom_build(
     // All dependencies of this unit should use profiles for custom builds.
     // If this is a build script of a proc macro, make sure it uses host
     // features.
-    let script_unit_for = UnitFor::new_host(
-        unit_for.is_for_host_features(),
-        unit_for.root_compile_kind(),
-    );
+    let script_unit_for = unit_for.for_custom_build();
     // When not overridden, then the dependencies to run a build script are:
     //
     // 1. Compiling the build script itself.
@@ -782,7 +779,7 @@ fn dep_build_script(
             // The profile stored in the Unit is the profile for the thing
             // the custom build script is running for.
             let profile = state.profiles.get_profile_run_custom_build(&unit.profile);
-            // UnitFor::new_host is used because we want the `host` flag set
+            // UnitFor::for_custom_build is used because we want the `host` flag set
             // for all of our build dependencies (so they all get
             // build-override profiles), including compiling the build.rs
             // script itself.
@@ -807,10 +804,7 @@ fn dep_build_script(
             // compiled twice. I believe it is not feasible to only build it
             // once because it would break a large number of scripts (they
             // would think they have the wrong set of features enabled).
-            let script_unit_for = UnitFor::new_host(
-                unit_for.is_for_host_features(),
-                unit_for.root_compile_kind(),
-            );
+            let script_unit_for = unit_for.for_custom_build();
             new_unit_dep_with_profile(
                 state,
                 unit,
diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs
index e6415daff..fe326688f 100644
--- a/src/cargo/core/profiles.rs
+++ b/src/cargo/core/profiles.rs
@@ -969,6 +969,18 @@ impl UnitFor {
         }
     }
 
+    pub fn for_custom_build(self) -> UnitFor {
+        UnitFor {
+            host: true,
+            host_features: self.host_features,
+            // Force build scripts to always use `panic=unwind` for now to
+            // maximally share dependencies with procedural macros.
+            panic_setting: PanicSetting::AlwaysUnwind,
+            root_compile_kind: self.root_compile_kind,
+            artifact_target_for_features: self.artifact_target_for_features,
+        }
+    }
+
     /// Set the artifact compile target for use in features using the given `artifact`.
     pub(crate) fn with_artifact_features(mut self, artifact: &Artifact) -> UnitFor {
         self.artifact_target_for_features = artifact.target().and_then(|t| t.to_compile_target());
diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs
index 41e497c03..c62c556b2 100644
--- a/src/cargo/core/resolver/features.rs
+++ b/src/cargo/core/resolver/features.rs
@@ -761,18 +761,22 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
     ) -> Vec<(PackageId, Vec<(&'a Dependency, FeaturesFor)>)> {
         // Helper for determining if a platform is activated.
         let platform_activated = |dep: &Dependency| -> bool {
-            // We always care about build-dependencies, and they are always
-            // Host. If we are computing dependencies "for a build script",
-            // even normal dependencies are host-only.
-            if fk == FeaturesFor::HostDep || dep.is_build() {
-                return self
+            match (dep.is_build(), fk) {
+                (true, _) | (_, FeaturesFor::HostDep) => {
+                    // We always care about build-dependencies, and they are always
+                    // Host. If we are computing dependencies "for a build script",
+                    // even normal dependencies are host-only.
+                    self.target_data
+                        .dep_platform_activated(dep, CompileKind::Host)
+                }
+                (_, FeaturesFor::NormalOrDevOrArtifactTarget(None)) => self
+                    .requested_targets
+                    .iter()
+                    .any(|kind| self.target_data.dep_platform_activated(dep, *kind)),
+                (_, FeaturesFor::NormalOrDevOrArtifactTarget(Some(target))) => self
                     .target_data
-                    .dep_platform_activated(dep, CompileKind::Host);
+                    .dep_platform_activated(dep, CompileKind::Target(target)),
             }
-            // Not a build dependency, and not for a build script, so must be Target.
-            self.requested_targets
-                .iter()
-                .any(|kind| self.target_data.dep_platform_activated(dep, *kind))
         };
         self.resolve
             .deps(pkg_id)

This should fix both #10431 and #10452.

The issue in the feature resolver is because of what is described in #10431 (comment). When traversing the dep graph, when detecting if a dependency is activated, it needs to look at what the current target is, not just what was requested on the command-line.

The other issue is that the FeaturesFor for build scripts was wrong. The feature map was being generated correctly, but the UnitFor::for_host was dropping the artifact_target_for_features value. This is needed for picking up the features to use for the build script. The solution I used here was to add a new method to specifically handle this scenario.

Does that sound reasonable to you?

version = "0.0.1"
authors = []

[target.'cfg(target_arch = "$ARCH")'.dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be something like the following:

Suggested change
[target.'cfg(target_arch = "$ARCH")'.dependencies]
[target.'$TARGET'.dependencies]

On macOS, both the host and alt target arch are x86_64.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, it's applied in 4e847a2 as part of following up on the above comment.

Byron added 3 commits March 14, 2022 10:24
This is the diff from
rust-lang#10433 (comment)
applied to the codebase with the previous hack removed.

Thanks a million to @ehuss who has the insight to find fixes like this.
@Byron
Copy link
Member Author

Byron commented Mar 14, 2022

Thanks a lot for the help, @ehuss .

I have applied the patch as is and removed all prior hacks and could validate that this indeed fixes this issue and #10452 🎉.

Does that sound reasonable to you?

I feel honored for you asking but must admit that weeks after finishing the initial PRs for RFC-3028/3176 I lack the deep insights I'd need to provide any useful feedback here :/. At the very least I seem to retain more permanent knowledge about the codebase to be able to put together PRs with fixes with help here and there. I am sorry to not provide more but my time with cargo is quite limited these days and thus far I wasn't getting back the intuition about the inner workings that I must have had at some point.

@ehuss
Copy link
Contributor

ehuss commented Mar 16, 2022

Thanks, and no worries!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2022

📌 Commit 0b53b1b has been approved by ehuss

@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 Mar 16, 2022
@bors
Copy link
Contributor

bors commented Mar 16, 2022

⌛ Testing commit 0b53b1b with merge 2ac382d...

@bors
Copy link
Contributor

bors commented Mar 16, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 2ac382d to master...

@bors bors merged commit 2ac382d into rust-lang:master Mar 16, 2022
@bors bors mentioned this pull request Mar 16, 2022
18 tasks
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2022
Update cargo

9 commits in 65c82664263feddc5fe2d424be0993c28d46377a..109bfbd055325ef87a6e7f63d67da7e838f8300b
2022-03-09 02:32:56 +0000 to 2022-03-17 21:43:09 +0000
- Refactor RegistryData::load to handle management of the index cache (rust-lang/cargo#10482)
- Separate VCS command paths with "--" (rust-lang/cargo#10483)
- Fix panic when artifact target is used for `[target.'cfg(&lt;target&gt;)'.dependencies` (rust-lang/cargo#10433)
- Bump [email protected] and [email protected] (rust-lang/cargo#10479)
- vendor: Don't allow multiple values for --sync (rust-lang/cargo#10448)
- Use types to make clere (credential process || token) (rust-lang/cargo#10471)
- Warning on conflicting keys (rust-lang/cargo#10316)
- Registry functions return Poll to enable parallel fetching of index data (rust-lang/cargo#10064)
- Refine the contributor guide (rust-lang/cargo#10468)
@ehuss ehuss added this to the 1.61.0 milestone Apr 7, 2022
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
Development

Successfully merging this pull request may close these issues.

-Zbindeps panics with activated_features for invalid package: features did not find PackageId
6 participants