This is an archive of the discontinued LLVM Phabricator instance.

[Workflow] Add Release Repo sync script
ClosedPublic

Authored by thieta on Jan 27 2023, 7:49 AM.

Details

Summary

Adds a bash script that syncs llvm/llvm-project and llvm/llvm-project-release-prs.
This should run on pushes to any of the repositories release branches.

I will follow this up with a change to the github actions to run this
script.

This breaks out the sync script from: https://reviews.llvm.org/D133476
so we can keep them separate.

Diff Detail

Event Timeline

thieta created this revision.Jan 27 2023, 7:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 7:49 AM
thieta requested review of this revision.Jan 27 2023, 7:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 7:49 AM
kwk added a comment.Feb 7 2023, 12:56 PM

@thieta I hope my comments make some sense.

llvm/utils/git/sync-release-repo.sh
6

Does it make sense to check for this origin?

17

If you default to some "random" UUID you don't have to worry about a remote already exsting.

Oh, and since we're at it. Could you explain a bit what origin is for you and what upstream?

42–43

The text says "pushing to origin" but you're pushing to "upstream" is that on purpose? Sorry it is late here and I might not follow everything.

46–53

I'm not sure this should be done. I mean afterall this script is called from a github action and why do you care how the state of that repo looks like? All cleanup steps can be removed IMHO

thieta updated this revision to Diff 495740.Feb 7 2023, 11:44 PM
  • Generate remote and branch names
  • Make terminology a lot clearer
  • Make cleanup optional

@thieta I hope my comments make some sense.

Your comments made total sense, it was hard to follow the different names, I cleaned it up and hopefully it's much easier to follow now. Plus added uuidgen (thanks for that tip!) to generate remotes and branch names.

The cleanup is there to make it easier to debug locally, especially now with the generated names. But I put them in a block for debug, hope that's fine!

thieta marked 4 inline comments as done.Feb 7 2023, 11:46 PM
kwk added inline comments.Feb 9 2023, 3:49 AM
llvm/utils/git/sync-release-repo.sh
21–27

You're only using the remote URLs once so no need for the variable.

I've re-added the CURRENT_BRANCH in order for the cleanup to go back where you came from.

44
56–57
65

One not necessarily was on the main branch.

thieta updated this revision to Diff 496093.Feb 9 2023, 4:50 AM
thieta marked 4 inline comments as done.

Incorporated feedback

thieta added a comment.Feb 9 2023, 4:50 AM

Thanks for your suggestions. I have pushed a new diff with the fixes.

thieta updated this revision to Diff 496095.Feb 9 2023, 5:02 AM

Fix comment

kwk accepted this revision.Feb 10 2023, 3:56 AM

LGTM except for a more detailed explanation in the comments at one place and except for a small change that slipped in because I requested too many changes ;)

llvm/utils/git/sync-release-repo.sh
16–17

Sorry to be nitpicking, but the $MAIN_URL and $REMOTE_URL variables no longer exist, Just add the llvm/llvm-project and llvm/llvm-project-release-prs repo references here is probably enough.

45

It would be nice to understand through the comment here what magic happens and why we need to update again before we can continue with the merge below.

This revision is now accepted and ready to land.Feb 10 2023, 3:56 AM
thieta updated this revision to Diff 496440.Feb 10 2023, 5:40 AM

Expanded and fixed comments

Thanks for your review @kwk - I'll merge this now and start working on the action trigger.

This revision was landed with ongoing or failed builds.Feb 10 2023, 5:41 AM
This revision was automatically updated to reflect the committed changes.