1CMake Review Process 2******************** 3 4The following documents the process for reviewing and integrating changes. 5See `CONTRIBUTING.rst`_ for instructions to contribute changes. 6See documentation on `CMake Development`_ for more information. 7 8.. _`CONTRIBUTING.rst`: ../../CONTRIBUTING.rst 9.. _`CMake Development`: README.rst 10 11.. contents:: The review process consists of the following steps: 12 13Merge Request 14============= 15 16A user initiates the review process for a change by pushing a *topic 17branch* to his or her own fork of the `CMake Repository`_ on GitLab and 18creating a *merge request* ("MR"). The new MR will appear on the 19`CMake Merge Requests Page`_. The rest of the review and integration 20process is managed by the merge request page for the change. 21 22During the review process, the MR submitter should address review comments 23or test failures by updating their local topic branch to fix their commits 24(e.g. via ``git commit --amend`` or ``git rebase -i``), and then issuing a 25(force-)push of the topic branch to their remote (e.g. ``git push --force``). 26This will automatically initiate a new round of review on the existing MR. 27 28We recommend that users enable the "Remove source branch when merge 29request is accepted" option when creating the MR or by editing it. 30This will cause the MR topic branch to be automatically removed from 31the user's fork during the `Merge`_ step. 32 33.. _`CMake Merge Requests Page`: https://gitlab.kitware.com/cmake/cmake/-/merge_requests 34.. _`CMake Repository`: https://gitlab.kitware.com/cmake/cmake 35 36Workflow Status 37--------------- 38 39`CMake GitLab Project Developers`_ may set one of the following labels 40in GitLab to track the state of a MR: 41 42* ``workflow:wip`` indicates that the MR needs additional updates from 43 the MR submitter before further review. Use this label after making 44 comments that require such updates. 45 46* ``workflow:in-review`` indicates that the MR awaits feedback from a 47 human reviewer or from `Topic Testing`_. Use this label after making 48 comments requesting such feedback. 49 50* ``workflow:nightly-testing`` indicates that the MR awaits results 51 of `Integration Testing`_. Use this label after making comments 52 requesting such staging. 53 54* ``workflow:expired`` indicates that the MR has been closed due 55 to a period of inactivity. See the `Expire`_ step. Use this label 56 after closing a MR for this reason. 57 58* ``workflow:external-discussion`` indicates that the MR has been closed 59 pending discussion elsewhere. See the `External Discussion`_ step. 60 Use this label after closing a MR for this reason. 61 62The workflow status labels are intended to be mutually exclusive, 63so please remove any existing workflow label when adding one. 64 65.. _`CMake GitLab Project Developers`: https://gitlab.kitware.com/cmake/cmake/-/settings/members 66 67Robot Review 68============ 69 70The "Kitware Robot" (``@kwrobot``) automatically performs basic checks on 71the commits proposed in a MR. If all is well the robot silently reports 72a successful "build" status to GitLab. Otherwise the robot posts a comment 73with its diagnostics. **A topic may not be merged until the automatic 74review succeeds.** 75 76Note that the MR submitter is expected to address the robot's comments by 77*rewriting* the commits named by the robot's diagnostics (e.g., via 78``git rebase -i``). This is because the robot checks each commit individually, 79not the topic as a whole. This is done in order to ensure that commits in the 80middle of a topic do not, for example, add a giant file which is then later 81removed in the topic. 82 83Automatic Check 84--------------- 85 86The automatic check is repeated whenever the topic branch is updated. 87One may explicitly request a re-check by adding a comment with the 88following command among the `comment trailing lines`_:: 89 90 Do: check 91 92``@kwrobot`` will add an award emoji to the comment to indicate that it 93was processed and also run its checks again. 94 95Automatic Format 96---------------- 97 98The automatic check will reject commits introducing source code not 99formatted according to ``clang-format``. One may ask the robot to 100automatically rewrite the MR topic branch with expected formatting 101by adding a comment with the following command among the 102`comment trailing lines`_:: 103 104 Do: reformat 105 106``@kwrobot`` will add an award emoji to the comment to indicate that it 107was processed and also rewrite the MR topic branch and force-push an 108updated version with every commit formatted as expected by the check. 109 110Human Review 111============ 112 113Anyone is welcome to review merge requests and make comments! 114 115Please make comments directly on the MR page Discussion and Changes tabs 116and not on individual commits. Comments on a commit may disappear 117from the MR page if the commit is rewritten in response. 118 119Reviewers may add comments providing feedback or to acknowledge their 120approval. Lines of specific forms will be extracted during the `merge`_ 121step and included as trailing lines of the generated merge commit message. 122Each review comment consists of up to two parts which must be specified 123in the following order: `comment body`_, then `comment trailing lines`_. 124Each part is optional, but they must be specified in this order. 125 126Comment Body 127------------ 128 129The body of a comment may be free-form `GitLab Flavored Markdown`_. 130See GitLab documentation on `Special GitLab References`_ to add links to 131things like issues, commits, or other merge requests (even across projects). 132 133Additionally, a line in the comment body may start with one of the 134following votes: 135 136* ``-1`` or ``:-1:`` indicates "the change is not ready for integration". 137 138* ``+1`` or ``:+1:`` indicates "I like the change". 139 This adds an ``Acked-by:`` trailer to the `merge`_ commit message. 140 141* ``+2`` indicates "the change is ready for integration". 142 This adds a ``Reviewed-by:`` trailer to the `merge`_ commit message. 143 144* ``+3`` indicates "I have tested the change and verified it works". 145 This adds a ``Tested-by:`` trailer to the `merge`_ commit message. 146 147.. _`GitLab Flavored Markdown`: https://gitlab.kitware.com/help/user/markdown.md 148.. _`Special GitLab References`: https://gitlab.kitware.com/help/user/markdown.md#special-gitlab-references 149 150Comment Trailing Lines 151---------------------- 152 153Zero or more *trailing* lines in the last section of a comment may appear 154with the form ``Key: Value``. The first such line should be separated 155from a preceding `comment body`_ by a blank line. Any key-value pair(s) 156may be specified for human reference. A few specific keys have meaning to 157``@kwrobot`` as follows. 158 159Comment Trailer Votes 160^^^^^^^^^^^^^^^^^^^^^ 161 162Among the `comment trailing lines`_ one may cast a vote using one of the 163following pairs followed by nothing but whitespace before the end of the line: 164 165* ``Rejected-by: me`` indicates "the change is not ready for integration". 166* ``Acked-by: me`` indicates "I like the change". 167 This adds an ``Acked-by:`` trailer to the `merge`_ commit message. 168* ``Reviewed-by: me`` indicates "the change is ready for integration". 169 This adds a ``Reviewed-by:`` trailer to the `merge`_ commit message. 170* ``Tested-by: me`` indicates "I have tested the change and verified it works". 171 This adds a ``Tested-by:`` trailer to the `merge`_ commit message. 172 173Each ``me`` reference may instead be an ``@username`` reference or a full 174``Real Name <user@domain>`` reference to credit someone else for performing 175the review. References to ``me`` and ``@username`` will automatically be 176transformed into a real name and email address according to the user's 177GitLab account profile. 178 179Comment Trailer Commands 180^^^^^^^^^^^^^^^^^^^^^^^^ 181 182Among the `comment trailing lines`_ authorized users may issue special 183commands to ``@kwrobot`` using the form ``Do: ...``: 184 185* ``Do: check`` explicitly re-runs the robot `Automatic Check`_. 186* ``Do: reformat`` rewrites the MR topic for `Automatic Format`_. 187* ``Do: test`` submits the MR for `Topic Testing`_. 188* ``Do: stage`` submits the MR for `Integration Testing`_. 189* ``Do: merge`` submits the MR for `Merge`_. 190 191See the corresponding sections for details on permissions and options 192for each command. 193 194Commit Messages 195--------------- 196 197Part of the human review is to check that each commit message is appropriate. 198The first line of the message should begin with one or two words indicating the 199area the commit applies to, followed by a colon and then a brief summary. 200Committers should aim to keep this first line short. Any subsequent lines 201should be separated from the first by a blank line and provide relevant, useful 202information. 203 204Area Prefix on Commit Messages 205^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 206 207The appropriateness of the initial word describing the area the commit applies 208to is not something the automatic robot review can judge, so it is up to the 209human reviewer to confirm that the area is specified and that it is 210appropriate. Good area words include the module name the commit is primarily 211fixing, the main C++ source file being edited, ``Help`` for generic 212documentation changes or a feature or functionality theme the changes apply to 213(e.g. ``server`` or ``Autogen``). Examples of suitable first lines of a commit 214message include: 215 216* ``Help: Fix example in cmake-buildsystem(7) manual`` 217* ``FindBoost: Add support for 1.64`` 218* ``Autogen: Extended mocInclude tests`` 219* ``cmLocalGenerator: Explain standard flag selection logic in comments`` 220 221Referencing Issues in Commit Messages 222^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 223 224If the commit fixes a particular reported issue, this information should 225ideally also be part of the commit message. The recommended way to do this is 226to place a line at the end of the message in the form ``Fixes: #xxxxx`` where 227``xxxxx`` is the GitLab issue number and to separate it from the rest of the 228text by a blank line. For example:: 229 230 Help: Fix FooBar example robustness issue 231 232 FooBar supports option X, but the example provided 233 would not work if Y was also specified. 234 235 Fixes: #12345 236 237GitLab will automatically create relevant links to the merge request and will 238close the issue when the commit is merged into master. GitLab understands a few 239other synonyms for ``Fixes`` and allows much more flexible forms than the 240above, but committers should aim for this format for consistency. Note that 241such details can alternatively be specified in the merge request description. 242 243Referencing Commits in Commit Messages 244^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 245 246The preferred form for references to other commits is 247``commit <shorthash> (<subject>, <date>)``, where: 248 249* ``<shorthash>``: 250 The abbreviated hash of the commit. 251 252* ``<subject>``: 253 The first line of the commit message. 254 255* ``<date>``: 256 The author date of the commit, in its original time zone, formatted as 257 ``CCYY-MM-DD``. ``git-log(1)`` shows the original time zone by default. 258 259This may be generated with ``git show -s --pretty=reference <commit>`` with 260Git 2.25 and newer. Older versions of Git can generate the same format via 261``git show -s --date=short --pretty="format:%h (%s, %ad)" <commit>``. 262 263If the commit is a fix for the mentioned commit, consider using a ``Fixes:`` 264trailer in the commit message with the specified format. This trailer should 265not be word-wrapped. Note that if there is also an issue for what is being 266fixed, it is preferable to link to the issue instead. 267 268If relevant, add the first release tag of CMake containing the commit after 269the ``<date>``, i.e., ``commit <shorthash> (<subject>, <date>, <tag>)``. 270Or, use the output of ``git describe --contains <commit>`` as the ``<tag>``. 271 272Alternatively, the full commit ``<hash>`` may be used. 273 274Revising Commit Messages 275^^^^^^^^^^^^^^^^^^^^^^^^ 276 277Reviewers are encouraged to ask the committer to amend commit messages to 278follow these guidelines, but prefer to focus on the changes themselves as a 279first priority. Maintainers will also make a check of commit messages before 280merging. 281 282Topic Testing 283============= 284 285CMake uses `GitLab CI`_ to test merge requests, configured by the top-level 286``.gitlab-ci.yml`` file. Results may be seen both on the merge request's 287pipeline page and on the `CMake CDash Page`_. Filtered CDash results 288showing just the pipeline's jobs can be reached by selecting the ``cdash`` 289job in the ``External`` stage of the pipeline. 290 291Lint and documentation build jobs run automatically after every push. 292Heavier jobs require a manual trigger to run: 293 294* Merge request authors may visit their merge request's pipeline and click the 295 "Play" button on one or more jobs manually. If the merge request has the 296 "Allow commits from members who can merge to the target branch" check box 297 enabled, CMake maintainers may use the "Play" button too. 298 299* `CMake GitLab Project Developers`_ may trigger CI on a merge request by 300 adding a comment with a command among the `comment trailing lines`_:: 301 302 Do: test 303 304 ``@kwrobot`` will add an award emoji to the comment to indicate that it 305 was processed and also trigger all manual jobs in the merge request's 306 pipeline. 307 308 The ``Do: test`` command accepts the following arguments: 309 310 * ``--named <regex>``, ``-n <regex>``: Trigger jobs matching ``<regex>`` 311 anywhere in their name. Job names may be seen on the merge request's 312 pipeline page. 313 * ``--stage <stage>``, ``-s <stage>``: Only affect jobs in a given stage. 314 Stage names may be seen on the merge request's pipeline page. Note that 315 the names are determined by what is in the ``.gitlab-ci.yml`` file and may 316 be capitalized in the web page, so lowercasing the webpage's display name 317 for stages may be required. 318 * ``--action <action>``, ``-a <action>``: The action to perform on the jobs. 319 Possible actions: 320 321 * ``manual`` (the default): Start jobs awaiting manual interaction. 322 * ``unsuccessful``: Start or restart jobs which have not completed 323 successfully. 324 * ``failed``: Restart jobs which have completed, but without success. 325 * ``completed``: Restart all completed jobs. 326 327 328In order to keep job names shorter and keep as much information visible on the 329GitLab web interface as possible, jobs have a short prefix which indicates 330what its main purpose is: 331 332 * ``b:`` jobs build CMake for the purposes of running the 333 test suite. 334 * ``l:`` jobs perform "linting" on the CMake source tree such as static 335 analysis. 336 * ``p:`` jobs perform preparatory tasks for use in other jobs. 337 * ``t:`` jobs perform testing of CMake. 338 * ``u:`` jobs upload other job results to permanent locations. 339 340If the merge request topic branch is updated by a push, a new manual trigger 341using one of the above methods is needed to start CI again. 342 343.. _`GitLab CI`: https://gitlab.kitware.com/help/ci/README.md 344.. _`CMake CDash Page`: https://open.cdash.org/index.php?project=CMake 345 346Integration Testing 347=================== 348 349The above `topic testing`_ tests the MR topic independent of other 350merge requests and on only a few key platforms and configurations. 351The `CMake Testing Process`_ also has a large number of machines 352provided by Kitware and generous volunteers that cover nearly all 353supported platforms, generators, and configurations. In order to 354avoid overwhelming these resources, they do not test every MR 355individually. Instead, these machines follow an *integration branch*, 356run tests on a nightly basis (or continuously during the day), and 357post to the `CMake CDash Page`_. Some follow ``master``. Most follow 358a special integration branch, the *topic stage*. 359 360The topic stage is a special branch maintained by the "Kitware Robot" 361(``@kwrobot``). It consists of the head of the MR target integration 362branch (e.g. ``master``) branch followed by a sequence of merges each 363integrating changes from an open MR that has been staged for integration 364testing. Each time the target integration branch is updated the stage 365is rebuilt automatically by merging the staged MR topics again. 366The branch is stored in the upstream repository by special refs: 367 368* ``refs/stage/master/head``: The current topic stage branch. 369 This is used by continuous builds that report to CDash. 370* ``refs/stage/master/nightly/latest``: Topic stage as of 1am UTC each night. 371 This is used by most nightly builds that report to CDash. 372* ``refs/stage/master/nightly/<yyyy>/<mm>/<dd>``: Topic stage as of 1am UTC 373 on the date specified. This is used for historical reference. 374 375`CMake GitLab Project Developers`_ may stage a MR for integration testing 376by adding a comment with a command among the `comment trailing lines`_:: 377 378 Do: stage 379 380``@kwrobot`` will add an award emoji to the comment to indicate that it 381was processed and also attempt to add the MR topic branch to the topic 382stage. If the MR cannot be added (e.g. due to conflicts) the robot will 383post a comment explaining what went wrong. 384 385Once a MR has been added to the topic stage it will remain on the stage 386until one of the following occurs: 387 388* The MR topic branch is updated by a push. 389 390* The MR target integration branch (e.g. ``master``) branch is updated 391 and the MR cannot be merged into the topic stage again due to conflicts. 392 393* A developer or the submitter posts an explicit ``Do: unstage`` command. 394 This is useful to remove a MR from the topic stage when one is not ready 395 to push an update to the MR topic branch. It is unnecessary to explicitly 396 unstage just before or after pushing an update because the push will cause 397 the MR to be unstaged automatically. 398 399* The MR is closed. 400 401* The MR is merged. 402 403Once a MR has been removed from the topic stage a new ``Do: stage`` 404command is needed to stage it again. 405 406.. _`CMake Testing Process`: testing.rst 407 408Resolve 409======= 410 411The workflow used by the CMake project supports a number of different 412ways in which a MR can be moved to a resolved state. In addition to 413the conventional practices of merging or closing a MR without merging it, 414a MR can also be moved to a quasi-resolved state pending some action. 415This may involve moving discussion to an issue or it may be the result of 416an extended period of inactivity. These quasi-resolved states are used 417to help manage the relatively large number of MRs the project receives 418and are not an indication of the changes being rejected. The following 419sections explain the different resolutions a MR may be given. 420 421Merge 422----- 423 424Once review has concluded that the MR topic is ready for integration, 425`CMake GitLab Project Masters`_ may merge the topic by adding a comment 426with a command among the `comment trailing lines`_:: 427 428 Do: merge 429 430``@kwrobot`` will add an award emoji to the comment to indicate that it 431was processed and also attempt to merge the MR topic branch to the MR 432target integration branch (e.g. ``master``). If the MR cannot be merged 433(e.g. due to conflicts) the robot will post a comment explaining what 434went wrong. If the MR is merged the robot will also remove the source 435branch from the user's fork if the corresponding MR option was checked. 436 437The robot automatically constructs a merge commit message of the following 438form:: 439 440 Merge topic 'mr-topic-branch-name' 441 442 00000000 commit message subject line (one line per commit) 443 444 Acked-by: Kitware Robot <kwrobot@kitware.com> 445 Merge-request: !0000 446 447Mention of the commit short sha1s and MR number helps GitLab link the 448commits back to the merge request and indicates when they were merged. 449The ``Acked-by:`` trailer shown indicates that `Robot Review`_ passed. 450Additional ``Acked-by:``, ``Reviewed-by:``, and similar trailers may be 451collected from `Human Review`_ comments that have been made since the 452last time the MR topic branch was updated with a push. 453 454The ``Do: merge`` command accepts the following arguments: 455 456* ``-t <topic>``: substitute ``<topic>`` for the name of the MR topic 457 branch in the constructed merge commit message. 458 459Additionally, ``Do: merge`` extracts configuration from trailing lines 460in the MR description (the following have no effect if used in a MR 461comment instead): 462 463* ``Backport: release[:<commit-ish>]``: merge the topic branch into 464 the ``release`` branch to backport the change. This is allowed 465 only if the topic branch is based on a commit in ``release`` already. 466 If only part of the topic branch should be backported, specify it as 467 ``:<commit-ish>``. The ``<commit-ish>`` may use `git rev-parse`_ 468 syntax to reference commits relative to the topic ``HEAD``. 469 See additional `backport instructions`_ for details. 470 For example: 471 472 ``Backport: release`` 473 Merge the topic branch head into both ``release`` and ``master``. 474 ``Backport: release:HEAD~1^2`` 475 Merge the topic branch head's parent's second parent commit into 476 the ``release`` branch. Merge the topic branch head to ``master``. 477 478* ``Topic-rename: <topic>``: substitute ``<topic>`` for the name of 479 the MR topic branch in the constructed merge commit message. 480 It is also used in merge commits constructed by ``Do: stage``. 481 The ``-t`` option to a ``Do: merge`` command overrides any topic 482 rename set in the MR description. 483 484.. _`CMake GitLab Project Masters`: https://gitlab.kitware.com/cmake/cmake/-/settings/members 485.. _`backport instructions`: https://gitlab.kitware.com/utils/git-workflow/-/wikis/Backport-topics 486.. _`git rev-parse`: https://git-scm.com/docs/git-rev-parse 487 488Close 489----- 490 491If review has concluded that the MR should not be integrated then it 492may be closed through GitLab. This would normally be a final state 493with no expectation that the MR would be re-opened in the future. 494It is also used when a MR is being superseded by another separate one, 495in which case a reference to the new MR should be added to the MR being 496closed. 497 498Expire 499------ 500 501If progress on a MR has stalled for a while, it may be closed with a 502``workflow:expired`` label and a comment indicating that the MR has 503been closed due to inactivity (it may also be done where the MR is blocked 504for an extended period by work in a different MR). This is not an 505indication that there is a problem with the MR's content, it is only a 506practical measure to allow the reviewers to focus attention on MRs that 507are actively being worked on. As a guide, the average period of inactivity 508before transitioning a MR to the expired state would be around 2 weeks, 509but this may decrease to 1 week or less when there is a high number of 510open merge requests. 511 512Reviewers would usually provide a message similar to the following when 513resolving a MR as expired:: 514 515 Closing for now. @<MR-author> please re-open when ready to continue work. 516 517This is to make it clear to contributors that they are welcome to re-open 518the expired MR when they are ready to return to working on it and moving 519it forward. In the meantime, the MR will appear as ``Closed`` in GitLab, 520but it can be differentiated from permanently closed MRs by the presence 521of the ``workflow:expired`` label. 522 523**NOTE:** Please re-open *before* pushing an update to the MR topic branch 524to ensure GitLab will still act on the association. If changes are pushed 525before re-opening the MR, the reviewer should initiate a ``Do: check`` to 526force GitLab to act on the updates. 527 528External Discussion 529------------------- 530 531In some situations, a series of comments on a MR may develop into a more 532involved discussion, or it may become apparent that there are broader 533discussions that need to take place before the MR can move forward in an 534agreed direction. Such discussions are better suited to GitLab issues 535rather than in a MR because MRs may be superseded by a different MR, or 536the set of changes may evolve to look quite different to the context in 537which the discussions began. When this occurs, reviewers may ask the 538MR author to open an issue to discuss things there and they will transition 539the MR to a resolved state with the label ``workflow:external-discussion``. 540The MR will appear in GitLab as closed, but it can be differentiated from 541permanently closed MRs by the presence of the ``workflow:external-discussion`` 542label. Reviewers should leave a message clearly explaining the action 543so that the MR author understands that the MR closure is temporary and 544it is clear what actions need to happen next. The following is an example 545of such a message, but it will usually be necessary to tailor the message 546to the individual situation:: 547 548 The desired behavior here looks to be more involved than first thought. 549 Please open an issue so we can discuss the relevant details there. 550 Once the path forward is clear, we can re-open this MR and continue work. 551 552When the discussion in the associated issue runs its course and the way 553forward is clear, the MR can be re-opened again and the 554``workflow:external-discussion`` label removed. Reviewers should ensure 555that the issue created contains a reference to the MR so that GitLab 556provides a cross-reference to link the two. 557