Fix panic when running `cargo tree` on a package with a cross compiled bindep by rukai · Pull Request #13207 · rust-lang/cargo

rukai · 2023-12-26T04:36:05Z

What does this PR try to resolve?

This PR fixes the cargo tree panic described in #12358 and #10593

How 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:

It introduces a test to catch the simple case.
The fix itself is small and (afaik) correct.
Provides genuine value to users by allowing usage of 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 into activated_features and activated_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.

rustbot · 2023-12-26T04:36:11Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

rukai · 2023-12-26T04:37:02Z

probably makes sense for
r? weihanglo

weihanglo

Sorry for the way too long delay of the review 😞.

weihanglo · 2024-03-28T18:32:29Z

src/cargo/core/resolver/features.rs

    ) -> Vec<InternedString> {
-        self.activated_features_int(pkg_id, features_for)
-            .expect("activated_features for invalid package")
+        let fk = features_for.apply_opts(&self.opts);


Would like to see this refactor being in its own commit, to following atomic commit principle.

weihanglo · 2024-03-28T18:39:37Z

src/cargo/ops/tree/graph.rs

+                .and_then(|target| target.to_resolved_compile_target(requested_kind))
+            {
+                // Dependency has a `{ …, target = <triple> }`
+                Some(target) => FeaturesFor::ArtifactDep(target),


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.

Okay this looks like an adaption of my own patch... Need to refresh my memory 😅.

weihanglo · 2024-03-28T18:41:15Z

tests/testsuite/artifact_dep.rs

 }
+
+#[cargo_test]
+fn artifact_dep_target_specified() {


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.

weihanglo · 2024-03-28T19:07:19Z

src/cargo/ops/tree/graph.rs

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

…d bindep

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.

bors · 2024-04-28T13:59:55Z

☔ The latest upstream changes (presumably #13816) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2024-06-17T14:28:43Z

@rukai are you still interested in fixing this?

rukai · 2024-06-17T22:46:08Z

I cant afford the time to work on this any time soon.
Anyone else is free to take this on.

…hanglo improve error reporting when feature not found in `activated_features` Pulls the error message refactor out of #14593 (originally #13207) to improve error reporting when we fail to get the list of activated features enabled for the given package. It now fully lists the activated_features hashmap keys too. From the [original author](#13207 (comment)): > I moved `activated_features_int` into `activated_features` and `activated_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. Old vs new error message: ```diff - activated_features for invalid package: features did not find PackageId { name: "bindep", version: "0.0.0", source: "[ROOT]/foo/bindep" } NormalOrDev + did not find features for (PackageId { name: "bindep", version: "0.0.0", source: "[ROOT]/foo/bindep" }, NormalOrDev) within activated_features: + [ + ( + PackageId { + name: "bindep", + version: "0.0.0", + source: "[ROOT]/foo/bindep", + }, + ArtifactDep( + CompileTarget { + name: "[ALT_TARGET]", + }, + ), + ), + ( + PackageId { + name: "foo", + version: "0.0.0", + source: "[ROOT]/foo", + }, + NormalOrDev, + ), + ] ``` r? weihanglo

…weihanglo Fix panic when running cargo tree on a package with a cross compiled bindep ### What does this PR try to resolve? This is an attempt to close out `@rukai's` [PR](#13207 (comment)) for #12358 and #10593 by adjusting the new integration test and resolving merge conflicts. I have also separated the changes into atomic commits as per [previous review](#13207 (comment)). ### How should we test and review this PR? The integration test that has been edited here is sufficient, plus the new integration test that confirms a more specific case where `cargo tree` throws an error. ### Additional information I have confirmed the test `artifact_dep_target_specified` fails on master branch and succeeds on this branch. The first commit fixes the panic and the integration test. Commits 2 and 3 add other tests that confirm behaviour mentioned in related issues. Commits: 1. [Fix panic when running cargo tree on a package with a cross compiled bindep](5c5ea78) - fixes some panics and changes the integration test to succeed 2. [test: cargo tree panic on artifact dep target deactivated](ed294ab) - adds test to confirm the behaviour for the specific panic from [#10539 (comment)](#10593 (comment))

elchukc · 2024-11-02T04:11:37Z

We can probably close this

weihanglo · 2024-11-02T04:26:46Z

Yes absolutely. Thank you!

rustbot assigned epage Dec 26, 2023

rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-features2 Area: issues specifically related to the v2 feature resolver Command-tree S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 26, 2023

rustbot assigned weihanglo and unassigned epage Dec 26, 2023

phil-opp mentioned this pull request Mar 4, 2024

Edition 3 post-2 build-dependencies problem phil-opp/blog_os#1303

Closed

weihanglo reviewed Mar 28, 2024

View reviewed changes

weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2024

rukai force-pushed the fix_cargo_tree_bindep_crosscompile branch from 8ab6462 to eaa50a9 Compare April 28, 2024 07:27

rukai mentioned this pull request Apr 28, 2024

Add failing test: artifact_dep_target_specified #13816

Merged

Fix panic when running cargo tree on a package with a cross compile…

77435e0

…d bindep

rukai force-pushed the fix_cargo_tree_bindep_crosscompile branch from eaa50a9 to 77435e0 Compare April 28, 2024 07:55

elchukc mentioned this pull request Sep 25, 2024

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

Merged

elchukc mentioned this pull request Oct 5, 2024

improve error reporting when feature not found in activated_features #14647

Merged

weihanglo closed this Nov 2, 2024

Conversation

rukai commented Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Dec 26, 2023

Uh oh!

rukai commented Dec 26, 2023

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

weihanglo Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

weihanglo Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

weihanglo Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

weihanglo Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

weihanglo Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

bors commented Apr 28, 2024

Uh oh!

weihanglo commented Jun 17, 2024

Uh oh!

rukai commented Jun 17, 2024

Uh oh!

elchukc commented Nov 2, 2024

Uh oh!

weihanglo commented Nov 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rukai commented Dec 26, 2023 •

edited

Loading