SubmittingPatches

Translations of this page: http://upload.wikimedia.org/wikipedia/en/thumb/f/f3/Flag_of_Russia.svg/22px-Flag_of_Russia.svg.png Русский

WineHQ Patch submission guidelines

Contents

  1. Patch Quality Checklist
  2. Use your real name
  3. Don't look at any Microsoft source or object code
  4. Try to imitate wine-patches style
  5. Patch workflow
  6. Generating patches
  7. Files to include
  8. Subject lines
  9. Include a description
  10. Small, clear and atomic changes
  11. Use the latest Wine GIT tree
  12. Test your patch!
  13. Know your mail client
  14. Avoid reformatting
  15. Avoid C++, C99 and non-portable C constructs
  16. Submitting patches written by somebody else
  17. See Also

Patch Quality Checklist

  • remove tabs and trailing whitespace in your changes, but don't change whitespace in unrelated parts of source files
  • look at each line of the patch, make sure there are no unintentional or unrelated changes
  • remove C++ comments (use /* */, never // )
  • use consistent style (e.g. don't write foo* bar in one place, and foo *bar in another)
  • follow the style of surrounding code when practical
  • avoid unneeded casts (e.g. don't cast an LPVOID to a DWORD *)
  • understand any compiler warnings, and fix root cause
  • run the test suite
  • test with winetestbot
  • send patch as attachment
  • don't combine unrelated changes in one patch
  • don't touch multiple dlls with one patch unless you really have to
  • only one attachment per post to wine-patches
  • If someone points out a few flaws in your patch, assume he did not list every flaw he knows about, and try to find and fix other flaws before resubmitting

  • If you have patches rejected multiple times, consider finding a mentor willing to pre-review your patches before you send them to wine-patches (see also http://www.winehq.org/pipermail/wine-devel/2010-August/086207.html )

Use your real name

The Wine project will not accept patches from an anonymous person.

Don't look at any Microsoft source or object code

See CleanRoomGuidelines for some tips on how to help keep the Wine source tree clean

Try to imitate wine-patches style

Look at the wine-patches mailing list archives to get an idea of how most people submit patches, and also look at the wine-cvs mailing list archives to see which patches actually get committed. Then imitate the style of the successful patches.

Patch workflow

Before making any changes, run the ConformanceTests to make sure your system is OK. (See MakeTestFailures for a few notes on common problems.) Then run them again after you make your changes to verify you didn't break anything.

Test your changes (and if you changed any conformance tests, test them on Windows, e.g., using WineTestBot), then generate a patch and send it to the wine-patches mailing list.

Watch the wine-devel mailing list for feedback on your patch. (The messages should be cc'd to you, but sometimes they aren't.)

If anybody replies to your patch, address their feedback carefully and send your patch(es) again.

Watch the patch status page to see the status of your patch. The maintainer usually commits twenty or so patches all at once at the end of the day (European time).

If your patch had no negative feedback, the maintainer will have a look at it as soon as he can. Patches from people 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, so the maintainer might not be able to look at them immediately.

If your patch disappears from http://source.winehq.org/patches/ without being committed, improve it (perhaps by adding more tests) and resend, or ask on IRC/wine-devel for suggestions.

Generating patches

Please use Git to generate your patches.

It's best to send small patches against the current tree using Git. First, make sure your tree is up to date, as described on the Git page.

Check in your patches to your Git tree using git commit.

If you're going to use git imap-send make sure that you've set up the proper options in your Git config file, then run: 

git format-patch --stdout --keep-subject --attach origin | git imap-send

That should upload your patches into your IMAP drafts folder, and allow you to check and send them. In Mozilla Thunderbird, that is as simple as clicking "Edit Draft..." then "Send" if the mail headers in the Git config file are set correctly. See "Sending the patches using imap" for more information on IMAP config settings.

If you can't use IMAP, use the following command to generate patches: 

git format-patch --keep-subject origin

That should generate a number of .txt files, which you can send manually. (Please use the .txt extension as it makes the patches easier to review.)

For instance: 

git fetch; git rebase origin                            # make sure you're up to date
./configure && make depend && make                      # rebuild the tree
cd dlls/msvcrt                                          # change to directory of interest
rm -rf ~/.wine                                          # be sure to test in a clean WINEPREFIX
cd tests; make testclean; make test; cd ..              # make sure all tests pass there before your changes
vi file.c                                               # change some stuff
make                                                    # check that there's no errors or warnings
rm -rf ~/.wine                                          # be sure to test in a clean WINEPREFIX
cd tests; make testclean; make test; cd ..              # make sure all tests pass there after your changes
cd ../..
git commit -a                                           # check in your changes to your local repository
git format-patch --keep-subject origin                  # generate patches

This will generate files 0001-yourcommitname.patch, 0002-yourcommitname.patch, etc. Look over the patches carefully, and adjust the subject line if needed (for instance, number the patches in a series as described below). Then send the patches to wine-patches@winehq.org, one per message.

Files to include

Include a single diff of all the files you changed. Do not include diffs for files that are automatically generated and are not stored in Git such as: 

include/config.h
*/Makefile

You do not need to include diffs for automatically generated or updated files either. If you are using Git, check-in changes to these files in a separate commit. This category includes: 

configure
po/*.po except if you made the changes manually

Subject lines

There are a number of conventions regarding subject lines:

Start with the component you're changing and a colon, like this: 

msvcrt/tests: Test fopen of left-handed files.

For Wine related projects like the website, bugzilla, documentation, AppDB, tools or our fontforge use the accurate prefix in that style: 

[website] Added ... translation
[bugzilla] Fix ...
[docs] Updated ...
[appdb] Add Feature ...
[tools] Fix ...
[fontforge] Fix ...

If you're sending a series of patches which depend on each other, number them like this: 

[1/2] msvcrt/tests: Test fopen of left-handed files.
[2/2] msvcrt: Fix fopen of left-handed files.

If you're resending a patch unchanged, add "(resend)" to the end of the subject line.

If you're resending a patch with some changes, add "(try 2)" to the end of the subject line, and describe what the differences are with the previous version of the patch.

Include a description

Your email should describe what change you made and why you made the change. It should mention what operating systems you ran the tests on. If this is an updated version of the patch, also say how it changed since the last version. If you're addressing somebody else's feedback, say so.

If you're working on a bug in bugzilla, mention the bug number, and consider filing a bug if none exists.

When mentioning the bug number, do not make it part of the commit header. Examples:

Do this: 

msvcrt: Change a corner case in the handling of flags X Y Z (with tests).
        This resolves the issue from bug #4711.

Not this: 

msvcrt: Change the handling of flags X Y Z because this fixes bug #4711

Small, clear and atomic changes

Keep the patch small and the point of the change clear. Small patches can be reviewed and applied more quickly than large changes, and they are. Make sure your patches are "atomic" (each patch should leave the tree in a sane state). Some of your patches may not be applied, but Wine should keep working, and not contain superfluous code despite this.

Small and atomic patches also make regression analysis easier. To do a regression analysis, we need to know at all points in the GIT tree that Wine compiled and functioned.

This also means you should submit one fix or group of related changes per patch. Accompanying tests should be in the same patch as the fix or change itself, though.

Use the latest Wine GIT tree

Generate your patch against the latest version of Wine from the WineHQ GIT tree. Make sure your source is updated before sending the patch, as patches that don't apply cleanly will probably be silently ignored.

Test your patch!

Did you compile it after that last change you made? You'd be surprised what can happen in last minute edits ... ;-)

Does it really do what you think it does? A good way to show that is to write a test case demonstrating it is correct.

Test cases show up many problems with code, and it will save you embarrassing "brown paper bag" experiences. It also means that the bug you solved will not creep back in because somebody else didn't understand it.

If you fixed a bug or added a feature, but didn't add a test to cover that case, please consider doing so.

If you added tests, did you make sure they pass on Windows? Use 'make crosstest' to build windows versions of the test, and submit the executable to WineTestBot to run it on many versions of Windows at once. (If you have a Windows machine locally, run it there first to avoid unneeded load on the test server.) Alternatively, ask on the developer mailing list.

If you changed the build machinery, did you verify that it still works with pmake and not just gnu make? (Don't want to break Mac, BSD or Solaris!)

Know your mail client

Be careful to not corrupt the patch through line-wrapping. Some mailers let you enclose the patch in the body of the mail, and won't mess up the formatting. Mozilla isn't one of those! If you use Mozilla Thunderbird or the Mozilla suite, make sure to attach your patches so it doesn't wrap the lines.

Avoid reformatting

My C coding style is the best! But it's not the same as yours. So, how about we just agree to disagree?

When you edit a piece of code, preserve the original formatting. The only thing worse than the wrong coding style is 10 mixed coding styles, so let's just stick to one bad one per file.

It's good to read through your diff, and check that there's no unnecessary diff chunks in it. Removing useless chunks will reduce the size of your diff, which is also good (see above).

In general, unless the file you're working on using tab formatting, avoid tabs in Wine code. If you have to use tabs, a tab is defined as 8 spaces. If you use ts=4 in your editor to make it look better, remember to go back and make sure things look sane with ts=8 before submitting your patch. Tabs generally suck, and you shouldn't use them.

Avoid C++, C99 and non-portable C constructs

Wine adheres to standard C89/C90. Code should be written so that it works with as many compilers as possible, so you should avoid compiler-specific constructs, C99 and C++. 

int foo()                        /* bad: use 'int foo(void)' instead */
{
    bar();
    int ch;                      /* bad: inline variable declaration */
    ch = baz();
    // does something            /* bad: C99/C++ style comment */
    for (int i = 0; i < 4; i++)  /* bad: inline variable declaration */
        putc('a' + i);
    ch += offset ?: size         /* bad: gcc extension, use 'offset ? offset : size' */
}

void bar(void) __attribute__((noreturn))  /* bad: not portable; use        */
{                                         /*     DECLSPEC_NORETURN instead */
    long x;               /* bad: long is different in Win64 and Unix64; use */
                          /*     int or LONG instead                         */
    int y = (int) &x;     /* bad: in 64-bit mode, the pointer will be truncated; */
                          /*     use INT_PTR or LONG_PTR instead                 */
    ExitProcess(0);
}

int baz(void)
{
    wchar_t str[6];                   /* bad: may be 32-bit in Unix; use WCHAR instead */

    const WCHAR str1[] = L"Hello";    /* bad: L"" uses wchar_t; use syntax below */
    const WCHAR str2[] = {
       'H','e','l','l','o',0
    };

    const TCHAR str3[] = { 'A', 0 };  /* bad: use WCHAR explicitly */
    lstrcpy(str, str2);               /* bad: use -W form explicitly */

    return L'\0';                     /* bad: L'' uses wchar_t; use '\0' or 0 */
}

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.

If you're simply resending a patch somebody else wrote, put a 'From:' line with the original author's name and email address in the body of the message, in the same format as it would have been on the original mail.

The best way is to commit the author's original patch to your tree with the correct authorship information, and then send the complete git-format-patch output with all the headers in the body of your mail. If the Wine maintainer's commit script detects an embedded git commit in the mail body it will ignore the actual SMTP headers in favor of the git info.

See Also

SubmittingPatches (last edited 2012-05-12 05:51:43 by jefflatimer)