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