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

Josh preparations #12759

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Josh preparations #12759

wants to merge 3 commits into from

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented May 3, 2024

Introduce utils mod in clippy_dev

There was some dependence between the different subcommands of clippy_dev. And
this dependence will increased with the introduction of the sync and release
subcommands. This moves the common functions to a utils module, to decouple
the other modules.

Implement sync subcommand in clippy_dev

Now that JOSH is used to sync, it is much easier to script the sync process.
This introduces the two commands sync pull and sync push. The first one will
pull changes from the Rust repo, the second one will push the changes to the
Rust repo. For details, see the documentation in the book.

Implement release subcommand in clippy_dev

This is a QoL improvement for doing releases. The release bump_version
subcommand increments the version in all relevant Cargo.toml files. The
release commit command will only work with the new Josh syncs. Until then the
old way, using git log has to be used.


changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 3, 2024
@flip1995 flip1995 marked this pull request as draft May 3, 2024 09:39
@flip1995 flip1995 force-pushed the josh-automation branch 2 times, most recently from ef84791 to 5276923 Compare May 3, 2024 10:46
clippy_dev/src/sync.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 force-pushed the josh-automation branch 8 times, most recently from 7dcb022 to 3f6693b Compare May 3, 2024 14:44
clippy_dev/src/sync.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 force-pushed the josh-automation branch 2 times, most recently from 3957dad to c9f2c44 Compare May 3, 2024 15:07
@flip1995
Copy link
Member Author

flip1995 commented May 4, 2024

flip1995#8

First sync has over 10k commit 😮 See josh-project/josh#1328

@flip1995 flip1995 added I-nominated Issue: Nominated to be discussed at the next Clippy meeting S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work labels May 4, 2024
@flip1995
Copy link
Member Author

flip1995 commented May 6, 2024

Pro/Con list for Josh:

Pros:

  • First sync takes ~1min instead of hours
  • Later syncs take seconds, instead of minutes1
  • No more massive merge-commits becoming normal commits on sync-back, messing up git blame and git history.
  • Scriptable (this PR), so sync can easily be done by anyone
  • No more patched script from some git/git PR (kinda, see cons)

Cons:

The only big con is the number of commits. All things considered, the pros outweigh the cons IMO.

Footnotes

  1. git subtree push -P src/tools/clippy clippy-local rustup 234.40s user 134.66s system 102% cpu 5:59.71 total (~6 minutes for each sync)

@flip1995 flip1995 removed the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label May 6, 2024
@RalfJung
Copy link
Member

RalfJung commented May 6, 2024 via email

@RalfJung
Copy link
Member

josh-project/josh#1329 landed, so a josh patch shouldn't be required any more (but it'd be good to re-test this with the latest josh master to make sure that the final PR that landed still works as intended).

@flip1995
Copy link
Member Author

Thanks, I will re-test this after the next sync on Thursday.

@flip1995
Copy link
Member Author

From the meeting: The move to Josh got accepted. It would be nice to reduce the number of commits though. I will follow up on this and the move once I'm back from my business trip.

@flip1995 flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label May 15, 2024
@bors

This comment was marked as resolved.

@flip1995 flip1995 force-pushed the josh-automation branch 2 times, most recently from fd562d8 to 01904b0 Compare May 22, 2024 20:40
@bors

This comment was marked as resolved.

@flip1995 flip1995 force-pushed the josh-automation branch 2 times, most recently from 7808dbc to 9a8881f Compare June 28, 2024 08:15
@bors

This comment was marked as resolved.

There was some dependence between the different subcommands of clippy_dev. And
this dependence will increased with the introduction of the sync and release
subcommands. This moves the common functions to a `utils` module, to decouple
the other modules.
Now that JOSH is used to sync, it is much easier to script the sync process.
This introduces the two commands `sync pull` and `sync push`. The first one will
pull changes from the Rust repo, the second one will push the changes to the
Rust repo. For details, see the documentation in the book.
This is a QoL improvement for doing releases. The `release bump_version`
subcommand increments the version in all relevant `Cargo.toml` files. The
`release commit` command will only work with the new Josh syncs. Until then the
old way, using `git log` has to be used.
@tgross35
Copy link
Contributor

It seems like you have been keeping this very up to date - is this just waiting on a final decision? Or is josh-project/josh#1328 considered a blocker.

@flip1995
Copy link
Member Author

We want to move to Josh, that's already decided. I didn't have the time to follow up with josh-project/josh#1328. But IIUC if that should get implemented, we'd need to force push over the Clippy master, if we want to switch to it later.

@RalfJung
Copy link
Member

But IIUC if that should get implemented, we'd need to force push over the Clippy master, if we want to switch to it later.

It's unclear, maybe there are ways to avoid that, e.g. using the rev filter.

@bors
Copy link
Collaborator

bors commented Jul 25, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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