docs/submittingpatches.html

   1 <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
   2 <html lang="en">
   3 <head>
   4   <meta http-equiv="content-type" content="text/html; charset=utf-8">
   5   <title>Submitting Patches</title>
   6   <link rel="stylesheet" type="text/css" href="mesa.css">
   7 </head>
   8 <body>
   9
  10 <div class="header">
  11   The Mesa 3D Graphics Library
  12 </div>
  13
  14 <iframe src="contents.html"></iframe>
  15 <div class="content">
  16
  17 <h1>Submitting Patches</h1>
  18
  19
  20 <ul>
  21 <li><a href="#guidelines">Basic guidelines</a>
  22 <li><a href="#formatting">Patch formatting</a>
  23 <li><a href="#testing">Testing Patches</a>
  24 <li><a href="#submit">Submitting Patches</a>
  25 <li><a href="#reviewing">Reviewing Patches</a>
  26 <li><a href="#nominations">Nominating a commit for a stable branch</a>
  27 <li><a href="#criteria">Criteria for accepting patches to the stable branch</a>
  28 <li><a href="#backports">Sending backports for the stable branch</a>
  29 <li><a href="#gittips">Git tips</a>
  30 </ul>
  31
  32 <h2 id="guidelines">Basic guidelines</h2>
  33
  34 <ul>
  35 <li>Patches should not mix code changes with code formatting changes (except,
  36 perhaps, in very trivial cases.)
  37 <li>Code patches should follow Mesa
  38 <a href="codingstyle.html" target="_parent">coding conventions</a>.
  39 <li>Whenever possible, patches should only affect individual Mesa/Gallium
  40 components.
  41 <li>Patches should never introduce build breaks and should be bisectable (see
  42 <code>git bisect</code>.)
  43 <li>Patches should be properly <a href="#formatting">formatted</a>.
  44 <li>Patches should be sufficiently <a href="#testing">tested</a> before submitting.
  45 <li>Patches should be <a href="#submit">submitted</a>
  46 to <a href="#mailing">mesa-dev</a> or with
  47 a <a href="#merge-request">merge request</a>
  48 for <a href="#reviewing">review</a>.
  49
  50 </ul>
  51
  52 <h2 id="formatting">Patch formatting</h2>
  53
  54 <ul>
  55 <li>Lines should be limited to 75 characters or less so that git logs
  56 displayed in 80-column terminals avoid line wrapping.  Note that git
  57 log uses 4 spaces of indentation (4 + 75 &lt; 80).
  58 <li>The first line should be a short, concise summary of the change prefixed
  59 with a module name.  Examples:
  60 <pre>
  61     mesa: Add support for querying GL_VERTEX_ATTRIB_ARRAY_LONG
  62
  63     gallium: add PIPE_CAP_DEVICE_RESET_STATUS_QUERY
  64
  65     i965: Fix missing type in local variable declaration.
  66 </pre>
  67 <li>Subsequent patch comments should describe the change in more detail,
  68 if needed.  For example:
  69 <pre>
  70     i965: Remove end-of-thread SEND alignment code.
  71
  72     This was present in Eric's initial implementation of the compaction code
  73     for Sandybridge (commit 077d01b6). There is no documentation saying this
  74     is necessary, and removing it causes no regressions in piglit on any
  75     platform.
  76 </pre>
  77 <li>A "Signed-off-by:" line is not required, but not discouraged either.
  78 <li>If a patch addresses an issue in gitlab, use the Closes: tag
  79 For example:
  80 <pre>
  81     Closes: https://gitlab.freedesktop.org/mesa/mesa/issues/1
  82 </pre>
  83 <p>Prefer the full url to just <pre>Closes: #1</pre>, since the url makes it
  84 easier to get to the bug page from <pre>git log</pre></p>
  85 <b>Do not use the Fixes: tag for this!</b> Mesa already uses Fixes for something else.
  86
  87 <li>If a patch addresses a issue introduced with earlier commit, that should be
  88 noted in the patch comment.  For example:
  89 <pre>
  90    Fixes: d7b3707c612 "util/disk_cache: use stat() to check if entry is a directory"
  91 </pre>
  92 <li>You can produce those fixes lines by running
  93 <pre>git config --global alias.fixes "show -s --pretty='format:Fixes: %h (\"%s\")'"</pre>
  94 once and then using <pre>git fixes &lt;sha1&gt;</pre>
  95 <li>If there have been several revisions to a patch during the review
  96 process, they should be noted such as in this example:
  97 <pre>
  98     st/mesa: add ARB_texture_stencil8 support (v4)
  99
 100     if we support stencil texturing, enable texture_stencil8
 101     there is no requirement to support native S8 for this,
 102     the texture can be converted to x24s8 fine.
 103
 104     v2: fold fixes from Marek in:
 105        a) put S8 last in the list
 106        b) fix renderable to always test for d/s renderable
 107         fixup the texture case to use a stencil only format
 108         for picking the format for the texture view.
 109     v3: hit fallback for getteximage
 110     v4: put s8 back in front, it shouldn't get picked now (Ilia)
 111 </pre>
 112 <li>If someone tested your patch, document it with a line like this:
 113 <pre>
 114     Tested-by: Joe Hacker &lt;jhacker@foo.com&gt;
 115 </pre>
 116 <li>If the patch was reviewed (usually the case) or acked by someone,
 117 that should be documented with:
 118 <pre>
 119     Reviewed-by: Joe Hacker &lt;jhacker@foo.com&gt;
 120     Acked-by: Joe Hacker &lt;jhacker@foo.com&gt;
 121 </pre>
 122 <li>If sending later revision of a patch, add all the tags - ack, r-b,
 123 Cc: mesa-stable and/or other. This provides reviewers with quick feedback if the
 124 patch has already been reviewed.
 125 </ul>
 126
 127
 128
 129 <h2 id="testing">Testing Patches</h2>
 130
 131 <p>
 132 It should go without saying that patches must be tested.  In general,
 133 do whatever testing is prudent.
 134 </p>
 135
 136 <p>
 137 You should always run the Mesa test suite before submitting patches.
 138 The test suite can be run using the 'meson test' command. All tests
 139 must pass before patches will be accepted, this may mean you have
 140 to update the tests themselves.
 141 </p>
 142
 143 <p>
 144 Whenever possible and applicable, test the patch with
 145 <a href="https://piglit.freedesktop.org">Piglit</a> and/or
 146 <a href="https://android.googlesource.com/platform/external/deqp/">dEQP</a>
 147 to check for regressions.
 148 </p>
 149
 150 <p>
 151 As mentioned at the begining, patches should be bisectable.
 152 A good way to test this is to make use of the `git rebase` command,
 153 to run your tests on each commit. Assuming your branch is based off
 154 <code>origin/master</code>, you can run:
 155 </p>
 156 <pre>
 157 $ git rebase --interactive --exec "meson test -C build/" origin/master
 158 </pre>
 159 <p>
 160 replacing <code>"meson test"</code> with whatever other test you want to
 161 run.
 162 </p>
 163
 164
 165 <h2 id="submit">Submitting Patches</h2>
 166
 167 <p>
 168 Patches may be submitted to the Mesa project by
 169 <a href="#mailing">email</a> or with a
 170 GitLab <a href="#merge-request">merge request</a>. To prevent
 171 duplicate code review, only use one method to submit your changes.
 172 </p>
 173
 174 <h3 id="mailing">Mailing Patches</h3>
 175
 176 <p>
 177 Patches may be sent to the mesa-dev mailing list for review:
 178 <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">
 179 mesa-dev@lists.freedesktop.org</a>.
 180 When submitting a patch make sure to use
 181 <a href="https://git-scm.com/docs/git-send-email">git send-email</a>
 182 rather than attaching patches to emails. Sending patches as
 183 attachments prevents people from being able to provide in-line review
 184 comments.
 185 </p>
 186
 187 <p>
 188 When submitting follow-up patches you can use --in-reply-to to make v2, v3,
 189 etc patches show up as replies to the originals. This usually works well
 190 when you're sending out updates to individual patches (as opposed to
 191 re-sending the whole series). Using --in-reply-to makes
 192 it harder for reviewers to accidentally review old patches.
 193 </p>
 194
 195 <p>
 196 When submitting follow-up patches you should also login to
 197 <a href="https://patchwork.freedesktop.org">patchwork</a> and change the
 198 state of your old patches to Superseded.
 199 </p>
 200
 201 <p>
 202 Some companies' mail server automatically append a legal disclaimer,
 203 usually containing something along the lines of "The information in this
 204 email is confidential" and "distribution is strictly prohibited".
 205 </p>
 206 <p>
 207 These legal notices prevent us from being able to accept your patch,
 208 rendering the whole process pointless. Please make sure these are
 209 disabled before sending your patches. (Note that you may need to contact
 210 your email administrator for this.)
 211 </p>
 212
 213 <h3 id="merge-request">GitLab Merge Requests</h3>
 214
 215 <p>
 216   <a href="https://gitlab.freedesktop.org/mesa/mesa">GitLab</a> Merge
 217   Requests (MR) can also be used to submit patches for Mesa.
 218 </p>
 219
 220 <p>
 221   If the MR may have interest for most of the Mesa community, you can
 222   send an email to the mesa-dev email list including a link to the MR.
 223   Don't send the patch to mesa-dev, just the MR link.
 224 </p>
 225 <p>
 226   Add labels to your MR to help reviewers find it. For example:
 227 </p>
 228 <ul>
 229   <li>Mesa changes affecting all drivers: mesa
 230   <li>Hardware vendor specific code: amd, intel, nvidia, ...
 231   <li>Driver specific code: anvil, freedreno, i965, iris, radeonsi,
 232     radv, vc4, ...
 233   <li>Other tag examples: gallium, util
 234 </ul>
 235 <p>
 236   Tick the following when creating the MR. It allows developers to
 237   rebase your work on top of master.
 238 </p>
 239 <pre>Allow commits from members who can merge to the target branch</pre>
 240 <p>
 241   If you revise your patches based on code review and push an update
 242   to your branch, you should maintain a <strong>clean</strong> history
 243   in your patches. There should not be "fixup" patches in the history.
 244   The series should be buildable and functional after every commit
 245   whenever you push the branch.
 246 </p>
 247 <p>
 248   It is your responsibility to keep the MR alive and making progress,
 249   as there are no guarantees that a Mesa dev will independently take
 250   interest in it.
 251 </p>
 252 <p>
 253   Some other notes:
 254 </p>
 255 <ul>
 256   <li>Make changes and update your branch based on feedback
 257   <li>After an update, for the feedback you handled, close the
 258     feedback discussion with the "Resolve Discussion" button. This way
 259     the reviewers know which feedback got handled and which didn't.
 260   <li>Old, stale MR may be closed, but you can reopen it if you
 261     still want to pursue the changes
 262   <li>You should periodically check to see if your MR needs to be
 263     rebased
 264   <li>Make sure your MR is closed if your patches get pushed outside
 265     of GitLab
 266   <li>Please send MRs from a personal fork rather than from the main
 267     Mesa repository, as it clutters it unnecessarily.
 268 </ul>
 269
 270 <h2 id="reviewing">Reviewing Patches</h2>
 271
 272 <p>
 273   To participate in code review, you should monitor the
 274   <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">
 275   mesa-dev</a> email list and the GitLab
 276   Mesa <a href="https://gitlab.freedesktop.org/mesa/mesa/merge_requests">Merge
 277   Requests</a> page.
 278 </p>
 279
 280 <p>
 281 When you've reviewed a patch on the mailing list, please be unambiguous
 282 about your review.  That is, state either
 283 </p>
 284 <pre>
 285     Reviewed-by: Joe Hacker &lt;jhacker@foo.com&gt;
 286 </pre>
 287 or
 288 <pre>
 289     Acked-by: Joe Hacker &lt;jhacker@foo.com&gt;
 290 </pre>
 291 <p>
 292 Rather than saying just "LGTM" or "Seems OK".
 293 </p>
 294
 295 <p>
 296 If small changes are suggested, it's OK to say something like:
 297 </p>
 298 <pre>
 299    With the above fixes, Reviewed-by: Joe Hacker &lt;jhacker@foo.com&gt;
 300 </pre>
 301 <p>
 302 which tells the patch author that the patch can be committed, as long
 303 as the issues are resolved first.
 304 </p>
 305
 306 <p>
 307 These Reviewed-by, Acked-by, and Tested-by tags should also be amended
 308 into commits in a MR before it is merged.
 309 </p>
 310
 311 <p>
 312 When providing a Reviewed-by, Acked-by, or Tested-by tag in a gitlab MR,
 313 enclose the tag in backticks:
 314 </p>
 315 <pre>
 316   `Reviewed-by: Joe Hacker &lt;jhacker@example.com&gt;`</pre>
 317 <p>
 318 This is the markdown format for literal, and will prevent gitlab from hiding
 319 the &lt; and &gt; symbols.
 320 </p>
 321
 322 <p>
 323 Review by non-experts is encouraged.  Understanding how someone else
 324 goes about solving a problem is a great way to learn your way around
 325 the project.  The submitter is expected to evaluate whether they have
 326 an appropriate amount of review feedback from people who also
 327 understand the code before merging their patches.
 328 </p>
 329
 330 <h2 id="nominations">Nominating a commit for a stable branch</h2>
 331
 332 <p>
 333 There are three ways to nominate a patch for inclusion in the stable branch and
 334 release.
 335 </p>
 336 <ul>
 337 <li> By adding the Cc: mesa-stable@ tag as described below.
 338 <li> Sending the commit ID (as seen in master branch) to the mesa-stable@ mailing list.
 339 <li> Forwarding the patch from the mesa-dev@ mailing list.
 340 </li>
 341 </ul>
 342 <p>
 343 Note: resending patch identical to one on mesa-dev@ or one that differs only
 344 by the extra mesa-stable@ tag is <strong>not</strong> recommended.
 345 </p>
 346 <p>
 347 If you are not the author of the original patch, please Cc: them in your
 348 nomination request.
 349 </p>
 350
 351 <p>
 352 The current patch status can be observed in the <a href="releasing.html#stagingbranch">staging branch</a>.
 353 </p>
 354
 355 <h3 id="thetag">The stable tag</h3>
 356
 357 <p>
 358 If you want a commit to be applied to a stable branch,
 359 you should add an appropriate note to the commit message.
 360 </p>
 361
 362 <p>
 363 Here are some examples of such a note:
 364 </p>
 365 <pre>
 366 CC: &lt;mesa-stable@lists.freedesktop.org&gt;
 367 </pre>
 368
 369 Simply adding the CC to the mesa-stable list address is adequate to nominate
 370 the commit for all the active stable branches. If the commit is not applicable
 371 for said branch the stable-release manager will reply stating so.
 372
 373 This "CC" syntax for patch nomination will cause patches to automatically be
 374 copied to the mesa-stable@ mailing list when you use "git send-email" to send
 375 patches to the mesa-dev@ mailing list. If you prefer using --suppress-cc that
 376 won't have any negative effect on the patch nomination.
 377
 378 <p>
 379 Note: by removing the tag [as the commit is pushed] the patch is
 380 <strong>explicitly</strong> rejected from inclusion in the stable branch(es).
 381 Thus, drop the line <strong>only</strong> if you want to cancel the nomination.
 382 </p>
 383
 384 Alternatively, if one uses the "Fixes" tag as described in the "Patch formatting"
 385 section, it nominates a commit for all active stable branches that include the
 386 commit that is referred to.
 387
 388 <h2 id="criteria">Criteria for accepting patches to the stable branch</h2>
 389
 390 Mesa has a designated release manager for each stable branch, and the release
 391 manager is the only developer that should be pushing changes to these branches.
 392 Everyone else should nominate patches using the mechanism described above.
 393
 394 The following rules define which patches are accepted and which are not. The
 395 stable-release manager is also given broad discretion in rejecting patches
 396 that have been nominated.
 397
 398 <ul>
 399   <li>Patch must conform with the <a href="#guidelines">Basic guidelines</a></li>
 400
 401   <li>Patch must have landed in master first. In case where the original
 402   patch is too large and/or otherwise contradicts with the rules set within, a
 403   backport is appropriate.</li>
 404
 405   <li>It must not introduce a regression - be that build or runtime wise.
 406
 407   Note:  If the regression is due to faulty piglit/dEQP/CTS/other test the
 408   latter must be fixed first. A reference to the offending test(s) and
 409   respective fix(es) should be provided in the nominated patch.</li>
 410
 411   <li>Patch cannot be larger than 100 lines.</li>
 412
 413   <li>Patches that move code around with no functional change should be
 414   rejected.</li>
 415
 416   <li>Patch must be a bug fix and not a new feature.
 417
 418   Note: An exception to this rule, are hardware-enabling "features". For
 419   example, <a href="#backports">backports</a> of new code to support a
 420   newly-developed hardware product can be accepted if they can be reasonably
 421   determined not to have effects on other hardware.</li>
 422
 423   <li>Patch must be reviewed, For example, the commit message has Reviewed-by,
 424   Signed-off-by, or Tested-by tags from someone but the author.</li>
 425
 426   <li>Performance patches are considered only if they provide information
 427   about the hardware, program in question and observed improvement. Use numbers
 428   to represent your measurements.</li>
 429 </ul>
 430
 431 If the patch complies with the rules it will be
 432 <a href="releasing.html#pickntest">cherry-picked</a>. Alternatively the release
 433 manager will reply to the patch in question stating why the patch has been
 434 rejected or would request a backport.
 435
 436 A summary of all the picked/rejected patches will be presented in the
 437 <a href="releasing.html#prerelease">pre-release</a> announcement.
 438
 439 The stable-release manager may at times need to force-push changes to the
 440 stable branches, for example, to drop a previously-picked patch that was later
 441 identified as causing a regression). These force-pushes may cause changes to
 442 be lost from the stable branch if developers push things directly. Consider
 443 yourself warned.
 444
 445 <h2 id="backports">Sending backports for the stable branch</h2>
 446 <p>
 447 By default merge conflicts are resolved by the stable-release manager. In which
 448 case he/she should provide a comment about the changes required, alongside the
 449 <code>Conflicts</code> section. Summary of which will be provided in the
 450 <a href="releasing.html#prerelease">pre-release</a> announcement.
 451 </p>
 452
 453 <p>
 454 Developers are interested in sending backports are recommended to use either a
 455 <code>[BACKPORT #branch]</code> subject prefix or provides similar information
 456 within the commit summary.
 457 </p>
 458
 459 <h2 id="gittips">Git tips</h2>
 460
 461 <ul>
 462 <li><code>git rebase -i ...</code> is your friend. Don't be afraid to use it.
 463 <li>Apply a fixup to commit FOO.
 464 <pre>
 465     git add ...
 466     git commit --fixup=FOO
 467     git rebase -i --autosquash ...
 468 </pre>
 469 <li>Test for build breakage between patches e.g last 8 commits.
 470 <pre>
 471     git rebase -i --exec="ninja -C build/" HEAD~8
 472 </pre>
 473 <li>Sets the default mailing address for your repo.
 474 <pre>
 475     git config --local sendemail.to mesa-dev@lists.freedesktop.org
 476 </pre>
 477 <li> Add version to subject line of patch series in this case for the last 8
 478 commits before sending.
 479 <pre>
 480     git send-email --subject-prefix="PATCH v4" HEAD~8
 481     git send-email -v4 @~8 # shorter version, inherited from git format-patch
 482 </pre>
 483 </ul>
 484
 485
 486 </div>
 487 </body>
 488 </html>