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 beginning, 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> By adding the fixes: tag as described below.
 282 <li> By submitting a merge request against the "staging/year.quarter" branch on gitlab.
 283 </li>
 284 </ul>
 285 <p>
 286 Please <strong>DO NOT</strong> send patches to
 287 mesa-stable@lists.freedesktop.org, it is not monitored actively and is a
 288 historical artifact.
 289 </p>
 290 <p>
 291 If you are not the author of the original patch, please Cc: them in your
 292 nomination request.
 293 </p>
 294
 295 <p>
 296 The current patch status can be observed in the <a href="releasing.html#stagingbranch">staging branch</a>.
 297 </p>
 298
 299 <h3 id="thetag">The stable tag</h3>
 300
 301 <p>
 302 If you want a commit to be applied to a stable branch,
 303 you should add an appropriate note to the commit message.
 304 </p>
 305
 306 <p>
 307 Using a "fixes tag" as described in <a href="#formatting">Patch formatting</a>
 308 is the preferred way to nominate a commit that you know ahead of time should be
 309 backported. There are scripts that will figure out which releases to apply the
 310 patch to automatically, so you don't need to figure it out.
 311 </p>
 312
 313 <p>
 314 Alternatively, you may use a "CC:" tag.
 315
 316 Here are some examples of such a note:
 317 </p>
 318 <pre>
 319 CC: 20.0 19.3 &lt;mesa-stable@lists.freedesktop.org&gt;
 320 </pre>
 321
 322 <p>
 323 Using the CC tag <strong>should</strong> include the stable branches you want
 324 to nominate the patch to. If you do not provide any version it is nominated to
 325 all active stable branches.
 326 </p>
 327
 328 <h2 id="criteria">Criteria for accepting patches to the stable branch</h2>
 329
 330 Mesa has a designated release manager for each stable branch, and the release
 331 manager is the only developer that should be pushing changes to these branches.
 332 Everyone else should nominate patches using the mechanism described above.
 333
 334 The following rules define which patches are accepted and which are not. The
 335 stable-release manager is also given broad discretion in rejecting patches
 336 that have been nominated.
 337
 338 <ul>
 339   <li>Patch must conform with the <a href="#guidelines">Basic guidelines</a></li>
 340
 341   <li>Patch must have landed in master first. In case where the original
 342   patch is too large and/or otherwise contradicts with the rules set within, a
 343   backport is appropriate.</li>
 344
 345   <li>It must not introduce a regression - be that build or runtime wise.
 346
 347   Note:  If the regression is due to faulty piglit/dEQP/CTS/other test the
 348   latter must be fixed first. A reference to the offending test(s) and
 349   respective fix(es) should be provided in the nominated patch.</li>
 350
 351   <li>Patch cannot be larger than 100 lines.</li>
 352
 353   <li>Patches that move code around with no functional change should be
 354   rejected.</li>
 355
 356   <li>Patch must be a bug fix and not a new feature.
 357
 358   Note: An exception to this rule, are hardware-enabling "features". For
 359   example, <a href="#backports">backports</a> of new code to support a
 360   newly-developed hardware product can be accepted if they can be reasonably
 361   determined not to have effects on other hardware.</li>
 362
 363   <li>Patch must be reviewed, For example, the commit message has Reviewed-by,
 364   Signed-off-by, or Tested-by tags from someone but the author.</li>
 365
 366   <li>Performance patches are considered only if they provide information
 367   about the hardware, program in question and observed improvement. Use numbers
 368   to represent your measurements.</li>
 369 </ul>
 370
 371 If the patch complies with the rules it will be
 372 <a href="releasing.html#pickntest">cherry-picked</a>. Alternatively the release
 373 manager will reply to the patch in question stating why the patch has been
 374 rejected or would request a backport.
 375
 376 The stable-release manager may at times need to force-push changes to the
 377 stable branches, for example, to drop a previously-picked patch that was later
 378 identified as causing a regression). These force-pushes may cause changes to
 379 be lost from the stable branch if developers push things directly. Consider
 380 yourself warned.
 381
 382 <h2 id="backports">Sending backports for the stable branch</h2>
 383 <p>
 384 By default merge conflicts are resolved by the stable-release manager. The
 385 release maintainer should resolve trivial conflicts, but for complex conflicts
 386 they should ask the original author to provide a backport or de-nominate the
 387 patch.
 388 </p>
 389
 390 <p>
 391 For patches that either need to be nominated after they've landed in master, or
 392 that are known ahead of time to not not apply cleanly to a stable branch (such
 393 as due to a rename), using a gitlab MR is most appropriate.
 394
 395 The MR should be based on and target the staging/year.quarter branch, not on
 396 the year.quarter branch, per the stable branch policy.
 397
 398 Assigning the MR to release maintainer for said branch or mentioning them is
 399 helpful, but not required.
 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>