Skip to content

Fix panic when running cargo tree on a package with a cross compiled bindep #13207

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+89 −30
Closed

Fix panic when running cargo tree on a package with a cross compiled bindep #13207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 26 additions & 26 deletions src/cargo/core/resolver/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

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.

let key = (pkg_id, fk);
if let Some(fs) = self.activated_features.get(&key) {
fs.iter().cloned().collect()
} else {
panic!(
"did not find features for {key:?} within activated_features:\n{:#?}",
self.activated_features
)
}
}

/// Variant of `activated_features` that returns `None` if this is
/// not a valid pkg_id/is_build combination. Used in places which do
/// not know which packages are activated (like `cargo clean`).
pub fn activated_features_unverified(
&self,
pkg_id: PackageId,
features_for: FeaturesFor,
) -> Option<Vec<InternedString>> {
let fk = features_for.apply_opts(&self.opts);
if let Some(fs) = self.activated_features.get(&(pkg_id, fk)) {
Some(fs.iter().cloned().collect())
} else {
None
}
}

/// Returns if the given dependency should be included.
Expand All @@ -340,30 +364,6 @@ impl ResolvedFeatures {
.unwrap_or(false)
}

/// Variant of `activated_features` that returns `None` if this is
/// not a valid pkg_id/is_build combination. Used in places which do
/// not know which packages are activated (like `cargo clean`).
pub fn activated_features_unverified(
&self,
pkg_id: PackageId,
features_for: FeaturesFor,
) -> Option<Vec<InternedString>> {
self.activated_features_int(pkg_id, features_for).ok()
}

fn activated_features_int(
&self,
pkg_id: PackageId,
features_for: FeaturesFor,
) -> CargoResult<Vec<InternedString>> {
let fk = features_for.apply_opts(&self.opts);
if let Some(fs) = self.activated_features.get(&(pkg_id, fk)) {
Ok(fs.iter().cloned().collect())
} else {
bail!("features did not find {:?} {:?}", pkg_id, fk)
}
}

/// Compares the result against the original resolver behavior.
///
/// Used by `cargo fix --edition` to display any differences.
Expand Down
18 changes: 14 additions & 4 deletions src/cargo/ops/tree/graph.rs
View file
Open in desktop
Copy link
Member

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, while dependencies and dev-dependencies are built for the target. You can specify the target attribute to build for a specific target, such as target = "wasm32-wasi"; a literal target = "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.)

Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,20 @@ 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
.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),
Copy link
Member

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.

Copy link
Member

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 😅.

None => {
if dep.is_build() || dep_pkg.proc_macro() {
FeaturesFor::HostDep
} else {
features_for
}
}
};
let dep_index = add_pkg(
graph,
Expand Down
49 changes: 49 additions & 0 deletions tests/testsuite/artifact_dep.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,55 @@ foo v0.0.0 ([CWD])
)
.run();
}

#[cargo_test]
fn artifact_dep_target_specified() {
Copy link
Member

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.

if cross_compile::disabled() {
return;
}
let target = cross_compile::alternate();

let p = project()
.file(
"Cargo.toml",
&r#"
[package]
name = "foo"
version = "0.0.0"
authors = []
resolver = "2"

[dependencies]
bindep = { path = "bindep", artifact = "bin", target = "$TARGET" }
"#
.replace("$TARGET", target),
)
.file("src/lib.rs", "")
.file("bindep/Cargo.toml", &basic_manifest("bindep", "0.0.0"))
.file("bindep/src/main.rs", "fn main() {}")
.build();

p.cargo("check -Z bindeps")
.masquerade_as_nightly_cargo(&["bindeps"])
.with_stderr_contains(
r#"[COMPILING] bindep v0.0.0 ([CWD]/bindep)
[CHECKING] foo v0.0.0 ([CWD])
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]"#,
)
.with_status(0)
.run();

p.cargo("tree -Z bindeps")
.masquerade_as_nightly_cargo(&["bindeps"])
.with_stdout(
"\
foo v0.0.0 ([CWD])
└── bindep v0.0.0 ([CWD]/bindep)",
)
.with_status(0)
.run();
}

#[cargo_test]
fn targets_are_picked_up_from_non_workspace_artifact_deps() {
if cross_compile::disabled() {
Expand Down