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

Exclude unchanged crates from a release #298

Open
tonyg opened this issue Aug 13, 2021 · 14 comments
Open

Exclude unchanged crates from a release #298

tonyg opened this issue Aug 13, 2021 · 14 comments
Labels
enhancement Improve the expected
Milestone

Comments

@tonyg
Copy link

tonyg commented Aug 13, 2021

Maintainer's notes:

  • Considerations for "unchanged":
    • If it has any changes
    • If it has [[bin]] and the lock file changed
    • If it depends on something being released and the dependency will no longer be compatible (pinned dependency with =, major version changed)
      • This needs to be calculated recursively

PR #296 adds a new flag, --exclude-unchanged, that filters the pkgs list (after applying --package, --exclude etc.) to include only those packages that have changed since the last tagged release, plus their transitive dependencies. It uses the existing change-detection logic, and only marks a package "unchanged" if the code would have reached the existing "Updating {} to {} despite no changes made since tag {}" warning.

@epage
Copy link
Collaborator

epage commented Aug 13, 2021

Not a fan of the flag name but I wonder if we should actually make this the default behavior.

If we make it a default or depending on how we expose this. one question is what to do with package selection flags. --package and --exclude could modify the changed rather than the default set. A couple small changed from clap-cargo might make that easier. --workspace could mean "changed members of workspace". --all is generally a synonym for --workspace but here it might be confusing. I wonder if we should remove it.

How should we handle the lock file? My guess is we should mark everything changed but let people know which ones were only because of the lock file.

If we are making decisions on the users behalf, should we make the change detection more accurate? We can use cargo package --list to see what files would be included in a package (either didn't exist or didn't know about it at the time we did this work)

epage added a commit to epage/cargo-release that referenced this issue Aug 13, 2021
We are now having `cargo` tell us which files are related to a package
and which aren't.

This is a step towards crate-ci#298.

BREAKING CHANGE: We've removed the `exclude-paths` config field.
epage added a commit to epage/cargo-release that referenced this issue Aug 13, 2021
We are now having `cargo` tell us which files are related to a package
and which aren't.

This is a step towards crate-ci#298.

BREAKING CHANGE: We've removed the `exclude-paths` config field.
@epage
Copy link
Collaborator

epage commented Aug 13, 2021

I'm thinking we should at least make this advisory at first as we test out the logic. For example, a corner case I just thought of is we need to be checking if a dependency has changed since the current crate last released and not since the dependency changed since its last release. People can release individual crates for various reasons and we need to account for this.

There are also a lot of dependency releases that don't impact their dependents. As we make this more automatic, we should probably be working to provide the user more information, like what the relevant commit summaries were. The challenge will be organizing all of the data to not overwhelm the user.

epage added a commit to epage/cargo-release that referenced this issue Aug 13, 2021
This isn't to the quality to make decisions on behalf of the user, but
its a start.  Remaining work
- Tracking changes-from-lock file
- Telling the user when a change is only because of the lock file
- Looking for changes in depenencies since the curren crates last
  release, in case the dependency changed
  - Except this will get annoying if the change doesn't matter because
    it will keep bugging the user.

This is a step towards crate-ci#298.
@epage
Copy link
Collaborator

epage commented Aug 13, 2021

Hmm, maybe we shouldn't consider changes from outside the current run. If someone decided not to make a release, it means they will keep having to make that decision each run.

@epage
Copy link
Collaborator

epage commented Aug 13, 2021

Lock files should only impact a crate if it has a [[bin]]. We should be able to look that up from the cargo-metadata.

epage added a commit to epage/cargo-release that referenced this issue Aug 19, 2021
`Cargo.lock` changes do not impact the release of a crate unless its a
`[[bin]]` and someone is going to `cargo install` it.

This is part of crate-ci#298.
@epage
Copy link
Collaborator

epage commented Aug 19, 2021

A way we can narrow down transitive changes is if extract the change detection logic in update_dependent_versions to tell us what dependents will need to be changed. We can then mark only those as changed, rather than all.

