Use josh-sync instead of miri-script for Josh synchronization
#4490
Conversation
|
Thank you for contributing to Miri! |
CONTRIBUTING.md
Outdated
| assuming we are on an up-to-date master branch: | ||
|
|
||
| ```sh | ||
| # Push the Miri changes to your rustc fork (substitute your github handle for YOUR_NAME). | ||
| ./miri rustc-push YOUR_NAME miri | ||
| rustc-josh-sync push YOUR_NAME miri |
There was a problem hiding this comment.
The docs were outdated, the branch name is optional now.
It seems like josh-sync still requires it? I guess I can get used to that again.
There was a problem hiding this comment.
It does require it now, yeah. We could add a default branch name for pushes to josh-sync.toml, although I find it a bit weird, because the branch is pushed to a rustc fork of the person doing the push, so in a sense it seems like a "user-centric" property, rather than something that is shared across everyone doing miri synces (unless it's just a single person 😆).
There was a problem hiding this comment.
It's true that that is more of a per-user thing, but if the name is sufficiently clear, like miri-sync, it's unlikely to collide. I copied this idea of having a default from RA, if I remember correctly.
|
I can't really test pulling since there's currently no changes, but pushing works fine and so surely pulling will also work. Can you please clean up the git history? Then we can land this. :) We can always adjust the docs later e.g. if default branches become a thing. |
There is still this issue ^ I think that the current CI workflow won't work correctly, because when there's nothing to pull, rustc-josh-sync will rollback the created commits, so when the CI workflow compares diff against I changed the workflow to compare with |
.github/workflows/ci.yml
Outdated
| - name: setup bot git name and email | ||
| run: | | ||
| git config --global user.name 'The Miri Cronjob Bot' | ||
| git config --global user.email 'miri@cron.bot' | ||
| - name: Install nightly toolchain | ||
| run: rustup toolchain install nightly --profile minimal | ||
| - name: get changes from rustc | ||
| run: ./miri rustc-pull | ||
| run: rustc-josh-sync pull --allow-noop |
There was a problem hiding this comment.
Didn't you do some exit code things that we should check here to determine whether it's a NOP, and then just skip the rest of the steps?
There was a problem hiding this comment.
Well, yes, by default the pull will exit with status code 2. But that also fails the whole job, which is not what we want. And if it exits with status code 0 (which is what --allow-noop does), the job will continue. There is AFAIK not an easy way to tell GH "stop this job, but also mark it as success". We would need to e.g. export some step output or an environment variable and then skip all the subsequent steps if it is set to true, which is what the default workflow does (https://github.com/rust-lang/josh-sync/blob/main/.github/workflows/rustc-pull.yml#L79). I didn't use the default workflow here though, because we also need to do the miri fmt postprocessing.
If you're fine with the approach of passing data between steps, I can replicate it here. But comparing with GITHUB_SHA seemed easier.
There was a problem hiding this comment.
Those GITHUB_OUTPUT hacks don't look pleasant at all.
To me the cleanest solution seems to be to simply merge all these steps into one, so we can nicely express early-successful-exit.
There was a problem hiding this comment.
I don't see it as a hack, it's a normal way to pass output between steps, even if a bit verbose. That being said, I was actually hoping that you would suggest combining everything into a single step 😆 Done that in the latest commit: 339dbf7
There was a problem hiding this comment.
Right, I guess "excessively verbose" is more accurate than "hack". IMO Github didn't design this very well, but whatever.
|
Done. |
|
Awesome, thanks! |
Discussed on Zulip. Removes local code, configures josh-sync, updates documentation and CI workflow.
Now that I think about it, I'm not sure if
--allow-noopwill be enough here. Becauserustc-josh-syncwill automatically rollup the commit and changes if it fails to pull anything, so checking forHEAD^probably won't do what we want. Maybe we can ignore the follow-up steps ifjosh-syncreturns exit status 2? (in that case there's nothing to pull)