review.rst 23.4 KB
Newer Older
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
CMake Review Process
********************

The following documents the process for reviewing and integrating changes.
See `CONTRIBUTING.rst`_ for instructions to contribute changes.
See documentation on `CMake Development`_ for more information.

.. _`CONTRIBUTING.rst`: ../../CONTRIBUTING.rst
.. _`CMake Development`: README.rst

.. contents:: The review process consists of the following steps:

Merge Request
=============

A user initiates the review process for a change by pushing a *topic
branch* to his or her own fork of the `CMake Repository`_ on GitLab and
creating a *merge request* ("MR").  The new MR will appear on the
`CMake Merge Requests Page`_.  The rest of the review and integration
process is managed by the merge request page for the change.

During the review process, the MR submitter should address review comments
or test failures by updating the MR with a (force-)push of the topic
branch.  The update initiates a new round of review.

We recommend that users enable the "Remove source branch when merge
request is accepted" option when creating the MR or by editing it.
This will cause the MR topic branch to be automatically removed from
the user's fork during the `Merge`_ step.

31
.. _`CMake Merge Requests Page`: https://gitlab.kitware.com/cmake/cmake/-/merge_requests
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
.. _`CMake Repository`: https://gitlab.kitware.com/cmake/cmake

Workflow Status
---------------

`CMake GitLab Project Developers`_ may set one of the following labels
in GitLab to track the state of a MR:

* ``workflow:wip`` indicates that the MR needs additional updates from
  the MR submitter before further review.  Use this label after making
  comments that require such updates.

* ``workflow:in-review`` indicates that the MR awaits feedback from a
  human reviewer or from `Topic Testing`_.  Use this label after making
  comments requesting such feedback.

* ``workflow:nightly-testing`` indicates that the MR awaits results
  of `Integration Testing`_.  Use this label after making comments
  requesting such staging.

* ``workflow:expired`` indicates that the MR has been closed due
  to a period of inactivity.  See the `Expire`_ step.  Use this label
  after closing a MR for this reason.

56
57
58
59
* ``workflow:external-discussion`` indicates that the MR has been closed
  pending discussion elsewhere.  See the `External Discussion`_ step.
  Use this label after closing a MR for this reason.

60
61
62
The workflow status labels are intended to be mutually exclusive,
so please remove any existing workflow label when adding one.

63
.. _`CMake GitLab Project Developers`: https://gitlab.kitware.com/cmake/cmake/-/settings/members
64
65
66
67
68
69
70
71

Robot Review
============

The "Kitware Robot" (``@kwrobot``) automatically performs basic checks on
the commits proposed in a MR.  If all is well the robot silently reports
a successful "build" status to GitLab.  Otherwise the robot posts a comment
with its diagnostics.  **A topic may not be merged until the automatic
72
73
74
75
76
77
78
79
review succeeds.**

Note that the MR submitter is expected to address the robot's comments by
*rewriting* the commits named by the robot's diagnostics (e.g., via
``git rebase -i``). This is because the robot checks each commit individually,
not the topic as a whole. This is done in order to ensure that commits in the
middle of a topic do not, for example, add a giant file which is then later
removed in the topic.
80

81
82
83
Automatic Check
---------------

84
85
86
87
88
89
90
91
92
The automatic check is repeated whenever the topic branch is updated.
One may explicitly request a re-check by adding a comment with the
following command among the `comment trailing lines`_::

  Do: check

``@kwrobot`` will add an award emoji to the comment to indicate that it
was processed and also run its checks again.

93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
Automatic Format
----------------

The automatic check will reject commits introducing source code not
formatted according to ``clang-format``.  One may ask the robot to
automatically rewrite the MR topic branch with expected formatting
by adding a comment with the following command among the
`comment trailing lines`_::

  Do: reformat

``@kwrobot`` will add an award emoji to the comment to indicate that it
was processed and also rewrite the MR topic branch and force-push an
updated version with every commit formatted as expected by the check.

108
109
110
111
112
Human Review
============

Anyone is welcome to review merge requests and make comments!

113
114
115
116
Please make comments directly on the MR page Discussion and Changes tabs
and not on individual commits.  Comments on a commit may disappear
from the MR page if the commit is rewritten in response.

117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
Reviewers may add comments providing feedback or to acknowledge their
approval.  Lines of specific forms will be extracted during the `merge`_
step and included as trailing lines of the generated merge commit message.
Each review comment consists of up to two parts which must be specified
in the following order: `comment body`_, then `comment trailing lines`_.
Each part is optional, but they must be specified in this order.

