<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>
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">mesa-dev</a>
-for <a href="#reviewing">review</a> using <code>git send-email</code>.
+<li>Patches should be <a href="#submit">submitted</a>
+to <a href="#mailing">mesa-dev</a> or with
+a <a href="#merge-request">merge request</a>
+for <a href="#reviewing">review</a>.
</ul>
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.
</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 <pre>Closes: #1</pre>, since the url makes it
+easier to get to the bug page from <pre>git log</pre></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
<li>In order for your patch to reach the prospective reviewer easier/faster,
use the script scripts/get_reviewer.pl to get a list of individuals and include
them in the CC list.
-<br>
+<p>
Please use common sense and do <strong>not</strong> blindly add everyone.
-<br>
+</p>
<pre>
- $ scripts/get_reviewer.pl --help # to get the the help screen
+ $ scripts/get_reviewer.pl --help # to get the help screen
$ scripts/get_reviewer.pl -f src/egl/drivers/dri2/platform_android.c
- Rob Herring <robh@kernel.org> (reviewer:ANDROID EGL SUPPORT,added_lines:188/700=27%,removed_lines:58/283=20%)
- Tomasz Figa <tfiga@chromium.org> (reviewer:ANDROID EGL SUPPORT,authored:12/41=29%,added_lines:308/700=44%,removed_lines:115/283=41%)
- Emil Velikov <emil.l.velikov@gmail.com> (authored:13/41=32%,removed_lines:76/283=27%)
+ Rob Herring <robh@kernel.org> (reviewer:ANDROID EGL SUPPORT,added_lines:188/700=27%,removed_lines:58/283=20%)
+ Tomasz Figa <tfiga@chromium.org> (reviewer:ANDROID EGL SUPPORT,authored:12/41=29%,added_lines:308/700=44%,removed_lines:115/283=41%)
+ Emil Velikov <emil.l.velikov@gmail.com> (authored:13/41=32%,removed_lines:76/283=27%)
</pre>
</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>
to check for regressions.
</p>
+<p>
+As mentioned at the begining, 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:
+Patches may be submitted to the Mesa project by
+<a href="#mailing">email</a> or with a
+GitLab <a href="#merge-request">merge request</a>. To prevent
+duplicate code review, only use one method to submit your changes.
+</p>
+
+<h3 id="mailing">Mailing Patches</h3>
+
+<p>
+Patches may 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
state of your old patches to Superseded.
</p>
+<p>
+Some companies' mail server automatically append a legal disclaimer,
+usually containing something along the lines of "The information in this
+email is confidential" and "distribution is strictly prohibited".
+</p>
+<p>
+These legal notices prevent us from being able to accept your patch,
+rendering the whole process pointless. Please make sure these are
+disabled before sending your patches. (Note that you may need to contact
+your email administrator for this.)
+</p>
+
+<h3 id="merge-request">GitLab Merge Requests</h3>
+
+<p>
+ <a href="https://gitlab.freedesktop.org/mesa/mesa">GitLab</a> Merge
+ Requests (MR) can also be used to submit patches for Mesa.
+</p>
+
+<p>
+ If the MR may have interest for most of the Mesa community, you can
+ send an email to the mesa-dev email list including a link to the MR.
+ Don't send the patch to mesa-dev, just the MR link.
+</p>
+<p>
+ 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>
+ 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>
+ To participate in code review, you should monitor the
+ <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">
+ mesa-dev</a> email list and the GitLab
+ Mesa <a href="https://gitlab.freedesktop.org/mesa/mesa/merge_requests">Merge
+ Requests</a> page.
+</p>
+
<p>
When you've reviewed a patch on the mailing list, please be unambiguous
about your review. That is, state either
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>
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.
</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>
Here are some examples of such a note:
</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>
+<pre>
+CC: <mesa-stable@lists.freedesktop.org>
+</pre>
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.
+the commit for all the active stable branches. If the commit is not applicable
+for said branch the stable-release manager will reply stating so.
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.
+won't have any negative effect on the patch nomination.
<p>
Note: by removing the tag [as the commit is pushed] the patch is
<strong>explicitly</strong> rejected from inclusion in the stable branch(es).
-<br>
Thus, drop the line <strong>only</strong> if you want to cancel the nomination.
</p>
+Alternatively, if one uses the "Fixes" tag as described in the "Patch formatting"
+section, it nominates a commit for all active stable branches that include the
+commit that is referred to.
+
<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.
+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.
-The stable-release manager will work with the list of nominated patches, and
-for each patch that meets the criteria 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 commit ID of the original
-patch.
+ 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.
+
+A summary of all the picked/rejected patches will be presented in the
+<a href="releasing.html#prerelease">pre-release</a> announcement.
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. In which
+case he/she should provide a comment about the changes required, alongside the
+<code>Conflicts</code> section. Summary of which will be provided in the
+<a href="releasing.html#prerelease">pre-release</a> announcement.
+</p>
-<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 unacceptable for the stable branch.</li>
-</ul>
+<p>
+Developers are interested in sending backports are recommended to use either a
+<code>[BACKPORT #branch]</code> subject prefix or provides similar information
+within the commit summary.
+</p>
<h2 id="gittips">Git tips</h2>
</pre>
<li>Test for build breakage between patches e.g last 8 commits.
<pre>
- git rebase -i --exec="make -j4" HEAD~8
+ git rebase -i --exec="ninja -C build/" HEAD~8
</pre>
<li>Sets the default mailing address for your repo.
<pre>
projects / mesa.git / blobdiff