Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 1 | Submitting Patches |
| 2 | ================== |
| 3 | |
Erik Faye-Lund | 1d250bf | 2020-06-27 09:48:48 +0200 | [diff] [blame] | 4 | .. _guidelines: |
| 5 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 6 | Basic guidelines |
| 7 | ---------------- |
| 8 | |
| 9 | - Patches should not mix code changes with code formatting changes |
| 10 | (except, perhaps, in very trivial cases.) |
Erik Faye-Lund | 5ee55b2 | 2020-06-27 10:21:45 +0200 | [diff] [blame] | 11 | - Code patches should follow Mesa :doc:`coding |
| 12 | conventions <codingstyle>`. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 13 | - Whenever possible, patches should only affect individual Mesa/Gallium |
| 14 | components. |
| 15 | - Patches should never introduce build breaks and should be bisectable |
Erik Faye-Lund | 50e26e5 | 2020-09-29 18:57:33 +0200 | [diff] [blame] | 16 | (see ``Git bisect``.) |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 17 | - Patches should be properly :ref:`formatted <formatting>`. |
| 18 | - Patches should be sufficiently :ref:`tested <testing>` before |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 19 | submitting. |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 20 | - Patches should be :ref:`submitted <submit>` via a merge request for |
| 21 | :ref:`review <reviewing>`. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 22 | |
| 23 | .. _formatting: |
| 24 | |
| 25 | Patch formatting |
| 26 | ---------------- |
| 27 | |
Erik Faye-Lund | 50e26e5 | 2020-09-29 18:57:33 +0200 | [diff] [blame] | 28 | - Lines should be limited to 75 characters or less so that Git logs |
Erik Faye-Lund | 7f4f441 | 2020-09-29 18:58:22 +0200 | [diff] [blame] | 29 | displayed in 80-column terminals avoid line wrapping. Note that |
| 30 | ``git log`` uses 4 spaces of indentation (4 + 75 < 80). |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 31 | - The first line should be a short, concise summary of the change |
| 32 | prefixed with a module name. Examples: |
| 33 | |
| 34 | :: |
| 35 | |
| 36 | mesa: Add support for querying GL_VERTEX_ATTRIB_ARRAY_LONG |
| 37 | |
| 38 | gallium: add PIPE_CAP_DEVICE_RESET_STATUS_QUERY |
| 39 | |
| 40 | i965: Fix missing type in local variable declaration. |
| 41 | |
| 42 | - Subsequent patch comments should describe the change in more detail, |
| 43 | if needed. For example: |
| 44 | |
| 45 | :: |
| 46 | |
| 47 | i965: Remove end-of-thread SEND alignment code. |
| 48 | |
| 49 | This was present in Eric's initial implementation of the compaction code |
| 50 | for Sandybridge (commit 077d01b6). There is no documentation saying this |
| 51 | is necessary, and removing it causes no regressions in piglit on any |
| 52 | platform. |
| 53 | |
| 54 | - A "Signed-off-by:" line is not required, but not discouraged either. |
Erik Faye-Lund | 7ee8a3a | 2020-09-25 15:20:20 +0200 | [diff] [blame] | 55 | - If a patch addresses an issue in GitLab, use the Closes: tag For |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 56 | example: |
| 57 | |
| 58 | :: |
| 59 | |
| 60 | Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/1 |
| 61 | |
Erik Faye-Lund | b8f1075 | 2020-09-25 15:24:30 +0200 | [diff] [blame] | 62 | Prefer the full URL to just ``Closes: #1``, since the URL makes it |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 63 | easier to get to the bug page from ``git log`` |
| 64 | |
Eric Engestrom | a096d41 | 2020-06-07 19:14:51 +0200 | [diff] [blame] | 65 | **Do not use the ``Fixes:`` tag for this!** Mesa already uses |
| 66 | ``Fixes:`` for something else. |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 67 | See :ref:`below <fixes>`. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 68 | |
| 69 | - If there have been several revisions to a patch during the review |
| 70 | process, they should be noted such as in this example: |
| 71 | |
| 72 | :: |
| 73 | |
| 74 | st/mesa: add ARB_texture_stencil8 support (v4) |
| 75 | |
| 76 | if we support stencil texturing, enable texture_stencil8 |
| 77 | there is no requirement to support native S8 for this, |
| 78 | the texture can be converted to x24s8 fine. |
| 79 | |
| 80 | v2: fold fixes from Marek in: |
| 81 | a) put S8 last in the list |
| 82 | b) fix renderable to always test for d/s renderable |
| 83 | fixup the texture case to use a stencil only format |
| 84 | for picking the format for the texture view. |
| 85 | v3: hit fallback for getteximage |
| 86 | v4: put s8 back in front, it shouldn't get picked now (Ilia) |
| 87 | |
| 88 | - If someone tested your patch, document it with a line like this: |
| 89 | |
| 90 | :: |
| 91 | |
| 92 | Tested-by: Joe Hacker <jhacker@foo.com> |
| 93 | |
| 94 | - If the patch was reviewed (usually the case) or acked by someone, |
| 95 | that should be documented with: |
| 96 | |
| 97 | :: |
| 98 | |
| 99 | Reviewed-by: Joe Hacker <jhacker@foo.com> |
| 100 | Acked-by: Joe Hacker <jhacker@foo.com> |
| 101 | |
Eric Engestrom | a29a158 | 2020-06-07 17:36:55 +0200 | [diff] [blame] | 102 | - When updating a merge request add all the tags (``Acked-by:``, ``Reviewed-by:``, |
| 103 | ``Fixes:``, ``Cc: mesa-stable`` and/or other) to the commit messages. |
| 104 | This provides reviewers with quick feedback if the patch has already |
| 105 | been reviewed. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 106 | |
Eric Engestrom | a096d41 | 2020-06-07 19:14:51 +0200 | [diff] [blame] | 107 | .. _fixes: |
| 108 | |
| 109 | The ``Fixes:`` tag |
| 110 | ------------------ |
| 111 | |
| 112 | If a patch addresses a issue introduced with earlier commit, that |
| 113 | should be noted in the commit message. For example:: |
| 114 | |
Frank Binns | fd91744 | 2020-07-28 17:54:05 +0100 | [diff] [blame] | 115 | Fixes: d7b3707c612 ("util/disk_cache: use stat() to check if entry is a directory") |
Eric Engestrom | a096d41 | 2020-06-07 19:14:51 +0200 | [diff] [blame] | 116 | |
| 117 | You can produce those fixes lines by running this command once:: |
| 118 | |
| 119 | git config --global alias.fixes "show -s --pretty='format:Fixes: %h (\"%s\")'" |
| 120 | |
| 121 | After that, using ``git fixes <sha1>`` will print the full line for you. |
| 122 | |
Eric Engestrom | 7b7d28e | 2020-06-07 19:18:31 +0200 | [diff] [blame] | 123 | The stable tag |
| 124 | ~~~~~~~~~~~~~~ |
| 125 | |
| 126 | If you want a commit to be applied to a stable branch, you should add an |
| 127 | appropriate note to the commit message. |
| 128 | |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 129 | Using a ``Fixes:`` tag as described in :ref:`Patch formatting <formatting>` |
Eric Engestrom | 7b7d28e | 2020-06-07 19:18:31 +0200 | [diff] [blame] | 130 | is the preferred way to nominate a commit that should be backported. |
| 131 | There are scripts that will figure out which releases to apply the patch |
| 132 | to automatically, so you don't need to figure it out. |
| 133 | |
| 134 | Alternatively, you may use a "CC:" tag. Here are some examples of such a |
| 135 | note:: |
| 136 | |
Eric Engestrom | be8a8ed | 2020-07-13 15:05:41 +0200 | [diff] [blame] | 137 | Cc: mesa-stable |
| 138 | Cc: 20.0 <mesa-stable> |
Eric Engestrom | 7b7d28e | 2020-06-07 19:18:31 +0200 | [diff] [blame] | 139 | CC: 20.0 19.3 <mesa-stable> |
| 140 | |
| 141 | Using the CC tag **should** include the stable branches you want to |
| 142 | nominate the patch to. If you do not provide any version it is nominated |
| 143 | to all active stable branches. |
| 144 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 145 | .. _testing: |
| 146 | |
| 147 | Testing Patches |
| 148 | --------------- |
| 149 | |
| 150 | It should go without saying that patches must be tested. In general, do |
| 151 | whatever testing is prudent. |
| 152 | |
| 153 | You should always run the Mesa test suite before submitting patches. The |
| 154 | test suite can be run using the 'meson test' command. All tests must |
| 155 | pass before patches will be accepted, this may mean you have to update |
| 156 | the tests themselves. |
| 157 | |
| 158 | Whenever possible and applicable, test the patch with |
| 159 | `Piglit <https://piglit.freedesktop.org>`__ and/or |
| 160 | `dEQP <https://android.googlesource.com/platform/external/deqp/>`__ to |
| 161 | check for regressions. |
| 162 | |
| 163 | As mentioned at the beginning, patches should be bisectable. A good way |
| 164 | to test this is to make use of the \`git rebase\` command, to run your |
| 165 | tests on each commit. Assuming your branch is based off |
| 166 | ``origin/master``, you can run: |
| 167 | |
| 168 | :: |
| 169 | |
| 170 | $ git rebase --interactive --exec "meson test -C build/" origin/master |
| 171 | |
| 172 | replacing ``"meson test"`` with whatever other test you want to run. |
| 173 | |
| 174 | .. _submit: |
| 175 | |
| 176 | Submitting Patches |
| 177 | ------------------ |
| 178 | |
| 179 | Patches are submitted to the Mesa project via a |
| 180 | `GitLab <https://gitlab.freedesktop.org/mesa/mesa>`__ Merge Request. |
| 181 | |
| 182 | Add labels to your MR to help reviewers find it. For example: |
| 183 | |
| 184 | - Mesa changes affecting all drivers: mesa |
| 185 | - Hardware vendor specific code: amd, intel, nvidia, ... |
| 186 | - Driver specific code: anvil, freedreno, i965, iris, radeonsi, radv, |
| 187 | vc4, ... |
| 188 | - Other tag examples: gallium, util |
| 189 | |
| 190 | Tick the following when creating the MR. It allows developers to rebase |
| 191 | your work on top of master. |
| 192 | |
| 193 | :: |
| 194 | |
| 195 | Allow commits from members who can merge to the target branch |
| 196 | |
| 197 | If you revise your patches based on code review and push an update to |
| 198 | your branch, you should maintain a **clean** history in your patches. |
| 199 | There should not be "fixup" patches in the history. The series should be |
| 200 | buildable and functional after every commit whenever you push the |
| 201 | branch. |
| 202 | |
| 203 | It is your responsibility to keep the MR alive and making progress, as |
| 204 | there are no guarantees that a Mesa dev will independently take interest |
| 205 | in it. |
| 206 | |
| 207 | Some other notes: |
| 208 | |
| 209 | - Make changes and update your branch based on feedback |
| 210 | - After an update, for the feedback you handled, close the feedback |
| 211 | discussion with the "Resolve Discussion" button. This way the |
| 212 | reviewers know which feedback got handled and which didn't. |
| 213 | - Old, stale MR may be closed, but you can reopen it if you still want |
| 214 | to pursue the changes |
| 215 | - You should periodically check to see if your MR needs to be rebased |
| 216 | - Make sure your MR is closed if your patches get pushed outside of |
| 217 | GitLab |
| 218 | - Please send MRs from a personal fork rather than from the main Mesa |
| 219 | repository, as it clutters it unnecessarily. |
| 220 | |
| 221 | .. _reviewing: |
| 222 | |
| 223 | Reviewing Patches |
| 224 | ----------------- |
| 225 | |
| 226 | To participate in code review, you can monitor the GitLab Mesa `Merge |
| 227 | Requests <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests>`__ |
Erik Faye-Lund | 7ee8a3a | 2020-09-25 15:20:20 +0200 | [diff] [blame] | 228 | page, and/or register for notifications in your GitLab settings. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 229 | |
| 230 | When you've reviewed a patch, please be unambiguous about your review. |
| 231 | That is, state either |
| 232 | |
| 233 | :: |
| 234 | |
| 235 | Reviewed-by: Joe Hacker <jhacker@foo.com> |
| 236 | |
| 237 | or |
| 238 | |
| 239 | :: |
| 240 | |
| 241 | Acked-by: Joe Hacker <jhacker@foo.com> |
| 242 | |
| 243 | Rather than saying just "LGTM" or "Seems OK". |
| 244 | |
| 245 | If small changes are suggested, it's OK to say something like: |
| 246 | |
| 247 | :: |
| 248 | |
| 249 | With the above fixes, Reviewed-by: Joe Hacker <jhacker@foo.com> |
| 250 | |
| 251 | which tells the patch author that the patch can be committed, as long as |
| 252 | the issues are resolved first. |
| 253 | |
| 254 | These Reviewed-by, Acked-by, and Tested-by tags should also be amended |
| 255 | into commits in a MR before it is merged. |
| 256 | |
Erik Faye-Lund | 7ee8a3a | 2020-09-25 15:20:20 +0200 | [diff] [blame] | 257 | When providing a Reviewed-by, Acked-by, or Tested-by tag in a GitLab MR, |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 258 | enclose the tag in backticks: |
| 259 | |
| 260 | :: |
| 261 | |
| 262 | `Reviewed-by: Joe Hacker <jhacker@example.com>` |
| 263 | |
Erik Faye-Lund | 7ee8a3a | 2020-09-25 15:20:20 +0200 | [diff] [blame] | 264 | This is the markdown format for literal, and will prevent GitLab from |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 265 | hiding the < and > symbols. |
| 266 | |
| 267 | Review by non-experts is encouraged. Understanding how someone else goes |
| 268 | about solving a problem is a great way to learn your way around the |
| 269 | project. The submitter is expected to evaluate whether they have an |
| 270 | appropriate amount of review feedback from people who also understand |
| 271 | the code before merging their patches. |
| 272 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 273 | Nominating a commit for a stable branch |
| 274 | --------------------------------------- |
| 275 | |
Eric Engestrom | 0e4eb7b | 2020-06-07 17:39:56 +0200 | [diff] [blame] | 276 | There are several ways to nominate a patch for inclusion in the stable |
| 277 | branch and release. In order or preference: |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 278 | |
Eric Engestrom | 7488d49 | 2020-06-07 17:57:52 +0200 | [diff] [blame] | 279 | - By adding the ``Fixes:`` tag in the commit message as described above, if you are fixing |
Eric Engestrom | 0e4eb7b | 2020-06-07 17:39:56 +0200 | [diff] [blame] | 280 | a specific commit. |
Eric Engestrom | 7b7d28e | 2020-06-07 19:18:31 +0200 | [diff] [blame] | 281 | - By adding the ``Cc: mesa-stable`` tag in the commit message as described above. |
Eric Engestrom | 5d27d5e | 2020-06-07 17:52:45 +0200 | [diff] [blame] | 282 | - By submitting a merge request against the ``staging/year.quarter`` |
Erik Faye-Lund | 7ee8a3a | 2020-09-25 15:20:20 +0200 | [diff] [blame] | 283 | branch on GitLab. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 284 | |
| 285 | Please **DO NOT** send patches to mesa-stable@lists.freedesktop.org, it |
| 286 | is not monitored actively and is a historical artifact. |
| 287 | |
| 288 | If you are not the author of the original patch, please Cc: them in your |
| 289 | nomination request. |
| 290 | |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 291 | The current patch status can be observed in the :ref:`staging |
| 292 | branch <stagingbranch>`. |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 293 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 294 | .. _criteria: |
| 295 | |
| 296 | Criteria for accepting patches to the stable branch |
| 297 | --------------------------------------------------- |
| 298 | |
| 299 | Mesa has a designated release manager for each stable branch, and the |
| 300 | release manager is the only developer that should be pushing changes to |
| 301 | these branches. Everyone else should nominate patches using the |
| 302 | mechanism described above. The following rules define which patches are |
| 303 | accepted and which are not. The stable-release manager is also given |
| 304 | broad discretion in rejecting patches that have been nominated. |
| 305 | |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 306 | - Patch must conform with the :ref:`Basic guidelines <guidelines>` |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 307 | - Patch must have landed in master first. In case where the original |
| 308 | patch is too large and/or otherwise contradicts with the rules set |
| 309 | within, a backport is appropriate. |
| 310 | - It must not introduce a regression - be that build or runtime wise. |
Erik Faye-Lund | bf3f0f7 | 2019-06-04 10:39:58 +0200 | [diff] [blame] | 311 | |
| 312 | .. note:: |
| 313 | If the regression is due to faulty piglit/dEQP/CTS/other test |
| 314 | the latter must be fixed first. A reference to the offending test(s) |
| 315 | and respective fix(es) should be provided in the nominated patch. |
| 316 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 317 | - Patch cannot be larger than 100 lines. |
| 318 | - Patches that move code around with no functional change should be |
| 319 | rejected. |
Erik Faye-Lund | bf3f0f7 | 2019-06-04 10:39:58 +0200 | [diff] [blame] | 320 | - Patch must be a bug fix and not a new feature. |
| 321 | |
| 322 | .. note:: |
| 323 | An exception to this rule, are hardware-enabling "features". For |
Erik Faye-Lund | b1c16e5 | 2020-06-27 10:00:10 +0200 | [diff] [blame] | 324 | example, :ref:`backports <backports>` of new code to support a |
Erik Faye-Lund | bf3f0f7 | 2019-06-04 10:39:58 +0200 | [diff] [blame] | 325 | newly-developed hardware product can be accepted if they can be |
| 326 | reasonably determined not to have effects on other hardware. |
| 327 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 328 | - Patch must be reviewed, For example, the commit message has |
| 329 | Reviewed-by, Signed-off-by, or Tested-by tags from someone but the |
| 330 | author. |
| 331 | - Performance patches are considered only if they provide information |
| 332 | about the hardware, program in question and observed improvement. Use |
| 333 | numbers to represent your measurements. |
| 334 | |
| 335 | If the patch complies with the rules it will be |
Erik Faye-Lund | 5ee55b2 | 2020-06-27 10:21:45 +0200 | [diff] [blame] | 336 | :ref:`cherry-picked <pickntest>`. Alternatively the release |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 337 | manager will reply to the patch in question stating why the patch has |
| 338 | been rejected or would request a backport. The stable-release manager |
| 339 | may at times need to force-push changes to the stable branches, for |
| 340 | example, to drop a previously-picked patch that was later identified as |
| 341 | causing a regression). These force-pushes may cause changes to be lost |
| 342 | from the stable branch if developers push things directly. Consider |
| 343 | yourself warned. |
| 344 | |
| 345 | .. _backports: |
| 346 | |
| 347 | Sending backports for the stable branch |
| 348 | --------------------------------------- |
| 349 | |
| 350 | By default merge conflicts are resolved by the stable-release manager. |
| 351 | The release maintainer should resolve trivial conflicts, but for complex |
| 352 | conflicts they should ask the original author to provide a backport or |
| 353 | de-nominate the patch. |
| 354 | |
| 355 | For patches that either need to be nominated after they've landed in |
| 356 | master, or that are known ahead of time to not not apply cleanly to a |
Erik Faye-Lund | 7ee8a3a | 2020-09-25 15:20:20 +0200 | [diff] [blame] | 357 | stable branch (such as due to a rename), using a GitLab MR is most |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 358 | appropriate. The MR should be based on and target the |
| 359 | staging/year.quarter branch, not on the year.quarter branch, per the |
| 360 | stable branch policy. Assigning the MR to release maintainer for said |
| 361 | branch or mentioning them is helpful, but not required. |
| 362 | |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 363 | Git tips |
| 364 | -------- |
| 365 | |
| 366 | - ``git rebase -i ...`` is your friend. Don't be afraid to use it. |
| 367 | - Apply a fixup to commit FOO. |
| 368 | |
Erik Faye-Lund | d6be994 | 2019-06-04 14:14:13 +0200 | [diff] [blame] | 369 | .. code-block:: console |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 370 | |
| 371 | git add ... |
| 372 | git commit --fixup=FOO |
| 373 | git rebase -i --autosquash ... |
| 374 | |
| 375 | - Test for build breakage between patches e.g last 8 commits. |
| 376 | |
Erik Faye-Lund | d6be994 | 2019-06-04 14:14:13 +0200 | [diff] [blame] | 377 | .. code-block:: console |
Erik Faye-Lund | 4d06683 | 2020-06-12 20:09:42 +0200 | [diff] [blame] | 378 | |
| 379 | git rebase -i --exec="ninja -C build/" HEAD~8 |