Comment Body
------------

The body of a comment may be free-form `GitLab Flavored Markdown`_.
See GitLab documentation on `Special GitLab References`_ to add links to
things like issues, commits, or other merge requests (even across projects).

Additionally, a line in the comment body may start with one of the
following votes:

* ``-1`` or ``:-1:`` indicates "the change is not ready for integration".

* ``+1`` or ``:+1:`` indicates "I like the change".
  This adds an ``Acked-by:`` trailer to the `merge`_ commit message.

* ``+2`` indicates "the change is ready for integration".
  This adds a ``Reviewed-by:`` trailer to the `merge`_ commit message.

* ``+3`` indicates "I have tested the change and verified it works".
  This adds a ``Tested-by:`` trailer to the `merge`_ commit message.

.. _`GitLab Flavored Markdown`: https://gitlab.kitware.com/help/user/markdown.md
.. _`Special GitLab References`: https://gitlab.kitware.com/help/user/markdown.md#special-gitlab-references

Comment Trailing Lines
----------------------

Zero or more *trailing* lines in the last section of a comment may appear
with the form ``Key: Value``.  The first such line should be separated
from a preceding `comment body`_ by a blank line.  Any key-value pair(s)
may be specified for human reference.  A few specific keys have meaning to
``@kwrobot`` as follows.

Comment Trailer Votes
^^^^^^^^^^^^^^^^^^^^^

Among the `comment trailing lines`_ one may cast a vote using one of the
following pairs followed by nothing but whitespace before the end of the line:

* ``Rejected-by: me`` indicates "the change is not ready for integration".
* ``Acked-by: me`` indicates "I like the change".
  This adds an ``Acked-by:`` trailer to the `merge`_ commit message.
* ``Reviewed-by: me`` indicates "the change is ready for integration".
  This adds a ``Reviewed-by:`` trailer to the `merge`_ commit message.
* ``Tested-by: me`` indicates "I have tested the change and verified it works".
  This adds a ``Tested-by:`` trailer to the `merge`_ commit message.

Each ``me`` reference may instead be an ``@username`` reference or a full
``Real Name <user@domain>`` reference to credit someone else for performing
the review.  References to ``me`` and ``@username`` will automatically be
transformed into a real name and email address according to the user's
GitLab account profile.

Comment Trailer Commands
^^^^^^^^^^^^^^^^^^^^^^^^

Among the `comment trailing lines`_ authorized users may issue special
commands to ``@kwrobot`` using the form ``Do: ...``:

183
* ``Do: check`` explicitly re-runs the robot `Automatic Check`_.
184
* ``Do: reformat`` rewrites the MR topic for `Automatic Format`_.
185
186
187
188
189
190
191
* ``Do: test`` submits the MR for `Topic Testing`_.
* ``Do: stage`` submits the MR for `Integration Testing`_.
* ``Do: merge`` submits the MR for `Merge`_.

See the corresponding sections for details on permissions and options
for each command.

192
193
194
195
196
197
198
199
200
201
Commit Messages
---------------

Part of the human review is to check that each commit message is appropriate.
The first line of the message should begin with one or two words indicating the
area the commit applies to, followed by a colon and then a brief summary.
Committers should aim to keep this first line short. Any subsequent lines
should be separated from the first by a blank line and provide relevant, useful
information.

202
203
204
Area Prefix on Commit Messages
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

205
206
207
208
209
210
211
212
213
214
215
216
217
218
The appropriateness of the initial word describing the area the commit applies
to is not something the automatic robot review can judge, so it is up to the
human reviewer to confirm that the area is specified and that it is
appropriate. Good area words include the module name the commit is primarily
fixing, the main C++ source file being edited, ``Help`` for generic
documentation changes or a feature or functionality theme the changes apply to
(e.g. ``server`` or ``Autogen``). Examples of suitable first lines of a commit
message include:

* ``Help: Fix example in cmake-buildsystem(7) manual``
* ``FindBoost: Add support for 1.64``
* ``Autogen: Extended mocInclude tests``
* ``cmLocalGenerator: Explain standard flag selection logic in comments``

219
220
221
Referencing Issues in Commit Messages
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
If the commit fixes a particular reported issue, this information should
ideally also be part of the commit message. The recommended way to do this is
to place a line at the end of the message in the form ``Fixes: #xxxxx`` where
``xxxxx`` is the GitLab issue number and to separate it from the rest of the
text by a blank line. For example::

  Help: Fix FooBar example robustness issue

  FooBar supports option X, but the example provided
  would not work if Y was also specified.

  Fixes: #12345

