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

If the only path is a loop then counted as the shortest path. #12977

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Nov 14, 2023

This is a fix for #12941

This graph data structure is used to store dependency DAGs. Where each edge represents a dependency from a package to the package that fulfilled the dependency. Different parts of the resolver store this data in opposite directions, sometimes packages point at the things that depend on them other times packages point to the parents that required them. Error messages often need to report on why a package is in the graph, either by walking up toward parents or down toward children depending on how this graph is stored. #12678 unified the two different walking implementations, and replace them with a breadth first search so as to find the shortest path. This code ignored when edge pointed at a package that had already been reached, because that generally describes a longer path to an existing package.

Unfortunately, when I said this was a DAG that was a simplification. There can be cycles introduced as dev-dependencies. The existing code would reasonably ignore the cycles figuring that if we continue searching we would eventually find the root package (a package that nothing depended on). Missing the possibility that the root package created the cycle.

Now we search through the entire graph looking for a root package. If we do not find a root package we report the path to the last package we processed.

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2023

r? @weihanglo

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2023
@weihanglo
Copy link
Member

Thank you. It is pretty straightforward, but could we have a bit more description so that won't lose track of the patch in the future?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 14, 2023

I can do either... Are you asking me to better comment the code or improve the PR description?

@weihanglo
Copy link
Member

PR description or commit message, which will show in git log so that people don't need to click the issue link and open a browser. It's pretty minor and not a blocker :)

@Eh2406
Copy link
Contributor Author

Eh2406 commented Nov 15, 2023

Edited the PR description. let me know if that's better, or is there somewhere else you want it.

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!

@epage
Copy link
Contributor

epage commented Nov 15, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 15, 2023

📌 Commit d19273c has been approved by epage

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 Nov 15, 2023
@bors
Copy link
Collaborator

bors commented Nov 15, 2023

⌛ Testing commit d19273c with merge 6658e1a...

@bors
Copy link
Collaborator

bors commented Nov 15, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 6658e1a to master...

@bors bors merged commit 6658e1a into rust-lang:master Nov 15, 2023
20 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2023
Update cargo

19 commits in 6790a5127895debec95c24aefaeb18e059270df3..2c03e0e2dcd05dd064fcf10cc1050d342eaf67e3
2023-11-10 17:09:35 +0000 to 2023-11-16 04:21:44 +0000
- docs(ref): Find a place to comment on --cap-lints (rust-lang/cargo#12976)
- Switch from AtomicU64 to Mutex. (rust-lang/cargo#12981)
- If the only path is a loop then counted as the shortest path. (rust-lang/cargo#12977)
- fix(resolver): Prefer MSRV, rather than ignore incompatible (rust-lang/cargo#12950)
- fix error message for duplicate links (rust-lang/cargo#12973)
- Only filter out target if its in the package root (rust-lang/cargo#12944)
- Ignore changing_spec_relearns_crate_types on windows-gnu (rust-lang/cargo#12972)
- fix: do not panic when failed to parse rustc commit-hash (rust-lang/cargo#12965)
- query{_vec} use IndexSummary (rust-lang/cargo#12970)
- Bump to 0.77.0; update changelog (rust-lang/cargo#12966)
- Improve about information of `cargo search` (rust-lang/cargo#12962)
- Fix --quiet being used with nested subcommands. (rust-lang/cargo#12959)
- make some debug assertion failures more informative (rust-lang/cargo#12963)
- refactor(toml): Consistently lead with 'Toml' prefix (rust-lang/cargo#12960)
- refactor(toml): Remove unused method (rust-lang/cargo#12961)
- Fix non-deterministic behavior in last-use repopulation (rust-lang/cargo#12958)
- Add cache garbage collection (rust-lang/cargo#12634)
- refactor(toml): Improve consistency (rust-lang/cargo#12954)
- Fix typo (rust-lang/cargo#12956)
@Eh2406 Eh2406 deleted the bug_12941 branch November 16, 2023 19:56
@ehuss ehuss added this to the 1.76.0 milestone Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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