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

Conversation

elchukc
Copy link
Contributor

@elchukc elchukc commented Sep 24, 2024

What does this PR try to resolve?

This is an attempt to close out @rukai's PR 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.

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 - fixes some panics and changes the integration test to succeed
  2. test: cargo tree panic on artifact dep target deactivated - adds test to confirm the behaviour for the specific panic from #10539 (comment)

@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot 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 Sep 24, 2024
@elchukc elchukc changed the title Fix cargo tree bindep crosscompile WIP Fix cargo tree bindep crosscompile Sep 25, 2024
@elchukc elchukc force-pushed the fix_cargo_tree_bindep_crosscompile branch 2 times, most recently from aa14297 to 7ae6261 Compare September 25, 2024 13:43
@elchukc elchukc marked this pull request as ready for review September 25, 2024 13:57
@elchukc elchukc changed the title WIP Fix cargo tree bindep crosscompile Fix cargo tree bindep crosscompile Sep 25, 2024
@elchukc elchukc changed the title Fix cargo tree bindep crosscompile Fix panic when running cargo tree on a package with a cross compiled bindep Sep 25, 2024
@elchukc
Copy link
Contributor Author

elchukc commented Sep 25, 2024

For continuity, probably
r? weihanglo

Also note I have not re-added None if features_for != FeaturesFor::default() => features_for but would be open to creating another test to catch that case.

@bors
Copy link
Collaborator

bors commented Sep 30, 2024

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

@elchukc elchukc force-pushed the fix_cargo_tree_bindep_crosscompile branch from 7ae6261 to 3a7edc2 Compare September 30, 2024 20:58
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.

src/cargo/core/resolver/features.rs Outdated Show resolved Hide resolved
bors added a commit that referenced this pull request Oct 5, 2024
…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
@bors
Copy link
Collaborator

bors commented Oct 5, 2024

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

@elchukc elchukc marked this pull request as draft October 5, 2024 23:55
@elchukc elchukc force-pushed the fix_cargo_tree_bindep_crosscompile branch 3 times, most recently from 3d33aa1 to 39ce7c1 Compare October 8, 2024 22:19
@elchukc elchukc marked this pull request as ready for review October 8, 2024 22:52
@elchukc elchukc force-pushed the fix_cargo_tree_bindep_crosscompile branch from 39ce7c1 to d379632 Compare October 9, 2024 01:03
@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-dep-info Area: dep-info, .d files labels Oct 9, 2024
@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-rebuild-detection Area: rebuild detection and fingerprinting A-testing-cargo-itself Area: cargo's tests A-unstable Area: nightly unstable support labels Oct 9, 2024
@elchukc elchukc force-pushed the fix_cargo_tree_bindep_crosscompile branch from d379632 to 4646712 Compare October 9, 2024 01:03
tests/testsuite/artifact_dep.rs Outdated Show resolved Hide resolved
"#,
),
)
.file("bar/src/lib.rs", "")
Copy link
Member

Choose a reason for hiding this comment

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

Should we put a bin here otherwise cargo check always fails?

Suggested change
.file("bar/src/lib.rs", "")
.file("bar/src/main.rs", "")

@elchukc elchukc force-pushed the fix_cargo_tree_bindep_crosscompile branch from 4646712 to ed294ab Compare October 11, 2024 02:22
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks!

While it doesn't really resolve all panics, at least we are one step forward :)

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 11, 2024

📌 Commit ed294ab has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2024
@bors
Copy link
Collaborator

bors commented Oct 11, 2024

⌛ Testing commit ed294ab with merge 1e5bad3...

@bors
Copy link
Collaborator

bors commented Oct 11, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 1e5bad3 to master...

@bors bors merged commit 1e5bad3 into rust-lang:master Oct 11, 2024
22 checks passed
@elchukc elchukc deleted the fix_cargo_tree_bindep_crosscompile branch October 11, 2024 15:29
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2024
Update cargo

14 commits in 15fbd2f607d4defc87053b8b76bf5038f2483cf4..8c30ce53688e25f7e9d860b33cc914fb2957ca9a
2024-10-08 21:08:11 +0000 to 2024-10-15 16:43:16 +0000
- docs: More information on what is and isn't included by cargo package (rust-lang/cargo#14684)
- fix(resolver): share conflict cache between activation retries (rust-lang/cargo#14692)
- fix(git): dont fetch tags by default (rust-lang/cargo#14688)
- Support package selection options like `--exclude` in `cargo publish` (rust-lang/cargo#14659)
- docs: install options -> uninstall options (rust-lang/cargo#14682)
- docs: tools should only interpret a line starting with `{` as JSON (rust-lang/cargo#14677)
- cargo test --help: clarify --tests and --benches (rust-lang/cargo#14675)
- docs(env): minor improvements in environment variables doc (rust-lang/cargo#14676)
- docs: document official external commands (rust-lang/cargo#14669)
- Fix panic when running cargo tree on a package with a cross compiled bindep (rust-lang/cargo#14593)
- Remove the support for `Cargo.toml` of the cargo-script (rust-lang/cargo#14670)
- docs(resolver): Lay groundwork for documenting MSRV-aware logic (rust-lang/cargo#14662)
- chore(deps): update rust crate pulldown-cmark to 0.12.0 (rust-lang/cargo#14668)
- Improve resolver speed (rust-lang/cargo#14663)
@rustbot rustbot added this to the 1.84.0 milestone Oct 16, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 17, 2024
Update cargo

14 commits in 15fbd2f607d4defc87053b8b76bf5038f2483cf4..8c30ce53688e25f7e9d860b33cc914fb2957ca9a
2024-10-08 21:08:11 +0000 to 2024-10-15 16:43:16 +0000
- docs: More information on what is and isn't included by cargo package (rust-lang/cargo#14684)
- fix(resolver): share conflict cache between activation retries (rust-lang/cargo#14692)
- fix(git): dont fetch tags by default (rust-lang/cargo#14688)
- Support package selection options like `--exclude` in `cargo publish` (rust-lang/cargo#14659)
- docs: install options -> uninstall options (rust-lang/cargo#14682)
- docs: tools should only interpret a line starting with `{` as JSON (rust-lang/cargo#14677)
- cargo test --help: clarify --tests and --benches (rust-lang/cargo#14675)
- docs(env): minor improvements in environment variables doc (rust-lang/cargo#14676)
- docs: document official external commands (rust-lang/cargo#14669)
- Fix panic when running cargo tree on a package with a cross compiled bindep (rust-lang/cargo#14593)
- Remove the support for `Cargo.toml` of the cargo-script (rust-lang/cargo#14670)
- docs(resolver): Lay groundwork for documenting MSRV-aware logic (rust-lang/cargo#14662)
- chore(deps): update rust crate pulldown-cmark to 0.12.0 (rust-lang/cargo#14668)
- Improve resolver speed (rust-lang/cargo#14663)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-dep-info Area: dep-info, .d files A-dependency-resolution Area: dependency resolution and the resolver A-documenting-cargo-itself Area: Cargo's documentation A-features2 Area: issues specifically related to the v2 feature resolver A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-rebuild-detection Area: rebuild detection and fingerprinting A-testing-cargo-itself Area: cargo's tests A-unstable Area: nightly unstable support Command-tree S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants