fix(resolver): Make resolver behavior independent of package order #12602
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This addresses the ordering issue of for one of the problems from rust-lang#12599. The intent behind the `path_pkg` check is to make sure we update workspace members in the lockfile if their version number changed. In this case, we don't need to recursively walk, because the change doesn't affect dependencies. However, we also shouldn't *prevent* recursive walks which is what we are doing today, both for packages marked to keep and for packages that have been "poisoned". So we fix this by moving that call after all recursive adds are complete so as to not interfere with them.
|
r? @Eh2406 |
rustbot
added
the
S-waiting-on-review
epage
changed the title
|
It's always a little disturbing when you can remove a line of code and the tests still pass. Especially when it's documented to fix a known bug. It is in the pile of hacks for lockfiles. I will need to load this code back into my had before I can be shore this is good. |
Except this doesn't remove any code, it just ones style of processing before the other. It does concern me a little bit but less than trying to fix the other issue in #12599. In that one, it is removal of a line or two and that does cause tests to fail. |
epage
force-pushed
the
resolver
branch
2 times, most recently
from
d9f1e05 to
874b4dd
Compare
|
@rfcbot ready |
epage
force-pushed
the
resolver
branch
3 times, most recently
from
bd19a68 to
0af2f65
Compare
|
🤞 @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 6, 2023
Update cargo 21 commits in 96fe1c9e1aecd8f57063e3753969bb6418fd2fd5..d14c85f4e6e7671673b1a1bc87231ff7164761e1 2023-08-29 20:10:34 +0000 to 2023-09-05 22:28:10 +0000 - fix(resolver): Make resolver behavior independent of package order (rust-lang/cargo#12602) - cargo-credential: change serialization of cache expiration (rust-lang/cargo#12622) - Update registry-web-api.md yank/unyank comments (rust-lang/cargo#12619) - test: new options of debuginfo are no longer unstable (rust-lang/cargo#12618) - use split_once for cleaner code (rust-lang/cargo#12615) - stop using lazy_static (rust-lang/cargo#12616) - doc: adjust all doc headings one level up (rust-lang/cargo#12595) - chore(deps): update compatible (rust-lang/cargo#12609) - chore(deps): update rust crate cargo_metadata to 0.17.0 (rust-lang/cargo#12610) - Prepare for partial-version package specs (rust-lang/cargo#12591) - refactor: Use more serde_untagged (rust-lang/cargo#12581) - fix(cli): Help users know possible `--target` values (rust-lang/cargo#12607) - Tab completion for --target uses rustup but fallsback to rustc (rust-lang/cargo#12606) - Fewer temporary needless strings (rust-lang/cargo#12604) - fix(help): Provide better commands heading for styling (rust-lang/cargo#12593) - fix(update): Clarify meaning of --aggressive as --recursive (rust-lang/cargo#12544) - docs(changelog): Clarify language for Cargo.lock policy (rust-lang/cargo#12601) - fix typo: "default branch branch" -> "default branch" (rust-lang/cargo#12598) - fix: add error for unsupported credential provider version (rust-lang/cargo#12590) - fix(help): Explain --explain (rust-lang/cargo#12592) - fix(help): Remove redundant information from new/init (rust-lang/cargo#12594) r? ghost
epage
added a commit
to epage/cargo
that referenced
this pull request
Nov 14, 2023
Before, when running `cargo update <member>`, we'd not reuse the previous resolve result and when the resolver started walking into the dependencies, it would do a git fetch. Now, we won't even try to resolve the workspace members and so we won't look at those dependencies and do git fetch. This will make `cargo update <workspace-member>` match `cargo update --workspace`. I considered whether there were other ways of handling this but I figured aiming for consistency in approaches was the best way. We can investigate improving those approaches separately. There are other discrepancies in the different code paths (handling of patches, adding sources) but I'm deferring looking over those. Between this and rust-lang#12602, this should finnally resolve rust-lang#12599. Fixes rust-lang#12599
epage
added a commit
to epage/cargo
that referenced
this pull request
Nov 14, 2023
Before, when running `cargo update <member>`, we'd not reuse the previous resolve result and when the resolver started walking into the dependencies, it would do a git fetch. Now, we won't even try to resolve the workspace members and so we won't look at those dependencies and do git fetch. This will make `cargo update <workspace-member>` match `cargo update --workspace`. I considered whether there were other ways of handling this but I figured aiming for consistency in approaches was the best way. We can investigate improving those approaches separately. There are other discrepancies in the different code paths (handling of patches, adding sources) but I'm deferring looking over those. Between this and rust-lang#12602, this should finnally resolve rust-lang#12599. Fixes rust-lang#12599
epage
added a commit
to epage/cargo
that referenced
this pull request
Nov 17, 2023
Before, when running `cargo update <member>`, we'd not reuse the previous resolve result and when the resolver started walking into the dependencies, it would do a git fetch. Now, we won't even try to resolve the workspace members and so we won't look at those dependencies and do git fetch. This will make `cargo update <workspace-member>` match `cargo update --workspace`. I considered whether there were other ways of handling this but I figured aiming for consistency in approaches was the best way. We can investigate improving those approaches separately. There are other discrepancies in the different code paths (handling of patches, adding sources) but I'm deferring looking over those. Between this and rust-lang#12602, this should finnally resolve rust-lang#12599. Fixes rust-lang#12599
epage
added a commit
to epage/cargo
that referenced
this pull request
Nov 17, 2023
Before, when running `cargo update <member>`, we'd not reuse the previous resolve result and when the resolver started walking into the dependencies, it would do a git fetch. Now, we won't even try to resolve the workspace members and so we won't look at those dependencies and do git fetch. This will make `cargo update <workspace-member>` match `cargo update --workspace`. I considered whether there were other ways of handling this but I figured aiming for consistency in approaches was the best way. We can investigate improving those approaches separately. There are other discrepancies in the different code paths (handling of patches, adding sources) but I'm deferring looking over those. Between this and rust-lang#12602, this should finnally resolve rust-lang#12599. Fixes rust-lang#12599
bors
added a commit
that referenced
this pull request
Nov 18, 2023
fix(resolver): Don't do git fetches when updating workspace members ### What does this PR try to resolve? Before, when running `cargo update <member>`, we'd not reuse the previous resolve result and when the resolver started walking into the dependencies, it would do a git fetch. Now, we won't even try to resolve the workspace members and so we won't look at those dependencies and do git fetch. This will make `cargo update <workspace-member>` match `cargo update --workspace`. Fixes #12599 Fixes #8821 ### How should we test and review this PR? I considered whether there were other ways of handling this but I figured aiming for consistency in approaches was the best way. We can investigate improving those approaches separately. There are other discrepancies in the different code paths (handling of patches, adding sources) but I'm deferring looking over those. I added a test to demonstrate the `--workspace` behavior to use as a base line to compare with. ### Additional information Between this and #12602, this should finally resolve #12599.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This address one of the problems mentioned in #12599
The intent behind the
path_pkgcheck is to make sure we updateworkspace members in the lockfile if their version number changed.
In this case, we don't need to recursively walk, because the change
doesn't affect dependencies. However, we also shouldn't prevent
recursive walks which is what we are doing today, both for packages
marked to keep and for packages that have been "poisoned". So we fix
this by moving that call after all recursive adds are complete so as to
not interfere with them.
This should not affect
Cargo.lockat rest, so no upgrade compatibility concerns.This just allows more packages to be considered available to change which can prevent unclear failures.
The main case I can think of that this does something "undesirable" is when wanting to prevent another "bug" from manifesting: the updating of git dependencies when updating workspace members (#12599). I think I'm ok with that as that needs to be looked into separately.