This effectively means that a dependent will only be marked changed if we are doing a breaking change. Otherwise, newly generated lock files will pick up the change "for free".

@pksunkara
Copy link

Not a fan of the flag name but I wonder if we should actually make this the default behavior.

If we make it a default or depending on how we expose this. one question is what to do with package selection flags. --package and --exclude could modify the changed rather than the default set. A couple small changed from clap-cargo might make that easier. --workspace could mean "changed members of workspace". --all is generally a synonym for --workspace but here it might be confusing. I wonder if we should remove it.

This is exactly why cargo-workspaces and cargo-release will always be tools for separate jobs. cargo-workspaces only updates the changed crates (with the option to force others) while cargo-release provides familiarity by using the standard cargo arguments assuming that the user knows which crates to update.

If this were to be implemented, the "familiarity by using the standard cargo arguments" reason vanishes from this tool.

@epage
Copy link
Collaborator

epage commented Oct 28, 2021

If this were to be implemented, the "familiarity by using the standard cargo arguments" reason vanishes from this tool.

No. it doesn't

@pksunkara
Copy link

pksunkara commented Oct 28, 2021

--workspace could mean "changed members of workspace"

Meaning of the flag changes with that. And that is what I meant by "familiarity by using the standard cargo arguments" being changed because the meaning of flags are not same anymore. Just the option name being same is not the familiarity I am talking about.

For example, a corner case I just thought of is we need to be checking if a dependency has changed since the current crate last released and not since the dependency changed since its last release.

Feel free to look at how cargo-workspaces does it during versioning/publishing if you want to. It's a pretty simple recursive loop.

@MarcoIeni
Copy link

I just run cargo release --workspace --execute and I found really surprising that it fails to publish the updated crates of the workspace because some of the crates were already published:

$ cargo release --workspace  --execute
[2022-04-11T13:29:50Z ERROR cargo_release::release] Tag `git_cmd-v0.1.2` already exists (for `git_cmd`)
[2022-04-11T13:29:50Z ERROR cargo_release::release] Tag `test_logs-v0.1.2` already exists (for `test_logs`)
[2022-04-11T13:29:50Z ERROR cargo_release::release] Tag `next_version-v0.1.1` already exists (for `next_version`)
[2022-04-11T13:29:50Z ERROR cargo_release::release] Tag `release-plz-v0.2.2` already exists (for `release-plz`)

Also because the dry-run tells you that the updated crate will be published:

$ cargo release --workspace
[2022-04-11T13:29:38Z ERROR cargo_release::release] Tag `git_cmd-v0.1.2` already exists (for `git_cmd`)
[2022-04-11T13:29:38Z ERROR cargo_release::release] Tag `test_logs-v0.1.2` already exists (for `test_logs`)
[2022-04-11T13:29:38Z ERROR cargo_release::release] Tag `next_version-v0.1.1` already exists (for `next_version`)
[2022-04-11T13:29:38Z ERROR cargo_release::release] Tag `release-plz-v0.2.2` already exists (for `release-plz`)
[2022-04-11T13:29:40Z WARN  cargo_release::release] Skipping publish of git_cmd 0.1.2, assuming we are recovering from a prior failed release
[2022-04-11T13:29:40Z WARN  cargo_release::release] Skipping publish of test_logs 0.1.2, assuming we are recovering from a prior failed release
[2022-04-11T13:29:40Z WARN  cargo_release::release] Skipping publish of next_version 0.1.1, assuming we are recovering from a prior failed release
[2022-04-11T13:29:40Z INFO  cargo_release::release] Publishing release_plz_core
    Updating crates.io index
   Packaging release_plz_core v0.2.3 (/home/marco/me/proj/release-plz/crates/release_plz_core)
   Uploading release_plz_core v0.2.3 (/home/marco/me/proj/release-plz/crates/release_plz_core)
warning: aborting upload due to dry run
[2022-04-11T13:29:41Z WARN  cargo_release::release] Skipping publish of release-plz 0.2.2, assuming we are recovering from a prior failed release
[2022-04-11T13:29:41Z INFO  cargo_release::release] Pushing main, git_cmd-v0.1.2 to origin
[2022-04-11T13:29:41Z INFO  cargo_release::release] Pushing main, test_logs-v0.1.2 to origin
[2022-04-11T13:29:41Z INFO  cargo_release::release] Pushing main, next_version-v0.1.1 to origin
[2022-04-11T13:29:41Z INFO  cargo_release::release] Pushing main, release_plz_core-v0.2.3 to origin
[2022-04-11T13:29:41Z INFO  cargo_release::release] Pushing main, release-plz-v0.2.2 to origin
[2022-04-11T13:29:41Z WARN  cargo_release::release] Ran a `dry-run`, re-run with `--execute` if all looked good.

@epage
Copy link
Collaborator

epage commented Apr 11, 2022

Was that first command all of the output? Could you run with a higher verbosity next time so we can have more context for what cargo-release is doing? You can do that by adding multiple -v flags.

@MarcoIeni
Copy link

MarcoIeni commented Apr 12, 2022

Was that first command all of the output?

Yes.

I will try to reproduce this in my toy workspace at commit MarcoIeni/rust-workspace-example@10970de.

Let's publish the first crate

There are three crates. None of them is published yet. Let's publish the first one:

$ cargo release -p marco-test-one --execute -v
[2022-04-12T20:33:06Z DEBUG cargo_release::release] Disabled by user, skipping marco-test-three (no marco-test-three-v0.1.0 tag)
[2022-04-12T20:33:06Z DEBUG cargo_release::release] Disabled by user, skipping marco-test-two (no marco-test-two-v0.1.0 tag)
[2022-04-12T20:33:06Z DEBUG globset] glob converted to regex: Glob { glob: "**/*", re: "(?-u)^(?:/?|.*/)[^/]*$", opts: GlobOptions { case_insensitive: false, literal_separator: true, backslash_escape: true }, tokens: Tokens([RecursivePrefix, ZeroOrMore]) }
[2022-04-12T20:33:06Z DEBUG globset] built glob set; 0 literals, 1 basenames, 0 extensions, 0 prefixes, 0 suffixes, 0 required extensions, 1 regexes
Release marco-test-one 0.1.0? [y/N]
y
[2022-04-12T20:33:13Z INFO  cargo_release::release] Publishing marco-test-one
    Updating crates.io index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging marco-test-one v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-one)
   Verifying marco-test-one v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-one)
   Compiling marco-test-one v0.1.0 (/home/marco/.cargo-target/package/marco-test-one-0.1.0)
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
   Uploading marco-test-one v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-one)
[2022-04-12T20:33:14Z INFO  cargo_release::cargo] Waiting for publish to complete...
[2022-04-12T20:33:17Z DEBUG cargo_release::release] Creating git tag marco-test-one-v0.1.0
[2022-04-12T20:33:17Z INFO  cargo_release::release] Pushing main, marco-test-one-v0.1.0 to origin
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 201 bytes | 201.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0)
To github.com:MarcoIeni/rust-workspace-example.git
 * [new tag]         marco-test-one-v0.1.0 -> marco-test-one-v0.1.0

The crate is published and the tag is created. Perfect 👍

Let's publish the other two crates

I would like to release the other two crates by specifying the --workspace flag.

dry run:

$ cargo release --workspace -v
[2022-04-12T20:35:56Z ERROR cargo_release::release] Tag `marco-test-one-v0.1.0` already exists (for `marco-test-one`)
[2022-04-12T20:35:56Z DEBUG globset] glob converted to regex: Glob { glob: "**/*", re: "(?-u)^(?:/?|.*/)[^/]*$", opts: GlobOptions { case_insensitive: false, literal_separator: true, backslash_escape: true }, tokens: Tokens([RecursivePrefix, ZeroOrMore]) }
[2022-04-12T20:35:56Z DEBUG globset] built glob set; 0 literals, 1 basenames, 0 extensions, 0 prefixes, 0 suffixes, 0 required extensions, 1 regexes
[2022-04-12T20:35:57Z WARN  cargo_release::release] Skipping publish of marco-test-one 0.1.0, assuming we are recovering from a prior failed release
[2022-04-12T20:35:57Z INFO  cargo_release::release] Publishing marco-test-three
[2022-04-12T20:35:57Z DEBUG cargo_release::release] Skipping verification to avoid unpublished dependencies from dry-run
    Updating crates.io index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging marco-test-three v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-three)
   Uploading marco-test-three v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-three)
warning: aborting upload due to dry run
[2022-04-12T20:35:57Z INFO  cargo_release::release] Publishing marco-test-two
[2022-04-12T20:35:57Z DEBUG cargo_release::release] Skipping verification to avoid unpublished dependencies from dry-run
    Updating crates.io index
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
   Packaging marco-test-two v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-two)
   Uploading marco-test-two v0.1.0 (/home/marco/me/proj/rust-workspace-example/crates/marco-test-two)
warning: aborting upload due to dry run
[2022-04-12T20:35:57Z DEBUG cargo_release::release] Creating git tag marco-test-one-v0.1.0
[2022-04-12T20:35:57Z DEBUG cargo_release::release] Creating git tag marco-test-three-v0.1.0
[2022-04-12T20:35:57Z DEBUG cargo_release::release] Creating git tag marco-test-two-v0.1.0
[2022-04-12T20:35:57Z INFO  cargo_release::release] Pushing main, marco-test-one-v0.1.0 to origin
[2022-04-12T20:35:57Z INFO  cargo_release::release] Pushing main, marco-test-three-v0.1.0 to origin
[2022-04-12T20:35:57Z INFO  cargo_release::release] Pushing main, marco-test-two-v0.1.0 to origin
[2022-04-12T20:35:57Z WARN  cargo_release::release] Ran a `dry-run`, re-run with `--execute` if all looked good.

Even if we get an error (it shouldn't be an error in my opinion, because it is expected that some of the crates of the workspace might already be published), it seems that the other two crates are going to be published.

Let's execute it:

$ cargo release --workspace -v --execute
[2022-04-12T20:37:52Z ERROR cargo_release::release] Tag `marco-test-one-v0.1.0` already exists (for `marco-test-one`)

That's all 😢

@epage
Copy link
Collaborator

epage commented Apr 12, 2022

Oh, that made it more obvious. I had missed that the tag existing messages were errors and its been a bit since I've been in this code.

When in a dry-run, we continue through errors so you can see more details but we stop immediately when executing.

Note that the dry run message is saying its trying to recover from a previous failure. We still do things like tagging in that case, so its proper that we check for it and error out because you've told us one thing (release all crates in a workspace) when doing so is suspect (tag already exists). Everything is working as expected with us not having this Issue implemented. You need to run cargo release --workspace -v --execute --exclude marco-test-one.

Something we can improve is to change that last message to say to resolve the errors.

epage added a commit to epage/cargo-release that referenced this issue Apr 12, 2022
Someone reported confusing behavior in a comment in crate-ci#298 that turned out
to be because dry-run failed and we had both missed that.  Now, if
dry-run fails, we'll report a clearer message about it.
@MarcoIeni
Copy link

You need to run cargo release --workspace -v --execute --exclude marco-test-one.

I see, but I was looking for a command I could always run in CI on main:

  • If all the packages are already published, do nothing
  • If some packages are already published, publish the other ones

Is this something you want to support in cargo-release?

@epage
Copy link
Collaborator

epage commented Apr 13, 2022

Keep in mind, as of right now cargo release without a version (relative or absolute) does a subset of the release process (run with and without major with dry run and extra logging to compare).

#117 is an issue for exploring automatically releasing in CI. I need to update it with some of my current thoughts / concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

4 participants