<html lang="en">
<head>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
- <title>Submitting patches</title>
+ <title>Submitting Patches</title>
<link rel="stylesheet" type="text/css" href="mesa.css">
</head>
<body>
<div class="header">
- <h1>The Mesa 3D Graphics Library</h1>
+ The Mesa 3D Graphics Library
</div>
<iframe src="contents.html"></iframe>
<div class="content">
-<h1>Submitting patches</h1>
+<h1>Submitting Patches</h1>
<ul>
<li><a href="#guidelines">Basic guidelines</a>
<li><a href="#formatting">Patch formatting</a>
<li><a href="#testing">Testing Patches</a>
-<li><a href="#mailing">Mailing Patches</a>
+<li><a href="#submit">Submitting Patches</a>
<li><a href="#reviewing">Reviewing Patches</a>
<li><a href="#nominations">Nominating a commit for a stable branch</a>
<li><a href="#criteria">Criteria for accepting patches to the stable branch</a>
+<li><a href="#backports">Sending backports for the stable branch</a>
+<li><a href="#gittips">Git tips</a>
</ul>
<h2 id="guidelines">Basic guidelines</h2>
perhaps, in very trivial cases.)
<li>Code patches should follow Mesa
<a href="codingstyle.html" target="_parent">coding conventions</a>.
-<li>Whenever possible, patches should only effect individual Mesa/Gallium
+<li>Whenever possible, patches should only affect individual Mesa/Gallium
components.
<li>Patches should never introduce build breaks and should be bisectable (see
<code>git bisect</code>.)
<li>Patches should be properly <a href="#formatting">formatted</a>.
<li>Patches should be sufficiently <a href="#testing">tested</a> before submitting.
-<li>Patches should be submitted to <a href="#mailing">submitted to mesa-dev</a>
-for <a href="#reviewing">review</a> using <code>git send-email</code>.
+<li>Patches should be <a href="#submit">submitted</a> via a merge request for
+<a href="#reviewing">review</a>.
</ul>
<li>The first line should be a short, concise summary of the change prefixed
with a module name. Examples:
<pre>
- mesa: Add support for querying GL_VERTEX_ATTRIB_ARRAY_LONG
+mesa: Add support for querying GL_VERTEX_ATTRIB_ARRAY_LONG
- gallium: add PIPE_CAP_DEVICE_RESET_STATUS_QUERY
+gallium: add PIPE_CAP_DEVICE_RESET_STATUS_QUERY
- i965: Fix missing type in local variable declaration.
+i965: Fix missing type in local variable declaration.
</pre>
<li>Subsequent patch comments should describe the change in more detail,
if needed. For example:
<pre>
- i965: Remove end-of-thread SEND alignment code.
-
- This was present in Eric's initial implementation of the compaction code
- for Sandybridge (commit 077d01b6). There is no documentation saying this
- is necessary, and removing it causes no regressions in piglit on any
- platform.
+i965: Remove end-of-thread SEND alignment code.
+
+This was present in Eric's initial implementation of the compaction code
+for Sandybridge (commit 077d01b6). There is no documentation saying this
+is necessary, and removing it causes no regressions in piglit on any
+platform.
</pre>
<li>A "Signed-off-by:" line is not required, but not discouraged either.
-<li>If a patch address a bugzilla issue, that should be noted in the
-patch comment. For example:
+<li>If a patch addresses an issue in gitlab, use the Closes: tag
+For example:
+<pre>
+Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/1
+</pre>
+<p>Prefer the full url to just <code>Closes: #1</code>, since the url makes it
+easier to get to the bug page from <code>git log</code></p>
+<b>Do not use the Fixes: tag for this!</b> Mesa already uses Fixes for something else.
+
+<li>If a patch addresses a issue introduced with earlier commit, that should be
+noted in the patch comment. For example:
<pre>
- Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89689
+Fixes: d7b3707c612 "util/disk_cache: use stat() to check if entry is a directory"
</pre>
+<li>You can produce those fixes lines by running
+<pre>git config --global alias.fixes "show -s --pretty='format:Fixes: %h (\"%s\")'"</pre>
+once and then using <pre>git fixes <sha1></pre>
<li>If there have been several revisions to a patch during the review
process, they should be noted such as in this example:
<pre>
- st/mesa: add ARB_texture_stencil8 support (v4)
-
- if we support stencil texturing, enable texture_stencil8
- there is no requirement to support native S8 for this,
- the texture can be converted to x24s8 fine.
-
- v2: fold fixes from Marek in:
- a) put S8 last in the list
- b) fix renderable to always test for d/s renderable
- fixup the texture case to use a stencil only format
- for picking the format for the texture view.
- v3: hit fallback for getteximage
- v4: put s8 back in front, it shouldn't get picked now (Ilia)
+st/mesa: add ARB_texture_stencil8 support (v4)
+
+if we support stencil texturing, enable texture_stencil8
+there is no requirement to support native S8 for this,
+the texture can be converted to x24s8 fine.
+
+v2: fold fixes from Marek in:
+ a) put S8 last in the list
+ b) fix renderable to always test for d/s renderable
+ fixup the texture case to use a stencil only format
+ for picking the format for the texture view.
+v3: hit fallback for getteximage
+v4: put s8 back in front, it shouldn't get picked now (Ilia)
</pre>
<li>If someone tested your patch, document it with a line like this:
<pre>
- Tested-by: Joe Hacker <jhacker@foo.com>
+Tested-by: Joe Hacker <jhacker@foo.com>
</pre>
<li>If the patch was reviewed (usually the case) or acked by someone,
that should be documented with:
<pre>
- Reviewed-by: Joe Hacker <jhacker@foo.com>
- Acked-by: Joe Hacker <jhacker@foo.com>
+Reviewed-by: Joe Hacker <jhacker@foo.com>
+Acked-by: Joe Hacker <jhacker@foo.com>
</pre>
+<li>If sending later revision of a patch, add all the tags - ack, r-b,
+Cc: mesa-stable and/or other. This provides reviewers with quick feedback if the
+patch has already been reviewed.
</ul>
<p>
You should always run the Mesa test suite before submitting patches.
-The test suite can be run using the 'make check' command. All tests
+The test suite can be run using the 'meson test' command. All tests
must pass before patches will be accepted, this may mean you have
to update the tests themselves.
</p>
<p>
Whenever possible and applicable, test the patch with
-<a href="http://piglit.freedesktop.org">Piglit</a> and/or
+<a href="https://piglit.freedesktop.org">Piglit</a> and/or
<a href="https://android.googlesource.com/platform/external/deqp/">dEQP</a>
to check for regressions.
</p>
+<p>
+As mentioned at the beginning, patches should be bisectable.
+A good way to test this is to make use of the `git rebase` command,
+to run your tests on each commit. Assuming your branch is based off
+<code>origin/master</code>, you can run:
+</p>
+<pre>
+$ git rebase --interactive --exec "meson test -C build/" origin/master
+</pre>
+<p>
+replacing <code>"meson test"</code> with whatever other test you want to
+run.
+</p>
-<h2 id="mailing">Mailing Patches</h2>
+<h2 id="submit">Submitting Patches</h2>
<p>
-Patches should be sent to the mesa-dev mailing list for review:
-<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">
-mesa-dev@lists.freedesktop.org<a/>.
-When submitting a patch make sure to use
-<a href="https://git-scm.com/docs/git-send-email">git send-email</a>
-rather than attaching patches to emails. Sending patches as
-attachments prevents people from being able to provide in-line review
-comments.
+Patches are submitted to the Mesa project via a
+<a href="https://gitlab.freedesktop.org/mesa/mesa">GitLab</a> Merge Request.
</p>
<p>
-When submitting follow-up patches you can use --in-reply-to to make v2, v3,
-etc patches show up as replies to the originals. This usually works well
-when you're sending out updates to individual patches (as opposed to
-re-sending the whole series). Using --in-reply-to makes
-it harder for reviewers to accidentally review old patches.
+ Add labels to your MR to help reviewers find it. For example:
+</p>
+<ul>
+ <li>Mesa changes affecting all drivers: mesa
+ <li>Hardware vendor specific code: amd, intel, nvidia, ...
+ <li>Driver specific code: anvil, freedreno, i965, iris, radeonsi,
+ radv, vc4, ...
+ <li>Other tag examples: gallium, util
+</ul>
+<p>
+ Tick the following when creating the MR. It allows developers to
+ rebase your work on top of master.
+</p>
+<pre>Allow commits from members who can merge to the target branch</pre>
+<p>
+ If you revise your patches based on code review and push an update
+ to your branch, you should maintain a <strong>clean</strong> history
+ in your patches. There should not be "fixup" patches in the history.
+ The series should be buildable and functional after every commit
+ whenever you push the branch.
</p>
-
<p>
-When submitting follow-up patches you should also login to
-<a href="https://patchwork.freedesktop.org">patchwork</a> and change the
-state of your old patches to Superseded.
+ It is your responsibility to keep the MR alive and making progress,
+ as there are no guarantees that a Mesa dev will independently take
+ interest in it.
</p>
+<p>
+ Some other notes:
+</p>
+<ul>
+ <li>Make changes and update your branch based on feedback
+ <li>After an update, for the feedback you handled, close the
+ feedback discussion with the "Resolve Discussion" button. This way
+ the reviewers know which feedback got handled and which didn't.
+ <li>Old, stale MR may be closed, but you can reopen it if you
+ still want to pursue the changes
+ <li>You should periodically check to see if your MR needs to be
+ rebased
+ <li>Make sure your MR is closed if your patches get pushed outside
+ of GitLab
+ <li>Please send MRs from a personal fork rather than from the main
+ Mesa repository, as it clutters it unnecessarily.
+</ul>
<h2 id="reviewing">Reviewing Patches</h2>
<p>
-When you've reviewed a patch on the mailing list, please be unambiguous
-about your review. That is, state either
+ To participate in code review, you can monitor the GitLab Mesa
+ <a href="https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests">Merge
+ Requests</a> page, and/or register for notifications in your gitlab
+ settings.
+</p>
+
+<p>
+When you've reviewed a patch, please be unambiguous about your review.
+ That is, state either
+</p>
<pre>
- Reviewed-by: Joe Hacker <jhacker@foo.com>
+Reviewed-by: Joe Hacker <jhacker@foo.com>
</pre>
or
<pre>
- Acked-by: Joe Hacker <jhacker@foo.com>
+Acked-by: Joe Hacker <jhacker@foo.com>
</pre>
+<p>
Rather than saying just "LGTM" or "Seems OK".
</p>
<p>
If small changes are suggested, it's OK to say something like:
+</p>
<pre>
- With the above fixes, Reviewed-by: Joe Hacker <jhacker@foo.com>
+With the above fixes, Reviewed-by: Joe Hacker <jhacker@foo.com>
</pre>
+<p>
which tells the patch author that the patch can be committed, as long
as the issues are resolved first.
</p>
+<p>
+These Reviewed-by, Acked-by, and Tested-by tags should also be amended
+into commits in a MR before it is merged.
+</p>
+
+<p>
+When providing a Reviewed-by, Acked-by, or Tested-by tag in a gitlab MR,
+enclose the tag in backticks:
+</p>
+<pre>
+`Reviewed-by: Joe Hacker <jhacker@example.com>`</pre>
+<p>
+This is the markdown format for literal, and will prevent gitlab from hiding
+the < and > symbols.
+</p>
+
+<p>
+Review by non-experts is encouraged. Understanding how someone else
+goes about solving a problem is a great way to learn your way around
+the project. The submitter is expected to evaluate whether they have
+an appropriate amount of review feedback from people who also
+understand the code before merging their patches.
+</p>
<h2 id="nominations">Nominating a commit for a stable branch</h2>
<p>
-There are three ways to nominate patch for inclusion of the stable branch and
+There are three ways to nominate a patch for inclusion in the stable branch and
release.
</p>
<ul>
<li> By adding the Cc: mesa-stable@ tag as described below.
-<li> Sending the commit ID (as seen in master branch) to the mesa-stable@ mailing list.
-<li> Forwarding the patch from the mesa-dev@ mailing list.
+<li> By adding the fixes: tag as described below.
+<li> By submitting a merge request against the "staging/year.quarter" branch on gitlab.
</li>
</ul>
<p>
-Note: resending patch identical to one on mesa-dev@ or one that differs only
-by the extra mesa-stable@ tag is <strong>not</strong> recommended.
+Please <strong>DO NOT</strong> send patches to
+mesa-stable@lists.freedesktop.org, it is not monitored actively and is a
+historical artifact.
+</p>
+<p>
+If you are not the author of the original patch, please Cc: them in your
+nomination request.
</p>
+<p>
+The current patch status can be observed in the <a href="releasing.html#stagingbranch">staging branch</a>.
+</p>
<h3 id="thetag">The stable tag</h3>
</p>
<p>
-Here are some examples of such a note:
+Using a "fixes tag" as described in <a href="#formatting">Patch formatting</a>
+is the preferred way to nominate a commit that you know ahead of time should be
+backported. There are scripts that will figure out which releases to apply the
+patch to automatically, so you don't need to figure it out.
</p>
-<ul>
- <li>CC: <mesa-stable@lists.freedesktop.org></li>
- <li>CC: "9.2 10.0" <mesa-stable@lists.freedesktop.org></li>
- <li>CC: "10.0" <mesa-stable@lists.freedesktop.org></li>
-</ul>
-Simply adding the CC to the mesa-stable list address is adequate to nominate
-the commit for the most-recently-created stable branch. It is only necessary
-to specify a specific branch name, (such as "9.2 10.0" or "10.0" in the
-examples above), if you want to nominate the commit for an older stable
-branch. And, as in these examples, you can nominate the commit for the older
-branch in addition to the more recent branch, or nominate the commit
-exclusively for the older branch.
+<p>
+Alternatively, you may use a "CC:" tag.
-This "CC" syntax for patch nomination will cause patches to automatically be
-copied to the mesa-stable@ mailing list when you use "git send-email" to send
-patches to the mesa-dev@ mailing list. If you prefer using --suppress-cc that
-won't have any effect negative effect on the patch nomination.
+Here are some examples of such a note:
+</p>
+<pre>
+CC: 20.0 19.3 <mesa-stable@lists.freedesktop.org>
+</pre>
+
+<p>
+Using the CC tag <strong>should</strong> include the stable branches you want
+to nominate the patch to. If you do not provide any version it is nominated to
+all active stable branches.
+</p>
<h2 id="criteria">Criteria for accepting patches to the stable branch</h2>
Mesa has a designated release manager for each stable branch, and the release
-manager is the only developer that should be pushing changes to these
-branches. Everyone else should simply nominate patches using the mechanism
-described above.
-
-The stable-release manager will work with the list of nominated patches, and
-for each patch that meets the crtieria below will cherry-pick the patch with:
-<code>git cherry-pick -x <commit></code>. The <code>-x</code> option is
-important so that the picked patch references the comit ID of the original
-patch.
+manager is the only developer that should be pushing changes to these branches.
+Everyone else should nominate patches using the mechanism described above.
+
+The following rules define which patches are accepted and which are not. The
+stable-release manager is also given broad discretion in rejecting patches
+that have been nominated.
+
+<ul>
+ <li>Patch must conform with the <a href="#guidelines">Basic guidelines</a></li>
+
+ <li>Patch must have landed in master first. In case where the original
+ patch is too large and/or otherwise contradicts with the rules set within, a
+ backport is appropriate.</li>
+
+ <li>It must not introduce a regression - be that build or runtime wise.
+
+ Note: If the regression is due to faulty piglit/dEQP/CTS/other test the
+ latter must be fixed first. A reference to the offending test(s) and
+ respective fix(es) should be provided in the nominated patch.</li>
+
+ <li>Patch cannot be larger than 100 lines.</li>
+
+ <li>Patches that move code around with no functional change should be
+ rejected.</li>
+
+ <li>Patch must be a bug fix and not a new feature.
+
+ Note: An exception to this rule, are hardware-enabling "features". For
+ example, <a href="#backports">backports</a> of new code to support a
+ newly-developed hardware product can be accepted if they can be reasonably
+ determined not to have effects on other hardware.</li>
+
+ <li>Patch must be reviewed, For example, the commit message has Reviewed-by,
+ Signed-off-by, or Tested-by tags from someone but the author.</li>
+
+ <li>Performance patches are considered only if they provide information
+ about the hardware, program in question and observed improvement. Use numbers
+ to represent your measurements.</li>
+</ul>
+
+If the patch complies with the rules it will be
+<a href="releasing.html#pickntest">cherry-picked</a>. Alternatively the release
+manager will reply to the patch in question stating why the patch has been
+rejected or would request a backport.
The stable-release manager may at times need to force-push changes to the
stable branches, for example, to drop a previously-picked patch that was later
be lost from the stable branch if developers push things directly. Consider
yourself warned.
-The stable-release manager is also given broad discretion in rejecting patches
-that have been nominated for the stable branch. The most basic rule is that
-the stable branch is for bug fixes only, (no new features, no
-regressions). Here is a non-exhaustive list of some reasons that a patch may
-be rejected:
+<h2 id="backports">Sending backports for the stable branch</h2>
+<p>
+By default merge conflicts are resolved by the stable-release manager. The
+release maintainer should resolve trivial conflicts, but for complex conflicts
+they should ask the original author to provide a backport or de-nominate the
+patch.
+</p>
+
+<p>
+For patches that either need to be nominated after they've landed in master, or
+that are known ahead of time to not not apply cleanly to a stable branch (such
+as due to a rename), using a gitlab MR is most appropriate.
+
+The MR should be based on and target the staging/year.quarter branch, not on
+the year.quarter branch, per the stable branch policy.
+
+Assigning the MR to release maintainer for said branch or mentioning them is
+helpful, but not required.
+</p>
+
+<h2 id="gittips">Git tips</h2>
<ul>
- <li>Patch introduces a regression. Any reported build breakage or other
- regression caused by a particular patch, (game no longer work, piglit test
- changes from PASS to FAIL), is justification for rejecting a patch.</li>
-
- <li>Patch is too large, (say, larger than 100 lines)</li>
-
- <li>Patch is not a fix. For example, a commit that moves code around with no
- functional change should be rejected.</li>
-
- <li>Patch fix is not clearly described. For example, a commit message
- of only a single line, no description of the bug, no mention of bugzilla,
- etc.</li>
-
- <li>Patch has not obviously been reviewed, For example, the commit message
- has no Reviewed-by, Signed-off-by, nor Tested-by tags from anyone but the
- author.</li>
-
- <li>Patch has not already been merged to the master branch. As a rule, bug
- fixes should never be applied first to a stable branch. Patches should land
- first on the master branch and then be cherry-picked to a stable
- branch. (This is to avoid future releases causing regressions if the patch
- is not also applied to master.) The only things that might look like
- exceptions would be backports of patches from master that happen to look
- significantly different.</li>
-
- <li>Patch depends on too many other patches. Ideally, all stable-branch
- patches should be self-contained. It sometimes occurs that a single, logical
- bug-fix occurs as two separate patches on master, (such as an original
- patch, then a subsequent fix-up to that patch). In such a case, these two
- patches should be squashed into a single, self-contained patch for the
- stable branch. (Of course, if the squashing makes the patch too large, then
- that could be a reason to reject the patch.)</li>
-
- <li>Patch includes new feature development, not bug fixes. New OpenGL
- features, extensions, etc. should be applied to Mesa master and included in
- the next major release. Stable releases are intended only for bug fixes.
-
- Note: As an exception to this rule, the stable-release manager may accept
- hardware-enabling "features". For example, backports of new code to support
- a newly-developed hardware product can be accepted if they can be reasonably
- determined to not have effects on other hardware.</li>
-
- <li>Patch is a performance optimization. As a rule, performance patches are
- not candidates for the stable branch. The only exception might be a case
- where an application's performance was recently severely impacted so as to
- become unusable. The fix for this performance regression could then be
- considered for a stable branch. The optimization must also be
- non-controversial and the patches still need to meet the other criteria of
- being simple and self-contained</li>
-
- <li>Patch introduces a new failure mode (such as an assert). While the new
- assert might technically be correct, for example to make Mesa more
- conformant, this is not the kind of "bug fix" we want in a stable
- release. The potential problem here is that an OpenGL program that was
- previously working, (even if technically non-compliant with the
- specification), could stop working after this patch. So that would be a
- regression that is unaacceptable for the stable branch.</li>
+<li><code>git rebase -i ...</code> is your friend. Don't be afraid to use it.
+<li>Apply a fixup to commit FOO.
+<pre>
+git add ...
+git commit --fixup=FOO
+git rebase -i --autosquash ...
+</pre>
+<li>Test for build breakage between patches e.g last 8 commits.
+<pre>
+git rebase -i --exec="ninja -C build/" HEAD~8
+</pre>
+<li>Sets the default mailing address for your repo.
+<pre>
+git config --local sendemail.to mesa-dev@lists.freedesktop.org
+</pre>
+<li> Add version to subject line of patch series in this case for the last 8
+commits before sending.
+<pre>
+git send-email --subject-prefix="PATCH v4" HEAD~8
+git send-email -v4 @~8 # shorter version, inherited from git format-patch
+</pre>
</ul>
projects / mesa.git / blobdiff