GitLab will automatically create relevant links to the merge request and will
close the issue when the commit is merged into master. GitLab understands a few
other synonyms for ``Fixes`` and allows much more flexible forms than the
above, but committers should aim for this format for consistency. Note that
such details can alternatively be specified in the merge request description.

241
242
243
244
Referencing Commits in Commit Messages
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The preferred form for references to other commits is
245
``commit <shorthash> (<subject>, <date>)``, where:
246

247
248
* ``<shorthash>``:
  The abbreviated hash of the commit.
249
250
251
252
253
254
255
256

* ``<subject>``:
  The first line of the commit message.

* ``<date>``:
  The author date of the commit, in its original time zone, formatted as
  ``CCYY-MM-DD``.  ``git-log(1)`` shows the original time zone by default.

257
258
This may be generated with ``git show -s --pretty=reference <commit>`` with
Git 2.25 and newer. Older versions of Git can generate the same format via
259
260
261
262
263
``git show -s --date=short --pretty="format:%h (%s, %ad)" <commit>``.

If the commit is a fix for the mentioned commit, consider using a ``Fixes:``
trailer in the commit message with the specified format. This trailer should
not be word-wrapped. Note that if there is also an issue for what is being
264
fixed, it is preferable to link to the issue instead.
265
266
267

If relevant, add the first release tag of CMake containing the commit after
the ``<date>``, i.e., ``commit <shorthash> (<subject>, <date>, <tag>)``.
268
Or, use the output of ``git describe --contains <commit>`` as the ``<tag>``.
269

270
271
Alternatively, the full commit ``<hash>`` may be used.

272
273
274
Revising Commit Messages
^^^^^^^^^^^^^^^^^^^^^^^^

275
276
277
278
279
Reviewers are encouraged to ask the committer to amend commit messages to
follow these guidelines, but prefer to focus on the changes themselves as a
first priority. Maintainers will also make a check of commit messages before
merging.

280
281
282
Topic Testing
=============

283
284
285
286
287
CMake uses `GitLab CI`_ to test merge requests, configured by the top-level
``.gitlab-ci.yml`` file.  Results may be seen both on the merge request's
pipeline page and on the `CMake CDash Page`_.  Filtered CDash results
showing just the pipeline's jobs can be reached by selecting the ``cdash``
job in the ``External`` stage of the pipeline.
288

289
290
Lint and documentation build jobs run automatically after every push.
Heavier jobs require a manual trigger to run:
291

292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
* Merge request authors may visit their merge request's pipeline and click the
  "Play" button on one or more jobs manually.  If the merge request has the
  "Allow commits from members who can merge to the target branch" check box
  enabled, CMake maintainers may use the "Play" button too.

* `CMake GitLab Project Developers`_ may trigger CI on a merge request by
  adding a comment with a command among the `comment trailing lines`_::

    Do: test

  ``@kwrobot`` will add an award emoji to the comment to indicate that it
  was processed and also trigger all manual jobs in the merge request's
  pipeline.

  The ``Do: test`` command accepts the following arguments:

  * ``--named <regex>``, ``-n <regex>``: Trigger jobs matching ``<regex>``
    anywhere in their name.  Job names may be seen on the merge request's
    pipeline page.
311
312
313
314
315
316
317
318
319
320
321
322
323
  * ``--stage <stage>``, ``-s <stage>``: Only affect jobs in a given stage.
    Stage names may be seen on the merge request's pipeline page.  Note that
    the names are determined by what is in the ``.gitlab-ci.yml`` file and may
    be capitalized in the web page, so lowercasing the webpage's display name
    for stages may be required.
  * ``--action <action>``, ``-a <action>``: The action to perform on the jobs.
    Possible actions:

    * ``manual`` (the default): Start jobs awaiting manual interaction.
    * ``unsuccessful``: Start or restart jobs which have not completed
      successfully.
    * ``failed``: Restart jobs which have completed, but without success.
    * ``completed``: Restart all completed jobs.
324
325
326
327
328

If the merge request topic branch is updated by a push, a new manual trigger
using one of the above methods is needed to start CI again.

.. _`GitLab CI`: https://gitlab.kitware.com/help/ci/README.md
329
330
331
332
333
334
335
.. _`CMake CDash Page`: https://open.cdash.org/index.php?project=CMake

