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

Cargo update -p <localcrate> in a workspace has inconsistent behaviors #12599

Closed
roblabla opened this issue Aug 30, 2023 · 9 comments · Fixed by #12975
Closed

Cargo update -p <localcrate> in a workspace has inconsistent behaviors #12599

roblabla opened this issue Aug 30, 2023 · 9 comments · Fixed by #12975
Labels
A-dependency-resolution Area: dependency resolution and the resolver Command-update

Comments

@roblabla
Copy link
Contributor

Cargo's handling of cargo update -p <cratename> is extremely inconsistent. Depending on how local crates are named or how up-to-date the Cargo.toml is compared to the Cargo.lock file, it will behave very differently with regards to git dependencies.

I have created a repository explaining the problems I found @ https://github.com/roblabla/cargo-wtf-why-are-you-drunk/

In that repository, you'll find two examples of inconsistencies in the behavior of cargo update -p:

  • In the first one, we show that cargo seems to handle git dependencies differently depending on whether it's got "work" to do on local crates or not. In particular, in my local monorepo, if I bump the version of a local crate in Cargo.toml, then cargo update -p <crate> will update the local lockfile to bump the versions of crate and nothing else. However, if I don't bump the version and run the same cargo update -p <crate> command, cargo will update the git dependencies.

    This discrepancy is especially frustrating, as it complicates our release process by making bumping the version more complicated.

  • In the second one, we show that whether or not cargo updates the git dependencies after bumping the version seem to depend on how your local crates in your workspace are named. This makes any call to cargo update -p <crate> inherently fragile, as what it does will depend on how your dependencies are named.

Possible Solutions

IMO, cargo update -p <crate> should never update an unrelated git dependency, as it does not match what the user intended.

Rust version:

rust 1.71.0 (8ede3aae2 2023-07-12)

@epage
Copy link
Contributor

epage commented Aug 30, 2023

So exhibit 2 involves running cargo update -p <workspace-member> and a git dependency gets updated.

I ran CARGO_LOG=trace and did a diff of the logs.

The sad case has a long list of

ignoring any lock pointing directly

that when only 1 exists in the happy case.

@epage
Copy link
Contributor

epage commented Aug 30, 2023

It looks like our tracking of what should be ignored is order dependent, depending on whether the package with the changed version is seen first or not.

epage added a commit to epage/cargo that referenced this issue Aug 30, 2023
@epage
Copy link
Contributor

epage commented Aug 30, 2023

@roblabla your Exhibit One reproduction cases have trigger.sh with different behavior. Could you unify them in the direction that is needed?

@epage
Copy link
Contributor

epage commented Aug 30, 2023

Correction, I'm assuming that difference is intended to showcase the difference in behavior.

We have a check where we only avoid a path dependency if its changed. That "if" i what is causing the problem here.

@roblabla
Copy link
Contributor Author

Correction, I'm assuming that difference is intended to showcase the difference in behavior.

Yeah, the trigger.sh in the happy case changes the version in the manifest, while in the sad case it doesn't, which causes the unexpected difference in behavior.

@epage
Copy link
Contributor

epage commented Aug 30, 2023

@Eh2406 for Exhibit One (cargo update -p workspace-member updating git deps when there is nothing to update for the workspace member),removing if pkg.package_id() != node seems to fix it but I'm hesitant to just randomly delete code. Thoughts?

@epage
Copy link
Contributor

epage commented Aug 30, 2023

@roblabla seems like these problems are very different, even in reproduction steps (one being order dependent while the other one isn't). Please in the future create separate Issues so we can more easily track the individual conversations and progress.

@roblabla
Copy link
Contributor Author

Alright. Reason they're together is that I actually discovered exhibit 2 while trying to reproduce exhibit 1 😅

epage added a commit to epage/cargo that referenced this issue Aug 30, 2023
epage added a commit to epage/cargo that referenced this issue Aug 31, 2023
epage added a commit to epage/cargo that referenced this issue Aug 31, 2023
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".
epage added a commit to epage/cargo that referenced this issue Aug 31, 2023
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.
@epage
Copy link
Contributor

epage commented Aug 31, 2023

@roblabla as an update, my change in #12602 was incorrect and it is now changed so you'll be seeing git updates in more cases.

We still need to dig into if its possible to skip these git updates. They go through a bit of a different path to evaluate if we should update because we only allow one version in the dependency graph and we'll need to further evaluate what can and shouldn't be changed there.

epage added a commit to epage/cargo that referenced this issue Sep 1, 2023
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.
epage added a commit to epage/cargo that referenced this issue Sep 1, 2023
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.
epage added a commit to epage/cargo that referenced this issue Sep 1, 2023
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.
bors added a commit that referenced this issue Sep 5, 2023
fix(resolver): Make resolver behavior independent of package order

This address one of the problems mentioned in #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.

This should not affect `Cargo.lock` at 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.
@epage epage added Command-update A-dependency-resolution Area: dependency resolution and the resolver labels Nov 3, 2023
epage added a commit to epage/cargo that referenced this issue 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 issue 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 issue 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 issue 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.
@bors bors closed this as completed in 9ca6376 Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver Command-update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants