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

feat(xtask): 'changes' xtask #12089

Closed
wants to merge 1 commit into from
Closed

feat(xtask): 'changes' xtask #12089

wants to merge 1 commit into from

Conversation

epage
Copy link
Contributor

@epage epage commented May 5, 2023

This will report what commits affect each publishable package and serves as an alternative to #12085.

CI can run this as cargo changes HEAD~ HEAD^2 and that will capture all of the commits within the PR (this is the range I used in committed).

There is still work needed to integrate this with an action; this is just a rough base-line implementation.

I originally tried to use cargo package --list to determine what files belong to what packages but that was taking too long for some reason (it works well in cargo release for my crates). We can either look into that in the future or do our own heuristics to filter out content.

This will report what commits affect each publishable package and serves
as an alternative to rust-lang#12085.

CI can run this as `cargo changes HEAD~ HEAD^2` and that will capture
all of the commits within the PR (this is the range I used in
`committed`).

There is still work needed to integrate this with an action; this is
just a rough base-line implementation.

I originally tried to use `cargo package --list` to determine what files
belong to what packages but that was taking too long for some reason (it
works well in `cargo release` for my crates).  We can either look into
that in the future or do our own heuristics to filter out content.
@rustbot
Copy link
Collaborator

rustbot commented May 5, 2023

r? @ehuss

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

@rustbot rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023
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 for the fast prototyping. This looks pretty complete to me!

Some thoughts:

  • The cargo project doesn't adopt git conventional commit message pattern. Not sure if it is worthy or we should enforce that.
  • Perhaps switch to gix if we plan to drop git2 someday.
  • If we merge this or else, should we remove xtask-unpublished?
  • The code is indeed in a high quality. I can't be pickier at code level. However, total lines of code usually correlate to maintenance cost. I am a bit worried it's a bit overkill when comparing to what can be done in shell scripts.
  • This is a good demonstration of how people can invent something interesting with cargo and other libraries. I love the pretty status output!

@epage
Copy link
Contributor Author

epage commented May 5, 2023

The cargo project doesn't adopt git conventional commit message pattern. Not sure if it is worthy or we should enforce that.

I copied that over from cargo release changes and am willing to drop it. The intention with both is that this is meant to help raw attention to extra information when its present, not to be prescriptive about it. The way the code is structured, conventional_status reports whether it has something or not and status can then report other information if we want to support other conventions.

Perhaps switch to gix if we plan to drop git2 someday.

Yeah, I stuck with what I was used to / what there are existing resources for. Digging into gix, I think I can switch over to it; the main question is on rev_parse_single as the docs aren't too clear on the behavior.

If we merge this or else, should we remove xtask-unpublished?

Unsure, in part because these are experiments but in part because I don't fully remember what all building blocks I originally intended...

The code is indeed in a high quality. I can't be pickier at code level. However, total lines of code usually correlate to maintenance cost. I am a bit worried it's a bit overkill when comparing to what can be done in shell scripts.

Shell scripts are also deceptively short and error prone :)

I also went past the absolute minimum (e.g. defaulting base-sha and head-sha, less error prone practices like enums over strings) for local usage as an xtask. If we are concerned about the burden we are taking on, we can drop features to be more inline with the shell script.

At the moment, this is also written in a way that I could see it being useful for writing up changelogs, so we might be able to get more mileage out of it.

This is a good demonstration of how people can invent something interesting with cargo and other libraries. I love the pretty status output!

And it is another way for us to dogfood to identify problems, like the fact that I can't make a call to say "tell me what files will be included in a package" but have to shell out for it (except I dropped it due to performance issues I hadn't debugged yet).

This also reminds me that I hope to simplify some of this code in the future. For clap, I created anstream which is like fwdansi on steroids. If we adopted it within clap, we could ore easily write formatted text, rather than having to use the individual Shell::write_stderr calls. Instead, we could do Shell::status and the message line could include formatting and it'd Just Work under all circumstances.

@bors
Copy link
Collaborator

bors commented May 6, 2023

☔ The latest upstream changes (presumably #12096) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
Copy link
Member

At the moment, this is also written in a way that I could see it being useful for writing up changelogs, so we might be able to get more mileage out of it.

This made me wonder we would like to change each PR title before r+. It will make more sense for conventional commit messages.

Unsure, in part because these are experiments but in part because I don't fully remember what all building blocks I originally intended...

My fault. I should ask your original intent first for xtask-unpublished but what I did was just guessing 😅.

To be clear, I see the values in these conventions and the command. However, it feels like going too far away from #12033. I was hoping there could be a way to catch not-yet-bumped versions in CI ASAP, so that we can start splitting internal building blocks into subcrates.

@ehuss
Copy link
Contributor

ehuss commented May 11, 2023

r? weihanglo

I think Weihang is following this more closely than I am. I haven't really reviewed this in detail, but the general concept seems good.

@rustbot rustbot assigned weihanglo and unassigned ehuss May 11, 2023
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.

This looks fine for listing changes. However, I still don't know how it works with requiring user to do a version bump. Did it really give user the information that they need bumping a version for a crate?

paths: std::collections::BTreeSet<std::path::PathBuf>,
}

impl CommitDescription {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably suggest to remove conventional commit part. Let's focus on the original goal for now.

Comment on lines +15 to +16
.arg(clap::Arg::new("base-sha").help("SHA to diff against"))
.arg(clap::Arg::new("head-sha").help("SHA with changes"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you consider copying part of #12126 for CI integration?

@weihanglo
Copy link
Member

weihanglo commented May 11, 2023

[Not a blocker] I still lean toward merging a similar approach like #12126 as a immediate guard rail. We can then properly design and expand this automation, thinking more on what we want for contributors and CI.

bors added a commit that referenced this pull request May 23, 2023
ci: check if any version bump needed for member crates

### What does this PR try to resolve?

Check if a crate requires a version bump in CI.

Part of #12033

### How should we test and review this PR?

Demo action result: <https://github.com/weihanglo/cargo/actions/runs/4941952784/jobs/8835049007>

### Additional information

This doesn't devalue #12089. I love the changelog detection, saving maintainers' times a lot ( I am the one writing changelogs for each release for now). However, the amount of code there is too excessive to me. I think someday when we are going to integrate [cargo-release](https://crates.io/crates/cargo-release) and/or [ehuss/cargo-new-release](https://github.com/ehuss/cargo-new-release), we can remove this script perhaps entirely :)

I tried to document the script a bit hard. Hope it won't get worse over time.
@epage
Copy link
Contributor Author

epage commented Jul 26, 2023

Based on the direction of #12395, I think we can close this. What information we can learn from here has been called out in that other PR.

@epage epage closed this Jul 26, 2023
@epage epage deleted the changes branch July 26, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants