WineHQ

Submitting Patches: Difference between revisions

(→‎Testing for problems: "make crosstest" target no longer exists, tests are built as exe by default)
 
(7 intermediate revisions by 5 users not shown)
Line 1: Line 1:
'''''Translations of this page:''''' not yet ported. Translators, please see the Discussion page.
'''''Translations of this page:''''' not yet ported. Translators, please see the Discussion page.


== WineHQ patch submission guidelines ==
== Wine patch submission guidelines ==
In order to review code better and use established lines of communication, the Wine project accepts code in the form of patch files (instead of pushing directly to the main Git repository). The process may seem a little complicated at first, but it will probably become very natural after just one or two submissions.  
In order to review code better and use established lines of communication, the Wine project accepts code in the form of merge requests on the [https://gitlab.winehq.org WineHQ Gitlab]. The process may seem a little complicated at first, but it will probably become very natural after just one or two submissions.  


=== Can I submit patches? ===
=== Can I submit patches? ===
Line 13: Line 13:
  git config --global user.email "[email protected]"
  git config --global user.email "[email protected]"


The [[Git Wine Tutorial]] has more details, but the key point is to '''use your real name'''. This isn't just for developers to show a stronger commitment and establish trust, but also helps discourage irresponsible programmers from submitting unacceptable code (such as dissassembled Microsoft components).
The [[Git Wine Tutorial]] has more details, but the key point is to '''use your real name'''. This isn't just for developers to show a stronger commitment and establish trust, but also helps discourage irresponsible programmers from submitting unacceptable code (such as disassembled Microsoft components).


Finally, there's always a chance that someone has recently changed the same code you are working on. To make sure your code hasn't been superseded, be sure to update your Git repo before generating your patch files.
You will need a [https://gitlab.winehq.org/-/profile Gitlab profile] which must also use your real name.  Please be precise; having the names match exactly really helps identify commits.
 
You will need to [https://gitlab.winehq.org/wine/wine/-/forks/new fork a personal copy of Wine] and then clone that repository to your local system.
 
You can then work in that local tree in any branch you choose, and when you are ready, you push your work to your personal copy of Wine and request that your work be merged into Wine with a merge request.
 
Finally, there's always a chance that someone has recently changed the same code you are working on. To make sure your code hasn't been superseded, be sure to update your Git repo before submitting your merge request.


  git pull --rebase
  git pull --rebase
Line 23: Line 29:
== Making your changes ==
== Making your changes ==
=== Code guidelines ===
=== Code guidelines ===
:''See also [[Wine Developer's Guide/Coding Practice#Some notes about style]]''
If you are making changes to the Wine program itself, there are some '''basic rules your code should follow''' to pass review:


*When changing existing code, try to follow the style of the old code but keep in mind the rules for new code
There are two main guides for writing good C code for Wine: '''[[Wine Developer's Guide/Coding Practice#Some notes about style|Some notes about style]]''' and '''[[Wine Developer's Guide/Coding Practice#Writing portable code|Writing portable code]]'''. When changing existing code, try to follow the style of the old code, but be sure to follow best practices when writing new code. Note that copying and pasting old code is considered adding new code!
*Copy and pasting old code is considered adding new code!
*When adding new code:
**Use spaces instead of tabs
**Use underscores instead of camelCase, except if the Windows API requires it
**Avoid type names that start with LP (e.g. write <code>DWORD *</code> instead of <code>LPDWORD</code>)
**Don't use Hungarian notation (e.g. write <code>char *important_buffer</code> instead of <code>char *pImportantBuffer</code>)
*Where possible, [[Developer Hints#Using only C89-compliant code|use only portable, C89-compliant code]]
*Use a consistent style in your own changes (e.g. don't write <code>int* foo</code> in one place and <code>int *foo</code> in another)
*Limit type casts as much as possible (e.g. don't cast a <code>VOID *</code> to a <code>DWORD *</code>)
*Remove trailing whitespace from your changes, but don't change whitespace in other lines
*Avoid very long lines ([https://www.winehq.org/pipermail/wine-devel/2012-September/097024.html there is no hard limit], the preferred length is 100, but 120 or 80 chars are fine too)


As a general rule, only the simplest code possible is accepted into Wine. Spend some time looking over your changes and think about whether there is a more concise or straightforward way to do what you want.
As a general rule, only the simplest code possible is accepted into Wine. Spend some time looking over your changes and think about whether there is a more concise or straightforward way to do what you want.
Line 45: Line 37:
In order to make your patches as easy to check as possible, try to keep them '''small, clear, and atomic''':
In order to make your patches as easy to check as possible, try to keep them '''small, clear, and atomic''':


*Only include closely related changes in one patch
*Only include closely related changes in one commit
*If you've written a fix and a lot of new [[Conformance Tests|conformance tests]] for an issue, send the tests as a separate patch before the fix
*If you've written a fix and a lot of new [[Conformance Tests|conformance tests]] for an issue, add the tests as a separate commit before the fix
*Limit a patch to changing a single file or component unless that would break something somewhere else
*Limit a commit to changing a single file or component unless that would break something somewhere else
*Don't implement extra things that no one uses. To quote Wine's chief maintainer: "We don't add a dll unless some app wants it, we don't add a function unless some app calls it, we don't add a version resource unless some app checks it, etc." [https://gitlab.winehq.org/wine/wine/-/merge_requests/2045#note_22578]
*Limit each merge request to only a few commits (no more than 5 is a good guideline)


These rules also make it easier to [[Regression Testing|find regressions]], and help prevent new bugs from slipping through.
These rules make the merge request easier to review, and also make it easier to [[Regression Testing|find regressions]], and help prevent new bugs from slipping through.


You can also look at the [https://source.winehq.org/patches/ patch status page] to see the patches that have been submitted and committed recently. Try to imitate the style of the successful patches.
You can also look at the [https://gitlab.winehq.org/wine/wine/-/merge_requests?scope=all&state=merged accepted merge requests] to see the requests that have been submitted and committed recently. Try to imitate the style of the successful merge requests.


Related parts of the Wine project, such as the website, may have slightly looser requirements on patches, but it is still important to keep patches clear and simple, bundle related changes together, and not break anything.
Related parts of the Wine project, such as the website, may have slightly looser requirements on patches, but it is still important to keep patches clear and simple, bundle related changes together, and not break anything.
Line 67: Line 61:
If your patch is related to a bug, it's recommended to include a '''Wine-Bug line''' with a URL to Bugzilla (see the example below). Do not include the bug number in the first line.
If your patch is related to a bug, it's recommended to include a '''Wine-Bug line''' with a URL to Bugzilla (see the example below). Do not include the bug number in the first line.


Your patch must include a '''Signed-off-by line'''. ''Signed-off-by'' means "I think this code is good enough to go into Wine." It is an assertion that your work meets all legal and technical requirements of Wine development. <code>git commit -s</code> will add this line automatically for you.
You can include any '''extra information''' that you do ''not'' want included in the commit log in the Description field when creating the merge request on the website.
 
Finally, include any '''extra information''' that you do ''not'' want included in the commit log beneath three dashes (<code>---</code>). If you are submitting a revised version of a patch that you submitted previously, this is where to describe the changes you made since the last version and address any feedback from other developers.


A complete example:
A complete example:
Line 75: Line 67:
gdiplus: Avoid calling GdipFillPath() with an empty path.
gdiplus: Avoid calling GdipFillPath() with an empty path.


There is no point filling an empty path, and an empty
There is no point filling an empty path, and an empty path will cause
path will cause SelectClipPath() used in brush_fill_path()
SelectClipPath() used in brush_fill_path() to return an error.
to return an error.


Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=987654
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=987654
Signed-off-by: Zhiyi Zhang <[email protected]>
---
v2: Fix test breakage.
</pre>
</pre>


== Generating and submitting patches ==
== Submitting your changes ==
=== Creating patch files ===
The [[Git Wine Tutorial]] includes more details about the different ways to create patch files. In general you want:


git format-patch origin
=== Quality check ===
It never hurts to check through your changes one last time:
*Look at each line of the patch, make sure there are no unintentional or unrelated changes
*Did your fix pass all related tests? If it wasn't covered by a test, have you written a test of your own? 


A coding system is used in the subject line in order to sort submissions:
=== Sending the merge request ===
*The subject line should begin with <code>[PATCH]</code> (in brackets)
The [[Git Wine Tutorial]] includes more details about sending merge requests. In general you should push your changes to a temporary branch in your fork on Gitlab:
*If you are sending multiple patches that depend on each other, there should be a fraction inside the brackets, giving the patch's order and the total number of patches in the set (see also <code>-n</code> option of <code>git format-patch</code>):


  [PATCH 1/3] This is the first patch
  git push origin master:my-awesome-fix
[PATCH 2/3] This patch depends on the first one
[PATCH 3/3] This patch depends on the others


*Next, if you are submitting a patch for something other than Wine itself (such as the website or documentation), use <code>--subject-prefix="PATCH <project>"</code> to include the project code in the brackets:
and then go to the link that is displayed to finish the creation of the merge request.


[PATCH website] Add ... translation
=== Managing your submission ===
[PATCH bugzilla] Fix ...
After you've submitted your merge request, a reviewer will be assigned to it, if someone is responsible for that area of code. You can also assign a reviewer yourself if you know who the right person is. If a reviewer gives some feedback or a critique, carefully think about what they say and fix your patch before resubmitting. If others point out a few flaws in your patch, it's safe to assume that they missed some too so try to check the whole patch and fix other flaws before resubmitting.
[PATCH docs] Update ...
[PATCH appdb] Add feature ...
[PATCH tools] Fix ...
[PATCH fontforge] Fix ...
 
*Finally, if your submission somehow fell through the cracks and you're trying to send a patch again ''without changes'', use <code>--subject-prefix="PATCH resend"</code>. If the patch had errors and you've changed it before resubmitting, include which try you're on with <code>-v2</code>, <code>-v3</code>, etc.:
 
[PATCH resend] user32: This patch just got lost somehow
[PATCH website resend] This patch for the website got lost somehow
[PATCH v2] ntdll: This patch was wrong the first time
[PATCH v3] comctl32: This patch has failed twice
 
Note: if you are submitting a patch series then apply the same resubmit "version" code to the entire series (v2, v3, etc.) or it will confuse the [[Wine TestBot]], resulting in the automated tests not being run for all the patches in the series.
 
=== Quality check ===
It never hurts to check through your patches one last time:
*Look at each line of the patch, make sure there are no unintentional or unrelated changes
*Remember not to actually edit the patch file itself. If you find errors, change the code and generate a new patch file.
*Did your fix pass all related tests? If it wasn't covered by a test, have you written a test of your own? 


=== Sending the patch ===
When updating your changes, don't simply push the fixes as a new commit on top of your merge request. You need to actually modify and recreate the existing commits to make it look as if you got everything right on the first try. You can then update your merge request by force pushing to its branch:
The [[Git Wine Tutorial]] includes more details about the different ways to send patch files. In general you want:


  git send-email --to=wine-[email protected] *.patch
  git push --force origin master:my-awesome-fix


There are a few things to look out for when mailing in your patch. First, be sure you address the patch to the [https://www.winehq.org/mailman/listinfo/wine-devel wine-devel mailing list], [email protected], and remember that you need to be subscribed to the mailing list in order for your submission to get past the spam filter. If you have a long series, it is recommended to send no more than four or five patches at a time. This makes it easier for others to review your series, and makes resending your series if needed less cumbersome.
If your merge request remains in the queue for several weeks without being committed and without feedback, improving it (perhaps by adding more tests) and resubmitting it (after rebasing to the current tip) may help. You can also ask for suggestions on the [https://www.winehq.org/irc IRC channel] or the [https://www.winehq.org/pipermail/wine-devel/ wine-devel mailing list]. If you have patches rejected multiple times, don't be discouraged. You might consider finding a mentor willing to pre-review your changes before you submit them (see https://www.winehq.org/pipermail/wine-devel/2010-August/086207.html for a discussion of this)


As an additional guard that the patches you send all have a <code>Signed-off-by</code> line, you can create the following script as <code>.git/hooks/sendemail-validate</code> within your cloned repo. Be sure to set executable permissions on it.
=== Reviewing merge requests ===
If you are asked to review a merge request, and find things that need to be improved, you should add a comment to the merge request from the Gitlab UI. If you prefer to use email, you can reply to the Gitlab notification (if you are subscribed to these), or to the mails that were sent to the [https://www.winehq.org/mailman3/hyperkitty/list/wine-[email protected]/ wine-gitlab mailing list] (if you are following that).


<syntaxhighlight lang="shell">
You can also fix things up yourself and force-push updated commits to the original branch. At the moment this requires the submitter to explicitly give you access (we are hoping to lift that restriction).  
#!/bin/sh
test "" = "$(grep '^Signed-off-by: ' "$1")" && {
echo >&2 Missing Signed-off-by line.
exit 1
} || true
</syntaxhighlight>


If you think that the merge request is good to go in, you should mark it approved in Gitlab. As a rule, a merge request won't be committed until all reviewers have approved it. If you have been assigned as a reviewer but feel that you cannot meaningfully review the changes, feel free to unassign yourself, and maybe re-assign it to someone more knowledgeable in that area.


The <code>git send-email</code> command is strongly recommended, but if you can't get it to work, you may send the patch file as an attachment. Remember to only attach one patch per email and use the subject line from the patch file as the subject line in the email. Also, always send your emails as plain text, not HTML or rich text.
=== Responding to review ===


If you want to use other email clients to send patches inline, check out [https://www.kernel.org/doc/html/v4.10/process/email-clients.html#email-clients-info-for-linux Email clients info for Linux] for how they should be set up so that they don't mess with your patches.
Sometimes, this is a trivial process. A reviewer points out an error in your patch which you missed, or some way in which the code style differs from the norm in Wine. In this case all you need to do is fix up that part of the patch and resend.


=== Managing your submission ===
Sometimes review is less trivial, and may result in extended discussion. Here are some things to keep in mind to keep a good relationship with your reviewers:
After you've submitted you're patch, you should watch the [https://www.winehq.org/pipermail/wine-devel/ wine-devel mailing list] for feedback on your patch (although replies should normally be cc'd to you, sometimes people may forget). If your patch does get some feedback or a critique, carefully think about what they say and fix your patch before resubmitting. If others point out a few flaws in your patch, it's safe to assume that they missed some too so try to check the whole patch and fix other flaws before resending.


Also be sure to watch the [https://source.winehq.org/patches/ patch status page] to see how far it has gone. The maintainer usually commits twenty or so patches all at once at the end of the day. If your patch had no negative feedback, the maintainer will have a look at it as soon as he can. Patches from those who have successfully committed patches before, and who are known to test their work thoroughly, are likely to be committed quickly. Patches from new developers require more scrutiny and the maintainer will probably not get to them immediately, but this does not mean your patch has been forgotten.
* '''Assume positive intent.''' You will be interacting with contributors from many cultures, most of whom are not native English speakers. Sometimes communication can come off as curt or impolite without meaning to.
 
* Reviewers may phrase suggestions as questions, but on the other hand, questions don't always mean that you need to change your code. On the ''other'' hand, if a question is asked once, it might be asked again, in the future, by a newcomer trying to understand the code (or by someone who's forgotten it).
If your patch disappears from https://source.winehq.org/patches/ without being committed and without feedback, improving it (perhaps by adding more tests) and resending it may help, or you can ask for suggestions on the [https://www.winehq.org/irc IRC channel] or the developers' mailing list mentioned above. If you have patches rejected multiple times, don't be discouraged. You might consider finding a mentor willing to pre-review your patches before you send them to wine-devel (see https://www.winehq.org/pipermail/wine-devel/2010-August/086207.html for a discussion of this)
* Try to understand the reviewer's perspective. Often you will spend a long time working on a code, polishing it to perfection, rewriting it multiple times. When you submit that patch, you are the world's expert on that code, and it can be frustrating to receive feedback asking that it be done differently, or questioning your decisions. Try to understand where that feedback is coming from.
:* Being the world's expert on a piece of code is a double-edged sword. You will not always be as familiar with that code as you are now. Often a reviewer will see parts of code as complex and difficult to understand that you understand perfectly. This can be an asset: the code should be comprehensible to you, not just now, but in the future as well, and it should also be comprehensible to anyone who's starting to work on that component.
:* You may be the expert on the code which you are submitting, but that doesn't always translate to understanding the way it interacts with other code. A maintainer's job is to understand these interactions.
:* People make mistakes, and have blind spots. That applies to both you and the reviewer. If the reviewer makes a mistake, or asks a question the answer to which seems obvious to you, try to be charitable. On the other hand, if a reviewer asks for some change and this change seems disadvantageous to you, it's fine to explain why—maybe there's something they missed.
:* Perhaps most importantly, try to avoid being argumentative. People do not enjoy reviewing patches when they feel like they are having an argument with the submitter, and this can lead to discussions and patches being ignored, sometimes even preemptively.
:: This is not the same thing as "don't argue". Part of not being argumentative is being open-minded to suggestions, and willing to change your patch according to the maintainer's preferences. A more important part of it, though, is understanding and acknowledging why the suggestion was made, and explaining why it's not the best approach. Work *with* the maintainer, not against them. Verbally acknowledging the tradeoffs that lead you to choose a certain approach over that suggested by the maintainer, or acknowledging why their suggested approach would be preferable even if it's not actually possible, can do a lot to keep review enjoyable for both parties. Even phrasing like "unfortunately we can't do it that way, because X" can go a long way.


== Special situations ==
== Special situations ==
=== Submitting patches written by somebody else ===
=== Submitting patches written by somebody else ===
Occasionally, the author of a useful patch stops working on it, and somebody else has to pick it up and continue. In this case, commit the author's original patch file to your local tree (<code>git am</code>) and add two additional lines to the commit message (<code>git commit --amend</code>):
Occasionally, the author of a useful patch stops working on it, and somebody else has to pick it up and continue. In this case, commit the author's original patch file to your local branch (<code>git am</code>), making sure that the Author: field of the commit is set to the original author, and submit a merge request. You'll be asked to approve the merge request to show that you have checked the original author's work.
*A <code>From:</code> line with the original author's name and email address. <code>git send-email</code> will add this automatically if the patch file has the original author's name and email instead of your own.
*A <code>Signed-off-by:</code> line to indicate that you (not the original author) assert that the patch is good enough to go into Wine.


=== Coauthored patches ===
=== Coauthored patches ===
If someone else has made a significant contribution to the patch, first try to split up the patch so that each patch has only one author. If that is impractical, explain in the commit message who you collaborated with on the patch, but don't add a Signed-off-by line for anyone but yourself.
If someone else has made a significant contribution to the patch, first try to split up the patch so that each patch has only one author. If that is impractical, explain in the commit message who you collaborated with on the patch, but keep yourself as the author.


Both reviewers and coauthors indicate approval of a patch by sending a Signed-off-by line to the wine-devel mailing list. For example:
=== Multiple outstanding merge requests ===
 
Wine's chief maintainer, Alexandre Julliard, says you should concentrate on one thing and get it in before trying others.  He may ignore some of your merge requests if he feels you have too many balls in the air.
<pre>
From: Nikolay Sivov <[email protected]>
Subject: Re: gdiplus: Avoid calling GdipFillPath() with an empty path.
 
Signed-off-by: Nikolay Sivov <[email protected]>
</pre>


=== Multiple outstanding patchsets ===
=== Introducing new dependencies ===
Alexandre says you should concentrate on one thing and get it in before trying others. He may ignore some of your patchsets if he feels you have too many balls in the air.
Sometimes new Wine functionality requires adding new build dependencies on Linux libraries. When that happens:
* Your merge request must still compile and not break Wine or the Wine tests even if the libraries are missing from the build environment.
* You are not required to send a patch to update the GitLab CI environment.
* Sending a patch to update the build environment is helpful though. If doing so it must be sent in a separate merge request with no other change. Also note that the GitLab CI environment will only be updated, manually, after that merge request is merged. This is to ensure that all merge requests go through a well known and verified environment.


----
----
[[Category:Development]]
[[Category:Development]]

Latest revision as of 13:58, 24 September 2023

Translations of this page: not yet ported. Translators, please see the Discussion page.

Wine patch submission guidelines

In order to review code better and use established lines of communication, the Wine project accepts code in the form of merge requests on the WineHQ Gitlab. The process may seem a little complicated at first, but it will probably become very natural after just one or two submissions.

Can I submit patches?

Not everyone can contribute code to Wine, primarily because doing so would violate the Clean Room Guidelines. If you have studied the Microsoft Windows source code, even if you aren't under a non-disclosure agreement, your patches probably won't be accepted. This is to ensure that the Wine project does not violate any copyrights belonging to Microsoft. Some more notes related to this can be found in the Developer FAQ.

Check your Git setup

It is important to make sure the author and email settings for your Git repo are configured correctly.

git config --global user.name "Your Name"
git config --global user.email "[email protected]"

The Git Wine Tutorial has more details, but the key point is to use your real name. This isn't just for developers to show a stronger commitment and establish trust, but also helps discourage irresponsible programmers from submitting unacceptable code (such as disassembled Microsoft components).

You will need a Gitlab profile which must also use your real name. Please be precise; having the names match exactly really helps identify commits.

You will need to fork a personal copy of Wine and then clone that repository to your local system.

You can then work in that local tree in any branch you choose, and when you are ready, you push your work to your personal copy of Wine and request that your work be merged into Wine with a merge request.

Finally, there's always a chance that someone has recently changed the same code you are working on. To make sure your code hasn't been superseded, be sure to update your Git repo before submitting your merge request.

git pull --rebase

If you prefer to manage your work in separate Git branches, remember that the above command will only update your current branch and all of your branches need to be up-to-date. Keeping your "master" branch in sync with WineHQ's repository and creating branches (and rebasing them on "master") for your own changes might be a good idea. Again, the Git Wine Tutorial page goes into much more detail on using Git to manage your work.

Making your changes

Code guidelines

There are two main guides for writing good C code for Wine: Some notes about style and Writing portable code. When changing existing code, try to follow the style of the old code, but be sure to follow best practices when writing new code. Note that copying and pasting old code is considered adding new code!

As a general rule, only the simplest code possible is accepted into Wine. Spend some time looking over your changes and think about whether there is a more concise or straightforward way to do what you want.

Patch guidelines

In order to make your patches as easy to check as possible, try to keep them small, clear, and atomic:

  • Only include closely related changes in one commit
  • If you've written a fix and a lot of new conformance tests for an issue, add the tests as a separate commit before the fix
  • Limit a commit to changing a single file or component unless that would break something somewhere else
  • Don't implement extra things that no one uses. To quote Wine's chief maintainer: "We don't add a dll unless some app wants it, we don't add a function unless some app calls it, we don't add a version resource unless some app checks it, etc." [1]
  • Limit each merge request to only a few commits (no more than 5 is a good guideline)

These rules make the merge request easier to review, and also make it easier to find regressions, and help prevent new bugs from slipping through.

You can also look at the accepted merge requests to see the requests that have been submitted and committed recently. Try to imitate the style of the successful merge requests.

Related parts of the Wine project, such as the website, may have slightly looser requirements on patches, but it is still important to keep patches clear and simple, bundle related changes together, and not break anything.

Testing for problems

Be sure to run the conformance tests on an unchanged branch of Wine (or before you make any changes) to see where your system is OK. Then run the tests again after you make your changes to verify that they don't break anything new. If you've added or modified any of the conformance tests, be sure to check that they work properly by using the make test command, then submitting the resulting executable to the Wine Test Bot.

Also, make note of any significant compiler warnings that come up when building your modified version of Wine and fix the underlying cause before submitting your patch.

The commit message

In the first line of the commit message, write the name of the component you changed, followed by a colon, one sentence in the imperative mood that will appear in the release notes, and a period. Be brief: the first line should ideally not be more than 72 characters in length.

After the first line, you may add explanatory paragraphs that tell what the patch accomplishes. Describe your changes in the imperative mood, as if you are giving orders to the codebase to change its behavior, e.g. Make xyzzy do frotz. instead of This patch makes xyzzy do frotz. or Changed xyzzy to do frotz. Try to make sure your explanation can be understood without external resources. Instead of giving a URL to a mailing list archive, summarize the relevant points of the discussion. Describe how the patch works and why it is needed, but do not repeat what the diff already shows. Wrap all paragraph lines to 72 characters.

If your patch is related to a bug, it's recommended to include a Wine-Bug line with a URL to Bugzilla (see the example below). Do not include the bug number in the first line.

You can include any extra information that you do not want included in the commit log in the Description field when creating the merge request on the website.

A complete example:

gdiplus: Avoid calling GdipFillPath() with an empty path.

There is no point filling an empty path, and an empty path will cause
SelectClipPath() used in brush_fill_path() to return an error.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=987654

Submitting your changes

Quality check

It never hurts to check through your changes one last time:

  • Look at each line of the patch, make sure there are no unintentional or unrelated changes
  • Did your fix pass all related tests? If it wasn't covered by a test, have you written a test of your own?

Sending the merge request

The Git Wine Tutorial includes more details about sending merge requests. In general you should push your changes to a temporary branch in your fork on Gitlab:

git push origin master:my-awesome-fix

and then go to the link that is displayed to finish the creation of the merge request.

Managing your submission

After you've submitted your merge request, a reviewer will be assigned to it, if someone is responsible for that area of code. You can also assign a reviewer yourself if you know who the right person is. If a reviewer gives some feedback or a critique, carefully think about what they say and fix your patch before resubmitting. If others point out a few flaws in your patch, it's safe to assume that they missed some too so try to check the whole patch and fix other flaws before resubmitting.

When updating your changes, don't simply push the fixes as a new commit on top of your merge request. You need to actually modify and recreate the existing commits to make it look as if you got everything right on the first try. You can then update your merge request by force pushing to its branch:

git push --force origin master:my-awesome-fix

If your merge request remains in the queue for several weeks without being committed and without feedback, improving it (perhaps by adding more tests) and resubmitting it (after rebasing to the current tip) may help. You can also ask for suggestions on the IRC channel or the wine-devel mailing list. If you have patches rejected multiple times, don't be discouraged. You might consider finding a mentor willing to pre-review your changes before you submit them (see https://www.winehq.org/pipermail/wine-devel/2010-August/086207.html for a discussion of this)

Reviewing merge requests

If you are asked to review a merge request, and find things that need to be improved, you should add a comment to the merge request from the Gitlab UI. If you prefer to use email, you can reply to the Gitlab notification (if you are subscribed to these), or to the mails that were sent to the wine-gitlab mailing list (if you are following that).

You can also fix things up yourself and force-push updated commits to the original branch. At the moment this requires the submitter to explicitly give you access (we are hoping to lift that restriction).

If you think that the merge request is good to go in, you should mark it approved in Gitlab. As a rule, a merge request won't be committed until all reviewers have approved it. If you have been assigned as a reviewer but feel that you cannot meaningfully review the changes, feel free to unassign yourself, and maybe re-assign it to someone more knowledgeable in that area.

Responding to review

Sometimes, this is a trivial process. A reviewer points out an error in your patch which you missed, or some way in which the code style differs from the norm in Wine. In this case all you need to do is fix up that part of the patch and resend.

Sometimes review is less trivial, and may result in extended discussion. Here are some things to keep in mind to keep a good relationship with your reviewers:

  • Assume positive intent. You will be interacting with contributors from many cultures, most of whom are not native English speakers. Sometimes communication can come off as curt or impolite without meaning to.
  • Reviewers may phrase suggestions as questions, but on the other hand, questions don't always mean that you need to change your code. On the other hand, if a question is asked once, it might be asked again, in the future, by a newcomer trying to understand the code (or by someone who's forgotten it).
  • Try to understand the reviewer's perspective. Often you will spend a long time working on a code, polishing it to perfection, rewriting it multiple times. When you submit that patch, you are the world's expert on that code, and it can be frustrating to receive feedback asking that it be done differently, or questioning your decisions. Try to understand where that feedback is coming from.
  • Being the world's expert on a piece of code is a double-edged sword. You will not always be as familiar with that code as you are now. Often a reviewer will see parts of code as complex and difficult to understand that you understand perfectly. This can be an asset: the code should be comprehensible to you, not just now, but in the future as well, and it should also be comprehensible to anyone who's starting to work on that component.
  • You may be the expert on the code which you are submitting, but that doesn't always translate to understanding the way it interacts with other code. A maintainer's job is to understand these interactions.
  • People make mistakes, and have blind spots. That applies to both you and the reviewer. If the reviewer makes a mistake, or asks a question the answer to which seems obvious to you, try to be charitable. On the other hand, if a reviewer asks for some change and this change seems disadvantageous to you, it's fine to explain why—maybe there's something they missed.
  • Perhaps most importantly, try to avoid being argumentative. People do not enjoy reviewing patches when they feel like they are having an argument with the submitter, and this can lead to discussions and patches being ignored, sometimes even preemptively.
This is not the same thing as "don't argue". Part of not being argumentative is being open-minded to suggestions, and willing to change your patch according to the maintainer's preferences. A more important part of it, though, is understanding and acknowledging why the suggestion was made, and explaining why it's not the best approach. Work *with* the maintainer, not against them. Verbally acknowledging the tradeoffs that lead you to choose a certain approach over that suggested by the maintainer, or acknowledging why their suggested approach would be preferable even if it's not actually possible, can do a lot to keep review enjoyable for both parties. Even phrasing like "unfortunately we can't do it that way, because X" can go a long way.

Special situations

Submitting patches written by somebody else

Occasionally, the author of a useful patch stops working on it, and somebody else has to pick it up and continue. In this case, commit the author's original patch file to your local branch (git am), making sure that the Author: field of the commit is set to the original author, and submit a merge request. You'll be asked to approve the merge request to show that you have checked the original author's work.

Coauthored patches

If someone else has made a significant contribution to the patch, first try to split up the patch so that each patch has only one author. If that is impractical, explain in the commit message who you collaborated with on the patch, but keep yourself as the author.

Multiple outstanding merge requests

Wine's chief maintainer, Alexandre Julliard, says you should concentrate on one thing and get it in before trying others. He may ignore some of your merge requests if he feels you have too many balls in the air.

Introducing new dependencies

Sometimes new Wine functionality requires adding new build dependencies on Linux libraries. When that happens:

  • Your merge request must still compile and not break Wine or the Wine tests even if the libraries are missing from the build environment.
  • You are not required to send a patch to update the GitLab CI environment.
  • Sending a patch to update the build environment is helpful though. If doing so it must be sent in a separate merge request with no other change. Also note that the GitLab CI environment will only be updated, manually, after that merge request is merged. This is to ensure that all merge requests go through a well known and verified environment.

This page was last edited on 24 September 2023, at 13:58.