Fix panic when running cargo tree on a package with a cross compiled bindep
#13207
Conversation
|
r? @epage (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-dependency-resolution
|
probably makes sense for |
| @@ -319,8 +319,32 @@ impl ResolvedFeatures { | |||
| pkg_id: PackageId, | |||
| features_for: FeaturesFor, | |||
| ) -> Vec<InternedString> { | |||
| self.activated_features_int(pkg_id, features_for) | |||
| .expect("activated_features for invalid package") | |||
| let fk = features_for.apply_opts(&self.opts); | |||
There was a problem hiding this comment.
Would like to see this refactor being in its own commit, to following atomic commit principle.
| .and_then(|target| target.to_resolved_compile_target(requested_kind)) | ||
| { | ||
| // Dependency has a `{ …, target = <triple> }` | ||
| Some(target) => FeaturesFor::ArtifactDep(target), |
There was a problem hiding this comment.
I might have a rotted memory of artifact dependencies. However, even when target is present the dependency might still be depended as a normal lib (with lib = true). The fix doesn't look 100% correct to me for this reason.
There was a problem hiding this comment.
Okay this looks like an adaption of my own patch... Need to refresh my memory 😅.
| @@ -1445,6 +1445,55 @@ foo v0.0.0 ([CWD]) | |||
| ) | |||
| .run(); | |||
| } | |||
|
|
|||
| #[cargo_test] | |||
| fn artifact_dep_target_specified() { | |||
There was a problem hiding this comment.
This test passes even without the patch in ops::tree::graph. We should probably first create a test verifying the current panic behavior in a commit, followed by the other commit fixing both the behavior and the test.
There was a problem hiding this comment.
I did however leave out the
None if features_for != FeaturesFor::default() => features_for, because that seems wrong to me. proc macro and build dependencies of a bindep need to be compiled for the host, regardless of the bindeps target, otherwise the host cant run them to build the bindep.
See https://rust-lang.github.io/rfcs/3028-cargo-binary-dependencies.html#reference-level-explanation, so arguably some build deps could be built for a target platform.
By default,
build-dependenciesare built for the host, whiledependenciesanddev-dependenciesare built for the target. You can specify thetargetattribute to build for a specific target, such astarget = "wasm32-wasi"; a literaltarget = "target"will build for the target even if specifying a build dependency. (If the target is not available, this will result in an error at build time, just as if building the specified crate with a--targetoption for an unavailable target.)
weihanglo
added
S-waiting-on-author
What does this PR try to resolve?
This PR fixes the
cargo treepanic described in #12358 and #10593How should we test and review this PR?
The new integration test is sufficient.
Additional information
There was discussion of holding off on this until a full design of how to present bindeps in
cargo treeis arrived at.However I think it makes sense to land this PR first as:
cargo treein many more bindeps use cases.So this PR is a good first step towards full support in
cargo tree.The changes to the
activated_featuresmethods are not related to the fix but just the improved error reporting I needed to fully diagnose the issue.I moved
activated_features_intintoactivated_featuresandactivated_features_unverifiedas I was concerned of the performance cost of generating the full error when its not a fatal error and may occur many times.I think this change will bring value in the future but I am happy to remove it if it is undesired.
I actually wrote up the test + fix independently before discovering #10593 (comment) , once I discovered it I copied in some specific parts to improve comments + correctness
I did however leave out the
None if features_for != FeaturesFor::default() => features_for,because that seems wrong to me. proc macro and build dependencies of a bindep need to be compiled for the host, regardless of the bindeps target, otherwise the host cant run them to build the bindep.