They have this feature for issues, we have talked to them about adding it for Pull Requests, but for now we would probably need our own notification system, like we have for issues.
I’m also one of those who loves Phabricator and the workflow we have/had. But I’m keen to move forward and learn if it helps makes contributing better.
Is it ok now to test the process a little before it becoming mandatory on 1st Sept?, my fear is unless those code owners who look at LLVM/clang code daily in Phabricator don’t start to become familiar with GitHub we could be inundated with PRs, I’d like to be able to practice a whitespace change PR with the others in the clang-format team. Is that ok now or is it still blocked?
1 Like
We have been enabling pull requests for some people at the directory level, so just let me know what directories are relevant to clang-format, and I’ll add those to the allowlist.
When you land ⚙ D156595 Workflows: Allow pull requests for .github directory, maybe I can send you a simple PR to add the directories
@mydeveloperday OK, this patch is merged now.
Just for information, I am currently setting up pre-commit CI that will run whenever a Github PR is open. For now I am handling Clang and libc++/libc++abi/libunwind only because that’s the projects I am familiar with, but I think the setup I use would make it easy for arbitrary sub-projects to run the checks that are relevant for them.
2 Likes
… and I am planning to move more generic parts to the similar footing (but simplified compared to the current setup).
With recommending Github PRs being less than a week away, is there already documentation on the new recommended workflows for reviewers?
It also looks like most issues for the Pull Request Migration project are still open/unresolved (Pull Request Migration · GitHub). Are any of those considered critical/blockers before moving to Github PRs?
5 Likes
I am happy to start writing a guide for the GH workflow unless someone else is already working on this. I can have something to be reviewed by early next week.
Feel free to suggest things we should cover in the “basic” workflow guide. There are advanced topics that I think we can cover in other topics (like stacked reviews).
It’d be nice if you could include a description of the recommended stacked patches workflow because GitHub obviously lacks the needed built-in functionality, but I think it’s a essential for productive work. So, it’d be beneficial if we had a clear and standardized process.
1 Like
I am committing to docs for the basic workflow. I don’t use stacked commits workflow myself and never used it - so I think it’s better that someone else writes and describes that.
1 Like
Any news about the updated workflows?
For example Contributing to LLVM — LLVM 18.0.0git documentation still describe the phabricator way of working. Not sure where I can find the guidelines and rules for the new workflow (that I guess still is planned to be used starting tomorrow)?
Personally I’ve never ever used GitHub PR:s, so that will be a totally new experience (I for example have no clue if my local repo still should point at llvm-project as remote or how this thing with forks are working. Maybe I’m supposed to have my fork as push remote, and fetch from llvm-project? But what if I need to make a revert or need to push without review? Then I want to push to the llvm-project remote directly. I guess I’ll need to mess around a lot more with remotes, or maybe I need to have multiple local repos for different tasks.).
I guess I’ll be able to find basic information about how to fork and create pull requests in github documentation. But what will for example happen to “Reviewers:” and “Subscribers:” fields. Should I still use those when submitting something for review? Or how do I add reviewers in the new workflow? Who will merge the patch when it is accepted (am I still doing that myself or is it “pulled” by someone else in the new workflow)?
Btw, there is no issue connected to updating the way-of-working documents that can be found in the “Pull Request Migration” project (Pull Request Migration · GitHub). So a bit hard to find information about these things and the progress on updating the “how to contribute” documentation.
1 Like
I am working on it, but I haven’t had enough time to do it as I hoped.
I think the github documentation is pretty good, and I also really recommend the GitHub CLI (cli.github.com).
Here what I have written as a overview so far:
- Clone the repo
gh repo clone llvm/llvm-project - Create a new branch
git switch -c my_changes - Make your changes and create one or more commits. Remember that the LLVM project likes commits to be singular in purpose.
- Run
gh pr createthis will ask you if you want to create a fork, select this value. It will walk you through doing the rest of the PR. - Reviewers will be assigned with the help of the CODEOWNERS file, but if that fails, assign some other reviewers.
- If you need to update the PR, make your commits in your branch and push them to the branch in your fork. Don’t squash these commits yet, you want to show your changes as separate commits.
- When the review is done, squash commits that are just follow-ups. This is easiest done with
git rebase --interactive origin/mainand mark the commits withffor fixup. There are also tools likegit-absorbthat can help with this process. - When the PR is accepted, you can use
gh pr merge --delete-branch --rebaseor click therebaseoption in the web UI.
Hope this helps. I will try to get the documentation ready for review as soon as possible.
2 Likes
Thanks for the update.
One thing that maybe is missing in that overview is how to rebase the PR to a newer base. It says that I shouldn’t squash commits when updating my PR. Is it still OK to rebase the branch (then I certainly may need to touch the existing commits on the branch)?
(Maybe that is a more advanced workflow not to be covered by the overview. But it was the first question that popped up in my head after reading the overview.)
One more thing. Not sure if we can rely on users having access to “gh”.
It would probably take some time to get the company to install that (or even allow us to use it).
So unfortunately, things like gh pr create isn’t something I think I’ll be able to use tomorrow at least.
Sure, this is similar to people using arc or not. Everything is, of course, supported without it and I’ll make sure to mention how to do it without gh as well. But at the same time, I would never have enjoyed using Phab without arc, and I will say the same for github and its CLI.
I think you definitely should rebase when convenient. The more often the less risk of merge problems.
Are there any plans for something like GitHubs merge queue? If I understand correctly, the current workflow will essentially be clicking merge when the review is done, that will squash all the commits to a single commit (to retain our linear history) and push it to main.
But there is no guarantee that this commit will work with main if there have been any changes inbetween the last CI run and clicking merge.
Will pushes to main still work or should things like revert commits or trivial fixes also go through PRs (and maybe self-accept?).
2 Likes
Doesn’t a rebase entail a force push, which in turn can cause problems with commit comments disappearing from any convenient view? (Mind you, so do outdated comments (the code in the area has changed since the comment was made), at least for the GitHub Enterprise version my company uses…) I was under the impression we’d agreed something on the topic of force pushes, but I forget what it was.
1 Like
The naive test I did for this seems to work, i.e. the comments are still there after a force push. The context is lost (i.e. I can’t seem to restore the change I commented on). I would say that maybe rebasing should be used sparingly until the change is fully reviewed unless a rebase is required to pass the review (passing tests etc.).