-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 running cargo tree
on a package with a cross compiled bindep
#13207
base: master
Are you sure you want to change the base?
Conversation
r? @epage (rustbot has picked a reviewer for you, use r? to override) |
probably makes sense for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the way too long delay of the review 😞.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-dependencies
are built for the host, whiledependencies
anddev-dependencies
are built for the target. You can specify thetarget
attribute 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--target
option for an unavailable target.)
8ab6462
to
eaa50a9
Compare
eaa50a9
to
77435e0
Compare
Add failing test: artifact_dep_target_specified This PR pulls the failing test out of #13207 [During review](#13207 (comment)), it was requested there be a previous commit with a failing test. It will be a while before I have the time to address the other issues with the PR, so I figured for now we should land this failing test first.
☔ The latest upstream changes (presumably #13816) made this pull request unmergeable. Please resolve the merge conflicts. |
@rukai are you still interested in fixing this? |
I cant afford the time to work on this any time soon. |
What does this PR try to resolve?
This PR fixes the
cargo tree
panic 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 tree
is arrived at.However I think it makes sense to land this PR first as:
cargo tree
in many more bindeps use cases.So this PR is a good first step towards full support in
cargo tree
.The changes to the
activated_features
methods are not related to the fix but just the improved error reporting I needed to fully diagnose the issue.I moved
activated_features_int
intoactivated_features
andactivated_features_unverified
as 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.