killercup / cargo-edit

A utility for managing cargo dependencies from the command line.
http://killercup.github.io/cargo-edit/
MIT License
3.09k stars 148 forks source link

Add support for workspace-wide version setting and bumping #814

Closed poliorcetics closed 2 years ago

poliorcetics commented 2 years ago

This PR adds support for the workspace.package.version added in Cargo 1.64.0.

I tried to make very atomic commits to make it easier to review since it needed quite some refactoring, I advise to review commit by commit else it will be quite heavy to read.

Closes #752

poliorcetics commented 2 years ago

@epage so the wanted behavior is to update the workspace-wide version if one of the two is true:

1) --workspace was given 2) --package <pkg> was given where pkg's version is inherited from the workspace

Is that correct ?

poliorcetics commented 2 years ago

All fixed, there is only the one question about --workspace-version merging with --workspace (and loosing the conflict with --exclude) or not

epage commented 2 years ago

Sorry for the delay; got caught up in clap v4 release stuff.

@epage so the wanted behavior is to update the workspace-wide version if one of the two is true:

1. `--workspace` was given

2. `--package <pkg>` was given where `pkg`'s version is inherited from the workspace

Is that correct ?

All fixed, there is only the one question about --workspace-version merging with --workspace (and loosing the conflict with --exclude) or not

A simpler take is that if a package is specified that uses version.workspace = true then we automatically update it.

We already have command-line flag processing separated from iterating over workspace members, so we can just keep that.

poliorcetics commented 2 years ago

Sorry for the delay; got caught up in clap v4 release stuff.

No worries, you don't owe me any of your free time 😄


I fixed a bug where workspace declared in the same Cargo.toml as a regular crate would update all the workspace.dependencies, regardless of if they were concerned by the update and addressed your comments.

I hope there aren't more bugs I missed, I think I tested pretty much everything now 😅

epage commented 2 years ago

FYI there are some things that aren't quite working out for me with this PR. I think part of it is the base you are working off of. I'm trying to get some changes made that will make adding workspace inheritance more comprehensible.

epage commented 2 years ago

Alright, things are in better shape.

My assumption is that we will partition the selected manifests into two groups: those inheriting and those that aren't. When we call update_dependents for the inheriting case, we pass in a &[PathBuf] of all of the dep crate roots that use workspace dependencies. We likely don't need more logic split out into new functions. Forcing code sharing when it doesn't quite work can make the code harder to follow than easier.

poliorcetics commented 2 years ago

So I can close this ? Seems like you added the code to support it ?

Ah no, workspace.version is not updated correctly yet

poliorcetics commented 2 years ago

Rebased, it was indeed much much simpler

epage commented 2 years ago

Thanks!