1 .. _phabricator-reviews:
3 =============================
4 Code Reviews with Phabricator
5 =============================
10 If you prefer to use a web user interface for code reviews, you can now submit
11 your patches for Clang and LLVM at `LLVM's Phabricator`_ instance.
13 While Phabricator is a useful tool for some, the relevant -commits mailing list
14 is the system of record for all LLVM code review. The mailing list should be
15 added as a subscriber on all reviews, and Phabricator users should be prepared
16 to respond to free-form comments in mail sent to the commits list.
21 To get started with Phabricator, navigate to `https://reviews.llvm.org`_ and
22 click the power icon in the top right. You can register with a GitHub account,
23 a Google account, or you can create your own profile.
25 Make *sure* that the email address registered with Phabricator is subscribed
26 to the relevant -commits mailing list. If you are not subscribed to the commit
27 list, all mail sent by Phabricator on your behalf will be held for moderation.
29 Note that if you use your git user name as Phabricator user name,
30 Phabricator will automatically connect your submits to your Phabricator user in
31 the `Code Repository Browser`_.
33 Requesting a review via the command line
34 ----------------------------------------
36 Phabricator has a tool called *Arcanist* to upload patches from
37 the command line. To get you set up, follow the
38 `Arcanist Quick Start`_ instructions.
40 You can learn more about how to use arc to interact with
41 Phabricator in the `Arcanist User Guide`_.
42 The basic way of creating a revision for the current commit in your local
50 Sometime you may want to create a draft revision to show the proof of concept
51 or for experimental purposes, In that case you can use the `--draft` option. It
52 will create a new draft revision. The good part is: it will not send mail to
53 llvm-commit mailing list, patch reviewers, and all other subscribers, buildbot
54 will also run on every patch update:
58 arc diff --draft HEAD~
61 If you later update your commit message, you need to add the `--verbatim`
62 option to have `arc` update the description on Phabricator:
66 arc diff --edit --verbatim
69 .. _phabricator-request-review-web:
71 Requesting a review via the web interface
72 -----------------------------------------
74 The tool to create and review patches in Phabricator is called
77 Note that you can upload patches created through git, but using `arc` on the
78 command line (see previous section) is preferred: it adds more metadata to
79 Phabricator which are useful for the pre-merge testing system and for
80 propagating attribution on commits when someone else has to push it for you.
82 To make reviews easier, please always include **as much context as
83 possible** with your diff! Don't worry, Phabricator
84 will automatically send a diff with a smaller context in the review
85 email, but having the full file in the web interface will help the
86 reviewer understand your code.
88 To get a full diff, use one of the following commands (or just use Arcanist
89 to upload your patch):
91 * ``git show HEAD -U999999 > mypatch.patch``
92 * ``git diff -U999999 @{u} > mypatch.patch``
93 * ``git diff HEAD~1 -U999999 > mypatch.patch``
95 Before uploading your patch, please make sure it is formatted properly, as
96 described in :ref:`How to Submit a Patch <format patches>`.
98 To upload a new patch:
100 * Click *Differential*.
101 * Click *+ Create Diff*.
102 * Paste the text diff or browse to the patch file. Click *Create Diff*.
103 * Leave this first Repository field blank. (We'll fill in the Repository
104 later, when sending the review.)
105 * Leave the drop down on *Create a new Revision...* and click *Continue*.
106 * Enter a descriptive title and summary. The title and summary are usually
107 in the form of a :ref:`commit message <commit messages>`.
108 * Add reviewers (see below for advice). (If you set the Repository field
109 correctly, llvm-commits or cfe-commits will be subscribed automatically;
110 otherwise, you will have to manually subscribe them.)
111 * In the Repository field, enter "rG LLVM Github Monorepo".
114 To submit an updated patch:
116 * Click *Differential*.
117 * Click *+ Create Diff*.
118 * Paste the updated diff or browse to the updated patch file. Click *Create Diff*.
119 * Select the review you want to from the *Attach To* dropdown and click
121 * Leave the Repository field blank. (We previously filled out the Repository
122 for the review request.)
123 * Add comments about the changes in the new diff. Click *Save*.
125 Choosing reviewers: You typically pick one or two people as initial reviewers.
126 This choice is not crucial, because you are merely suggesting and not requiring
127 them to participate. Many people will see the email notification on cfe-commits
128 or llvm-commits, and if the subject line suggests the patch is something they
129 should look at, they will.
132 .. _finding-potential-reviewers:
134 Finding potential reviewers
135 ---------------------------
137 Here are a couple of ways to pick the initial reviewer(s):
139 * Use ``git blame`` and the commit log to find names of people who have
140 recently modified the same area of code that you are modifying.
141 * Look in CODE_OWNERS.TXT to see who might be responsible for that area.
142 * If you've discussed the change on a dev list, the people who participated
143 might be appropriate reviewers.
145 Even if you think the code owner is the busiest person in the world, it's still
146 okay to put them as a reviewer. Being the code owner means they have accepted
147 responsibility for making sure the review happens.
149 Reviewing code with Phabricator
150 -------------------------------
152 Phabricator allows you to add inline comments as well as overall comments
153 to a revision. To add an inline comment, select the lines of code you want
154 to comment on by clicking and dragging the line numbers in the diff pane.
155 When you have added all your comments, scroll to the bottom of the page and
156 click the Submit button.
158 You can add overall comments in the text box at the bottom of the page.
159 When you're done, click the Submit button.
161 Phabricator has many useful features, for example allowing you to select
162 diffs between different versions of the patch as it was reviewed in the
163 *Revision Update History*. Most features are self descriptive - explore, and
164 if you have a question, drop by on #llvm in IRC to get help.
166 Note that as e-mail is the system of reference for code reviews, and some
167 people prefer it over a web interface, we do not generate automated mail
168 when a review changes state, for example by clicking "Accept Revision" in
169 the web interface. Thus, please type LGTM into the comment box to accept
170 a change from Phabricator.
172 .. _pre-merge-testing:
177 The pre-merge tests are a continuous integration (CI) workflow. The workflow
178 checks the patches uploaded to Phabricator before a user merges them to the main
179 branch - thus the term *pre-merge testing*.
181 When a user uploads a patch to Phabricator, Phabricator triggers the checks and
182 then displays the results. This way bugs in a patch are contained during the
183 code review stage and do not pollute the main branch.
185 Our goal with pre-merge testing is to report most true problems while strongly
186 minimizing the number of false positive reports. Our goal is that problems
187 reported are always actionable. If you notice a false positive, please report
188 it so that we can identify the cause.
190 If you notice issues or have an idea on how to improve pre-merge checks, please
191 `create a new issue <https://github.com/google/llvm-premerge-checks/issues/new>`_
192 or give a ❤️ to an existing one.
197 To get a patch on Phabricator tested, the build server must be able to apply the
198 patch to the checked out git repository. Please make sure that either:
200 * You set a git hash as ``sourceControlBaseRevision`` in Phabricator which is
201 available on the GitHub repository,
202 * **or** you define the dependencies of your patch in Phabricator,
203 * **or** your patch can be applied to the main branch.
205 Only then can the build server apply the patch locally and run the builds and
208 Accessing build results
209 ^^^^^^^^^^^^^^^^^^^^^^^
210 Phabricator will automatically trigger a build for every new patch you upload or
211 modify. Phabricator shows the build results at the top of the entry. Clicking on
212 the links (in the red box) will show more details:
214 .. image:: Phabricator_premerge_results.png
216 The CI will compile and run tests, run clang-format and clang-tidy on lines
219 If a unit test failed, this is shown below the build status. You can also expand
220 the unit test to see the details:
222 .. image:: Phabricator_premerge_unit_tests.png
227 In case you want to opt-out entirely of pre-merge testing, add yourself to the
228 `OPT OUT project <https://reviews.llvm.org/project/view/83/>`_. If you decide
229 to opt-out, please let us know why, so we might be able to improve in the future.
234 The code responsible for running the pre-merge flow can be found in the `external
235 repository <https://github.com/google/llvm-premerge-checks>`_. For enhancement
236 ideas and most bugs, please file an issue on said repository. For immediate
237 operational problems, the point of contact is
238 `Mikhail Goncharov <mailto:goncharo@google.com>`_.
240 Background on the pre-merge infrastructure can be found in `this 2020 DevMeeting
241 talk <https://llvm.org/devmtg/2020-09/slides/Goncharov-Pre-merge_checks.pdf>`_
246 Once a patch has been reviewed and approved on Phabricator it can then be
247 committed to trunk. If you do not have commit access, someone has to
248 commit the change for you (with attribution). It is sufficient to add
249 a comment to the approved review indicating you cannot commit the patch
250 yourself. If you have commit access, there are multiple workflows to commit the
251 change. Whichever method you follow it is recommended that your commit message
256 Differential Revision: <URL>
258 where ``<URL>`` is the URL for the code review, starting with
259 ``https://reviews.llvm.org/``.
261 This allows people reading the version history to see the review for
262 context. This also allows Phabricator to detect the commit, close the
263 review, and add a link from the review to the commit.
265 Note that if you use the Arcanist tool the ``Differential Revision`` line will
266 be added automatically. If you don't want to use Arcanist, you can add the
267 ``Differential Revision`` line (as the last line) to the commit message
270 Using the Arcanist tool can simplify the process of committing reviewed code as
271 it will retrieve reviewers, the ``Differential Revision``, etc from the review
272 and place it in the commit message. You may also commit an accepted change
273 directly using ``git push``, per the section in the :ref:`getting started
274 guide <commit_from_git>`.
276 Note that if you commit the change without using Arcanist and forget to add the
277 ``Differential Revision`` line to your commit message then it is recommended
278 that you close the review manually. In the web UI, under "Leap Into Action" put
279 the git revision number in the Comment, set the Action to "Close Revision" and
280 click Submit. Note the review must have been Accepted first.
282 Committing someone's change from Phabricator
283 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
285 On a clean Git repository on an up to date ``main`` branch run the
286 following (where ``<Revision>`` is the Phabricator review number):
290 arc patch D<Revision>
293 This will create a new branch called ``arcpatch-D<Revision>`` based on the
294 current ``main`` and will create a commit corresponding to ``D<Revision>`` with a
295 commit message derived from information in the Phabricator review.
297 Check you are happy with the commit message and amend it if necessary.
298 For example, ensure the 'Author' property of the commit is set to the original author.
299 You can use a command to correct the author property if it is incorrect:
303 git commit --amend --author="John Doe <jdoe@llvm.org>"
305 Then, make sure the commit is up-to-date, and commit it. This can be done by running
310 git pull --rebase https://github.com/llvm/llvm-project.git main
311 git show # Ensure the patch looks correct.
312 ninja check-$whatever # Rerun the appropriate tests if needed.
313 git push https://github.com/llvm/llvm-project.git HEAD:main
319 If you decide you should not commit the patch, you should explicitly abandon
320 the review so that reviewers don't think it is still open. In the web UI,
321 scroll to the bottom of the page where normally you would enter an overall
322 comment. In the drop-down Action list, which defaults to "Comment," you should
323 select "Abandon Revision" and then enter a comment explaining why. Click the
324 Submit button to finish closing the review.
329 Please let us know whether you like it and what could be improved! We're still
330 working on setting up a bug tracker, but you can email klimek-at-google-dot-com
331 and chandlerc-at-gmail-dot-com and CC the llvm-dev mailing list with questions
332 until then. We also could use help implementing improvements. This sadly is
333 really painful and hard because the Phabricator codebase is in PHP and not as
334 testable as you might like. However, we've put exactly what we're deploying up
335 on an `llvm-reviews GitHub project`_ where folks can hack on it and post pull
336 requests. We're looking into what the right long-term hosting for this is, but
337 note that it is a derivative of an existing open source project, and so not
338 trivially a good fit for an official LLVM project.
340 .. _LLVM's Phabricator: https://reviews.llvm.org
341 .. _`https://reviews.llvm.org`: https://reviews.llvm.org
342 .. _Code Repository Browser: https://reviews.llvm.org/diffusion/
343 .. _Arcanist Quick Start: https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/
344 .. _Arcanist User Guide: https://secure.phabricator.com/book/phabricator/article/arcanist/
345 .. _llvm-reviews GitHub project: https://github.com/r4nt/llvm-reviews/