Integration Testing
===================

The above `topic testing`_ tests the MR topic independent of other
merge requests and on only a few key platforms and configurations.
336
337
338
339
340
341
342
343
The `CMake Testing Process`_ also has a large number of machines
provided by Kitware and generous volunteers that cover nearly all
supported platforms, generators, and configurations.  In order to
avoid overwhelming these resources, they do not test every MR
individually.  Instead, these machines follow an *integration branch*,
run tests on a nightly basis (or continuously during the day), and
post to the `CMake CDash Page`_.  Some follow ``master``.  Most follow
a special integration branch, the *topic stage*.
344
345
346
347
348
349
350

The topic stage is a special branch maintained by the "Kitware Robot"
(``@kwrobot``).  It consists of the head of the MR target integration
branch (e.g. ``master``) branch followed by a sequence of merges each
integrating changes from an open MR that has been staged for integration
testing.  Each time the target integration branch is updated the stage
is rebuilt automatically by merging the staged MR topics again.
351
352
353
354
355
356
357
358
The branch is stored in the upstream repository by special refs:

* ``refs/stage/master/head``: The current topic stage branch.
  This is used by continuous builds that report to CDash.
* ``refs/stage/master/nightly/latest``: Topic stage as of 1am UTC each night.
  This is used by most nightly builds that report to CDash.
* ``refs/stage/master/nightly/<yyyy>/<mm>/<dd>``: Topic stage as of 1am UTC
  on the date specified. This is used for historical reference.
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378

`CMake GitLab Project Developers`_ may stage a MR for integration testing
by adding a comment with a command among the `comment trailing lines`_::

  Do: stage

``@kwrobot`` will add an award emoji to the comment to indicate that it
was processed and also attempt to add the MR topic branch to the topic
stage.  If the MR cannot be added (e.g. due to conflicts) the robot will
post a comment explaining what went wrong.

Once a MR has been added to the topic stage it will remain on the stage
until one of the following occurs:

* The MR topic branch is updated by a push.

* The MR target integration branch (e.g. ``master``) branch is updated
  and the MR cannot be merged into the topic stage again due to conflicts.

* A developer or the submitter posts an explicit ``Do: unstage`` command.
379
380
381
382
  This is useful to remove a MR from the topic stage when one is not ready
  to push an update to the MR topic branch.  It is unnecessary to explicitly
  unstage just before or after pushing an update because the push will cause
  the MR to be unstaged automatically.
383
384
385
386
387
388
389
390

* The MR is closed.

* The MR is merged.

Once a MR has been removed from the topic stage a new ``Do: stage``
command is needed to stage it again.

391
392
.. _`CMake Testing Process`: testing.rst

393
394
395
Resolve
=======

396
397
398
399
400
401
402
403
404
The workflow used by the CMake project supports a number of different
ways in which a MR can be moved to a resolved state.  In addition to
the conventional practices of merging or closing a MR without merging it,
a MR can also be moved to a quasi-resolved state pending some action.
This may involve moving discussion to an issue or it may be the result of
an extended period of inactivity.  These quasi-resolved states are used
to help manage the relatively large number of MRs the project receives
and are not an indication of the changes being rejected.  The following
sections explain the different resolutions a MR may be given.
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443

Merge
-----

Once review has concluded that the MR topic is ready for integration,
`CMake GitLab Project Masters`_ may merge the topic by adding a comment
with a command among the `comment trailing lines`_::

  Do: merge

``@kwrobot`` will add an award emoji to the comment to indicate that it
was processed and also attempt to merge the MR topic branch to the MR
target integration branch (e.g. ``master``).  If the MR cannot be merged
(e.g. due to conflicts) the robot will post a comment explaining what
went wrong.  If the MR is merged the robot will also remove the source
branch from the user's fork if the corresponding MR option was checked.

The robot automatically constructs a merge commit message of the following
form::

  Merge topic 'mr-topic-branch-name'

  00000000 commit message subject line (one line per commit)

  Acked-by: Kitware Robot <kwrobot@kitware.com>
  Merge-request: !0000

Mention of the commit short sha1s and MR number helps GitLab link the
commits back to the merge request and indicates when they were merged.
The ``Acked-by:`` trailer shown indicates that `Robot Review`_ passed.
Additional ``Acked-by:``, ``Reviewed-by:``, and similar trailers may be
collected from `Human Review`_ comments that have been made since the
last time the MR topic branch was updated with a push.

The ``Do: merge`` command accepts the following arguments:

