1Submitting Patches 2================== 3 4.. _guidelines: 5 6Basic guidelines 7---------------- 8 9- Patches should not mix code changes with code formatting changes 10 (except, perhaps, in very trivial cases.) 11- Code patches should follow Mesa :doc:`coding 12 conventions <codingstyle>`. 13- Whenever possible, patches should only affect individual Mesa/Gallium 14 components. 15- Patches should never introduce build breaks and should be bisectable 16 (see ``Git bisect``.) 17- Patches should be properly :ref:`formatted <formatting>`. 18- Patches should be sufficiently :ref:`tested <testing>` before 19 submitting. 20- Patches should be :ref:`submitted <submit>` via a merge request for 21 :ref:`review <reviewing>`. 22 23.. _formatting: 24 25Patch formatting 26---------------- 27 28- Lines should be limited to 75 characters or less so that Git logs 29 displayed in 80-column terminals avoid line wrapping. Note that 30 ``git log`` uses 4 spaces of indentation (4 + 75 < 80). 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. 55- If a patch addresses an issue in GitLab, use the Closes: tag For 56 example: 57 58 :: 59 60 Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/1 61 62 Prefer the full URL to just ``Closes: #1``, since the URL makes it 63 easier to get to the bug page from ``git log`` 64 65 **Do not use the ``Fixes:`` tag for this!** Mesa already uses 66 ``Fixes:`` for something else. 67 See :ref:`below <fixes>`. 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 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. 106 107.. _fixes: 108 109The ``Fixes:`` tag 110------------------ 111 112If a patch addresses a issue introduced with earlier commit, that 113should be noted in the commit message. For example:: 114 115 Fixes: d7b3707c612 ("util/disk_cache: use stat() to check if entry is a directory") 116 117You 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 121After that, using ``git fixes <sha1>`` will print the full line for you. 122 123The stable tag 124~~~~~~~~~~~~~~ 125 126If you want a commit to be applied to a stable branch, you should add an 127appropriate note to the commit message. 128 129Using a ``Fixes:`` tag as described in :ref:`Patch formatting <formatting>` 130is the preferred way to nominate a commit that should be backported. 131There are scripts that will figure out which releases to apply the patch 132to automatically, so you don't need to figure it out. 133 134Alternatively, you may use a "CC:" tag. Here are some examples of such a 135note:: 136 137 Cc: mesa-stable 138 Cc: 20.0 <mesa-stable> 139 CC: 20.0 19.3 <mesa-stable> 140 141Using the CC tag **should** include the stable branches you want to 142nominate the patch to. If you do not provide any version it is nominated 143to all active stable branches. 144 145.. _testing: 146 147Testing Patches 148--------------- 149 150It should go without saying that patches must be tested. In general, do 151whatever testing is prudent. 152 153You should always run the Mesa test suite before submitting patches. The 154test suite can be run using the 'meson test' command. All tests must 155pass before patches will be accepted, this may mean you have to update 156the tests themselves. 157 158Whenever 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 161check for regressions. 162 163As mentioned at the beginning, patches should be bisectable. A good way 164to test this is to make use of the \`git rebase\` command, to run your 165tests 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 172replacing ``"meson test"`` with whatever other test you want to run. 173 174.. _submit: 175 176Submitting Patches 177------------------ 178 179Patches are submitted to the Mesa project via a 180`GitLab <https://gitlab.freedesktop.org/mesa/mesa>`__ Merge Request. 181 182Add 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 190Tick the following when creating the MR. It allows developers to rebase 191your work on top of master. 192 193:: 194 195 Allow commits from members who can merge to the target branch 196 197If you revise your patches based on code review and push an update to 198your branch, you should maintain a **clean** history in your patches. 199There should not be "fixup" patches in the history. The series should be 200buildable and functional after every commit whenever you push the 201branch. 202 203It is your responsibility to keep the MR alive and making progress, as 204there are no guarantees that a Mesa dev will independently take interest 205in it. 206 207Some 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 223Reviewing Patches 224----------------- 225 226To participate in code review, you can monitor the GitLab Mesa `Merge 227Requests <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests>`__ 228page, and/or register for notifications in your GitLab settings. 229 230When you've reviewed a patch, please be unambiguous about your review. 231That is, state either 232 233:: 234 235 Reviewed-by: Joe Hacker <jhacker@foo.com> 236 237or 238 239:: 240 241 Acked-by: Joe Hacker <jhacker@foo.com> 242 243Rather than saying just "LGTM" or "Seems OK". 244 245If 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 251which tells the patch author that the patch can be committed, as long as 252the issues are resolved first. 253 254These Reviewed-by, Acked-by, and Tested-by tags should also be amended 255into commits in a MR before it is merged. 256 257When providing a Reviewed-by, Acked-by, or Tested-by tag in a GitLab MR, 258enclose the tag in backticks: 259 260:: 261 262 `Reviewed-by: Joe Hacker <jhacker@example.com>` 263 264This is the markdown format for literal, and will prevent GitLab from 265hiding the < and > symbols. 266 267Review by non-experts is encouraged. Understanding how someone else goes 268about solving a problem is a great way to learn your way around the 269project. The submitter is expected to evaluate whether they have an 270appropriate amount of review feedback from people who also understand 271the code before merging their patches. 272 273Nominating a commit for a stable branch 274--------------------------------------- 275 276There are several ways to nominate a patch for inclusion in the stable 277branch and release. In order or preference: 278 279- By adding the ``Fixes:`` tag in the commit message as described above, if you are fixing 280 a specific commit. 281- By adding the ``Cc: mesa-stable`` tag in the commit message as described above. 282- By submitting a merge request against the ``staging/year.quarter`` 283 branch on GitLab. 284 285Please **DO NOT** send patches to mesa-stable@lists.freedesktop.org, it 286is not monitored actively and is a historical artifact. 287 288If you are not the author of the original patch, please Cc: them in your 289nomination request. 290 291The current patch status can be observed in the :ref:`staging 292branch <stagingbranch>`. 293 294.. _criteria: 295 296Criteria for accepting patches to the stable branch 297--------------------------------------------------- 298 299Mesa has a designated release manager for each stable branch, and the 300release manager is the only developer that should be pushing changes to 301these branches. Everyone else should nominate patches using the 302mechanism described above. The following rules define which patches are 303accepted and which are not. The stable-release manager is also given 304broad discretion in rejecting patches that have been nominated. 305 306- Patch must conform with the :ref:`Basic guidelines <guidelines>` 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. 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 317- Patch cannot be larger than 100 lines. 318- Patches that move code around with no functional change should be 319 rejected. 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 324 example, :ref:`backports <backports>` of new code to support a 325 newly-developed hardware product can be accepted if they can be 326 reasonably determined not to have effects on other hardware. 327 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 335If the patch complies with the rules it will be 336:ref:`cherry-picked <pickntest>`. Alternatively the release 337manager will reply to the patch in question stating why the patch has 338been rejected or would request a backport. The stable-release manager 339may at times need to force-push changes to the stable branches, for 340example, to drop a previously-picked patch that was later identified as 341causing a regression). These force-pushes may cause changes to be lost 342from the stable branch if developers push things directly. Consider 343yourself warned. 344 345.. _backports: 346 347Sending backports for the stable branch 348--------------------------------------- 349 350By default merge conflicts are resolved by the stable-release manager. 351The release maintainer should resolve trivial conflicts, but for complex 352conflicts they should ask the original author to provide a backport or 353de-nominate the patch. 354 355For patches that either need to be nominated after they've landed in 356master, or that are known ahead of time to not not apply cleanly to a 357stable branch (such as due to a rename), using a GitLab MR is most 358appropriate. The MR should be based on and target the 359staging/year.quarter branch, not on the year.quarter branch, per the 360stable branch policy. Assigning the MR to release maintainer for said 361branch or mentioning them is helpful, but not required. 362 363Git 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 369 .. code-block:: console 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 377 .. code-block:: console 378 379 git rebase -i --exec="ninja -C build/" HEAD~8 380