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 running cargo tree on a package with a cross compiled bindep #14593

Merged
merged 2 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions src/cargo/ops/tree/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,32 @@ fn add_pkg(
let dep_pkg = graph.package_map[&dep_id];

for dep in deps {
let dep_features_for = if dep.is_build() || dep_pkg.proc_macro() {
FeaturesFor::HostDep
} else {
features_for
let dep_features_for = match dep
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reviving this PR!

However, this is missing an important part of my original patch. Please see my previous comment :)

I am tempted to merge this PR as it fixes some panics. I will be more existed if we can implement the design proposed here: #10593 (comment).

Copy link
Contributor Author

@elchukc elchukc Oct 8, 2024

Choose a reason for hiding this comment

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

I'm very happy to keep implementing the design! (I'm now emotionally invested), but I think we should split it into separate PRs. I've updated the PR message to explain each commit as they currently exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: Moved the proposed design change work to #14658 to keep this PR clear.

.artifact()
.and_then(|artifact| artifact.target())
.and_then(|target| target.to_resolved_compile_target(requested_kind))
{
// Dependency has a `{ …, target = <triple> }`
Some(target) => FeaturesFor::ArtifactDep(target),
// Get the information of the dependent crate from `features_for`.
// If a dependent crate is
//
// * specified as an artifact dep with a `target`, or
// * a host dep,
//
// its transitive deps, including build-deps, need to be built on that target.
None if features_for != FeaturesFor::default() => features_for,
// Dependent crate is a normal dep, then back to old rules:
//
// * normal deps, dev-deps -> inherited target
// * build-deps -> host
None => {
if dep.is_build() || dep_pkg.proc_macro() {
FeaturesFor::HostDep
} else {
features_for
}
}
};
let dep_index = add_pkg(
graph,
Expand Down
89 changes: 84 additions & 5 deletions tests/testsuite/artifact_dep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1568,14 +1568,93 @@ fn artifact_dep_target_specified() {
.with_status(0)
.run();

// TODO: This command currently fails due to a bug in cargo but it should be fixed so that it succeeds in the future.
p.cargo("tree -Z bindeps")
.masquerade_as_nightly_cargo(&["bindeps"])
.with_stdout_data("")
.with_stderr_data(r#"...
[..]did not find features for (PackageId { name: "bindep", version: "0.0.0", source: "[..]" }, NormalOrDev) within activated_features:[..]
.with_stdout_data(str![[r#"
foo v0.0.0 ([ROOT]/foo)
└── bindep v0.0.0 ([ROOT]/foo/bindep)

"#]])
.with_status(0)
.run();
}

/// From issue #10593
/// The case where:
/// * artifact dep is { target = <specified> }
/// * dependency of that artifact dependency specifies the same target
/// * the target is not activated.
#[cargo_test]
fn dep_of_artifact_dep_same_target_specified() {
if cross_compile::disabled() {
return;
}
let target = cross_compile::alternate();
let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.1.0"
edition = "2015"
resolver = "2"

[dependencies]
bar = {{ path = "bar", artifact = "bin", target = "{target}" }}
"#,
),
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
&format!(
r#"
[package]
name = "bar"
version = "0.1.0"

[target.{target}.dependencies]
baz = {{ path = "../baz" }}
"#,
),
)
.file("bar/src/main.rs", "fn main() {}")
.file(
"baz/Cargo.toml",
r#"
[package]
name = "baz"
version = "0.1.0"

"#,
)
.file("baz/src/lib.rs", "")
.build();

p.cargo("check -Z bindeps")
.masquerade_as_nightly_cargo(&["bindeps"])
.with_stderr_data(str![[r#"
[LOCKING] 2 packages to latest compatible versions
[COMPILING] baz v0.1.0 ([ROOT]/foo/baz)
[COMPILING] bar v0.1.0 ([ROOT]/foo/bar)
[CHECKING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.with_status(0)
.run();

// TODO This command currently fails due to a bug in cargo but it should be fixed so that it succeeds in the future.
p.cargo("tree -Z bindeps")
.masquerade_as_nightly_cargo(&["bindeps"])
.with_stderr_data(
r#"...
no entry found for key
...
"#)
"#,
)
.with_status(101)
.run();
}
Expand Down
Loading