* ``-t <topic>``: substitute ``<topic>`` for the name of the MR topic
  branch in the constructed merge commit message.

444
Additionally, ``Do: merge`` extracts configuration from trailing lines
445
446
in the MR description (the following have no effect if used in a MR
comment instead):
447

448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
* ``Backport: release[:<commit-ish>]``: merge the topic branch into
  the ``release`` branch to backport the change.  This is allowed
  only if the topic branch is based on a commit in ``release`` already.
  If only part of the topic branch should be backported, specify it as
  ``:<commit-ish>``.  The ``<commit-ish>`` may use `git rev-parse`_
  syntax to reference commits relative to the topic ``HEAD``.
  See additional `backport instructions`_ for details.
  For example:

  ``Backport: release``
    Merge the topic branch head into both ``release`` and ``master``.
  ``Backport: release:HEAD~1^2``
    Merge the topic branch head's parent's second parent commit into
    the ``release`` branch.  Merge the topic branch head to ``master``.

463
464
* ``Topic-rename: <topic>``: substitute ``<topic>`` for the name of
  the MR topic branch in the constructed merge commit message.
465
466
467
  It is also used in merge commits constructed by ``Do: stage``.
  The ``-t`` option to a ``Do: merge`` command overrides any topic
  rename set in the MR description.
468

469
470
.. _`CMake GitLab Project Masters`: https://gitlab.kitware.com/cmake/cmake/-/settings/members
.. _`backport instructions`: https://gitlab.kitware.com/utils/git-workflow/-/wikis/Backport-topics
471
.. _`git rev-parse`: https://git-scm.com/docs/git-rev-parse
472
473
474
475
476

Close
-----

If review has concluded that the MR should not be integrated then it
477
478
479
480
481
may be closed through GitLab.  This would normally be a final state
with no expectation that the MR would be re-opened in the future.
It is also used when a MR is being superseded by another separate one,
in which case a reference to the new MR should be added to the MR being
closed.
482
483
484
485
486
487

Expire
------

If progress on a MR has stalled for a while, it may be closed with a
``workflow:expired`` label and a comment indicating that the MR has
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
been closed due to inactivity (it may also be done where the MR is blocked
for an extended period by work in a different MR).  This is not an
indication that there is a problem with the MR's content, it is only a
practical measure to allow the reviewers to focus attention on MRs that
are actively being worked on.  As a guide, the average period of inactivity
before transitioning a MR to the expired state would be around 2 weeks,
but this may decrease to 1 week or less when there is a high number of
open merge requests.

Reviewers would usually provide a message similar to the following when
resolving a MR as expired::

  Closing for now. @<MR-author> please re-open when ready to continue work.

This is to make it clear to contributors that they are welcome to re-open
the expired MR when they are ready to return to working on it and moving
it forward.  In the meantime, the MR will appear as ``Closed`` in GitLab,
but it can be differentiated from permanently closed MRs by the presence
of the ``workflow:expired`` label.

**NOTE:** Please re-open *before* pushing an update to the MR topic branch
to ensure GitLab will still act on the association.  If changes are pushed
before re-opening the MR, the reviewer should initiate a ``Do: check`` to
force GitLab to act on the updates.

External Discussion
-------------------

In some situations, a series of comments on a MR may develop into a more
involved discussion, or it may become apparent that there are broader
discussions that need to take place before the MR can move forward in an
agreed direction.  Such discussions are better suited to GitLab issues
rather than in a MR because MRs may be superseded by a different MR, or
the set of changes may evolve to look quite different to the context in
which the discussions began.  When this occurs, reviewers may ask the
MR author to open an issue to discuss things there and they will transition
the MR to a resolved state with the label ``workflow:external-discussion``.
The MR will appear in GitLab as closed, but it can be differentiated from
permanently closed MRs by the presence of the ``workflow:external-discussion``
label.  Reviewers should leave a message clearly explaining the action
so that the MR author understands that the MR closure is temporary and
it is clear what actions need to happen next.  The following is an example
of such a message, but it will usually be necessary to tailor the message
to the individual situation::

  The desired behavior here looks to be more involved than first thought.
  Please open an issue so we can discuss the relevant details there.
  Once the path forward is clear, we can re-open this MR and continue work.

When the discussion in the associated issue runs its course and the way
forward is clear, the MR can be re-opened again and the
``workflow:external-discussion`` label removed.  Reviewers should ensure
that the issue created contains a reference to the MR so that GitLab
provides a cross-reference to link the two.