Coding Hints:Patches

From Wine-Wiki.org
Revision as of 11:04, 21 September 2010 by Josephblack (Talk | contribs)
(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

Contents

Submitting Patches to Wine

Sending patches to most open source projects can be challenging for anyone new to the project. By looking over previous patch reviews, you can learn from the developers suggestions. Read through the following for many tips and suggestions for sending patches to wine. You can list further examples here.


Other pages of interest may be:

This is not official documentation or a guideline, but rather a compilation of comments from the mailing lists.

While reading this keep in mind that views, practices and opinions change. If you spot something out of date - a fix or a comment is appreciated.


Posting patches to the Wine developer Mailing List for review is always a good start, and you could post to the list and try asking for a mentor. Oliver Stieber: anyone who has any difficulty getting any patches committed should just ask for some help on wine-devel and I'm sure someone will be willing to help them out (If not just Email me and I'll give you a hand!). Roles seem to change a lot, so I'm not sure that appointing anyone is a good idea (appart from the default maintainer of for categories of bugs), you can always look at the code or http://www.winehq.org/site/status and find whoever has submited the recient changes, it may be initially daunting but I'm sure everyone's glad someone lends a hand. L. Ulmer: most patches to 'non-core' DLLs (DirectX in my case) are applied without much problem by Alexandre (as long as no other DirectX 'core' hackers do not complain when the patch is sent to wine-patches). S. Edwards: CC's of patches [could be] sent to certain developers along with wine-patches.

M. McCormack then explained a downside of not working to get patches officially merged: If you choose the "unmergable" option, you'll soon find that you're not able to merge the Wine community's contributions back into your tree. J. White: the [Codeweavers] use of the word "Proprietary Advantage" is an inside joke. It usually refers to code that is so bad that there is no chance Alexandre would accept it into WineHQ. [...] Our preference has been and continues to be that everything we do go into Wine first; that's the only way we get the peer review that can make Wine work well. But I have generally found that Alexandre's instincts are quite good. He's also very reasonable; I've never found him unwilling to discuss an issue, and I think I've even managed to change his mind on occasion.

"The Correct Patch'

M. Hearn [May 05] briefly described a 'correct' patch : Unified diff from the source root, test cases, clear explanation, good comments, no extra changes ...Wine archives

Z. Goldberg [wine devel aug 08]: Your patch is more likely to be accepted if it comes with a test which works on windows, didnt pass in wine before but passes now and demonstrates why the new behavior is correct. [There are possible exceptions for really obvious patches as noted by D. Timoshkov]: There is no need for a test [...] that's not really an implementation but just a better stub which follows existing code.


D. Timoshkov: Send patches to wine-patches [...]. Do not forget a changelog description for your change. [Jul 07]; If you have a test case that doesn't pass under Wine - add it first, only then send a proposed fix.

When sending the patch in, first you probably want to subscribe to the mailing list. One developer noted [Aug 07]: You can go to your preferences, by going here: http://www.winehq.org/mailman/listinfo/wine-devel Fill in your email address in the last box and click the edit options button. Scroll down and there is an option for "Mail delivery". Turn it off, and you can remain subscribed but not be sent the postings.

Dont Diff autogenerated files as noted by D. Kegel aug 08 wine devel: You can avoid the particular problem by not including a diff for 'configure' in the patch; the diff for configure.ac suffices.


Trusted Developers

A. Julliard [Jun 06]: There's no such thing as special developer vs. other developer, it's more like a linear scale based on trust: the more I trust a developer the easiest it is for him to get his patches committed. The only way to earn trust points is by submitting consistently good patches.

That doesn't mean you can't get stuff in if you aren't trusted, but it means it will require extra scrutiny, which is why it's very important at the beginning to send small patches that are easy to review. Once you have earned enough trust you can get away with being a bit more sloppy (but not too much, or you'll lose trust points again ;-) [wine archive]

M. Hearn [Jun 06]: There's no special privilege for CW employees or SoC students[...] I'm a CW employee on and off (more off than on lately due to university) and I get patches rejected all the time <chuckle> :) wine archive


Sending Patches

Setting up Email

J. McKenzie [feb 10] If you are not subscribed to the list [wine patches], your patch is REJECTED. There is NO moderation. You have to subscribe to submit patches. It would be BEST to send your patch, for comments to the Wine Development list (after subscribing) before submission. This way you can discover how things are done[..] [ed, you can alter the settings so as to not post you emails with all the patches for wine]

If this is your first patch, you might try sending it to yourself first. [Sept 09 J. McKenzie mentioned this regarding a patch]: Please look at the processed mail message before sending to make sure that your patch is an attachment and not bound in-line. Thunderbird likes to do this to text attachments, so you might want to change the extension to .patch or .diff so Thunderbird will not do this. I did not have this problem with using git when git was configured per the GitWine page on the Wiki.

He added :Using git-send is preferred to copy/paste. Thunderbird seems to cooperate with it once you have git setup properly. A. English [feb 10] Please don't send patches as compressed files.

Attribution

[Bug 16466]: M Zalewski 2009-01-12 [..]you should put both your name and your surname when submitting to wine-patches - if not, it can be considered an anonymous submission, and they are not accepted.

A developer asked if he could submitt patches using an anonymous name. A. Julliard [Jun 06]: collaboration is based on trust, and it's hard to trust people who don't even want to give out their name. Obviously you can give me a false name, but I'll figure it out eventually, and then trust you even less than an anonymous submitter so there's really no point in trying to cheat. If you have good reasons for wanting to remain anonymous, please discuss it with me in private and we can arrange something; but otherwise please use your real name.

Make sure the patch has the email correct as seen from this [redacted to preserve his dignity].

From: [redacted first name] [secondname] <redactedemail@redacted.(none)> 

A. English [Jan 09]: Your e-mail is messed up in the patches. Alexandre's scripts take the patch info over the e-mail headers, so you should fix that.

The Changelog Entry

J. Hawkins [Sept 07 wine devel] A good changelog entry details what you changed, not what apps are fixed by a change.

R. Shearman: Actually, a good changelog entry also describes why something was changed,if not obvious from the patch such as fixing a todo_wine in the tests. Although as James says, you'll need a better reason than just fixing a program though, as that hints at programming by trial-and-error rather than by reason.

A. Julliard commented on a changelog entry [sept 07 wine devel]: Two bugs I've found working on previous implentations... [PATCH] winex11.drv: fix two bugs in WTInfoA

A. Julliard: Please write a better description of the changes, "fix bug" can apply to just about any commit (and in general, if you have two bugs to fix there should be two patches...)

D. Timoshkov [Jul 08 winedevel] in reply to this Changelog: user32.dll Fix 16b BMP handling with BT_BITFIELD.

Please have a look how others name the patches. A usual convenience is to write dll name without extension, ': ' and short description.

An example by D. Smith was praised by a developer[Jan 09]: The changelog was as follows:

I plan on implementing windowless richedit controls by refactoring out the references the richedit controls hWnd into an ITextHost implementation.  :This way windowless richedit controls can be implemented using the same code by using the ITextHost implementation provided to the CreateTextServices function.
This patch mostly aims to provide an implementation of ITextHost that can be created and used in the windowed richedit control. Later patches will use the ITextHost interface to avoid using the richedit controls hWnd. Once the existing code is able to properly handle to the case where hWnd is NULL by using ITextHost, then ITextServices can be implemented using the existing richedit code.
---
dlls/riched20/Makefile.in | 1 +
dlls/riched20/editor.c | 71 +++--
dlls/riched20/editor.h | 49 +++
dlls/riched20/editstr.h | 7 +
dlls/riched20/txthost.c | 726
+++++++++++++++++++++++++++++++++++++++++++++ include/textserv.h
| 40 ---
6 files changed, 830 insertions(+), 64 deletions(-) create mode 100644 dlls/riched20/txthost.c


A. English:I just wanted to thank Dylan for all his work on riched, and also for his explanation on his work when submitting patches. [...] it makes it easier to get an idea what it is he's doing because of his clear explanations on each patchset.


One patch simply wrote: makes an installer run farther but this was criticized by D. Kegel: Please, when posting patches like that, mention *which* installer, unless you have some good reason to keep it private.

A. Juliard [feb10] Also don't use bug numbers, add some meaningful message instead. The code must be understandable without reference to bugzilla.

Patch Numbering

D. Kegel [Jan 08 wine devel] commented upon a patch which said.. Depends on my previous patch: winhttp/tests: The way we usually express that is to prefix each patch with its position in the series of patches. For instance, your two-patch series might have used subject lines

[1/2] winhttp/tests: Add tests for WinHttpCrackUrl port number handling
[2/2] winhttp: Correctly parse specified ports in WinHttpCrackUrl

I like how you broke out the tests into a first patch. (These days it's probably also ok to just include both the tests and and the implementation in a single patch if you prefer.)


[Jan 10] When submitting multiple patches that apply to the same component but each patch is unrelated (ie. each one fixes a different issue), is it still appropriate to label the patches with the [PATCH X/Y] notation? In the case I have at hand the patches are even for different files of the same component, so the apply order completely doesn't matter.

E. Hoover: Then send them separately. That avoids having the rest of the patch chain being blocked by a rejected patch further in the front of the chain.

Test Cases

D. Kegel [Mar 08 wine devel]: please don't send in any patches that don't have conformance tests, or whose conformance tests don't pass in both wine and windows.

D. Kegel [Jan 08 wine devel] These days it's probably also ok to just include both the tests and and the implementation in a single patch if you prefer.

Especially consider a test case for regressions. P. Vriens: Maybe we should even make it mandatory that fixes for regressions should be accompanied with tests [...]

J. Hawkins:That would be a great policy. Even more so, I personally believe that fixes in general should be backed up with a test case, and if one cannot provide a test case, one should describe why a test case is not feasible. [..] http://permalink.gmane.org/gmane.comp.emulators.wine.devel/68474 [edited]


H. Verbeet [feb 10]: Sending the test before the series is (IMO) something you do when it's not entirely obvious that the test would fail without the change introduced by the patch.

Setting up Git

One patch recieved the comment on Wine devel Jan 09: Please set your real e-mail address in your Git configuration; see http://wiki.winehq.org/GitWine#head-8631765a8d71509690613c9191ed2094c7775722 for directions. [See below for git comments found in mailing list]

A. Julliard [Apr 06] encouraged the use of the souce control manager called git for sending patches

McCormack explained: As of the recently released GIT 1.3.0, sending patches via an IMAP folder with GIT just got easier. If everything is set up right, you should be able to run the following command:

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

This requires some configuration in ~/.git/config:

[core]
name = "Mike McCormack"
email = "[redacted] @codeweavers.com"
[imap]
Folder = "INBOX.Drafts"
Tunnel = "ssh -q usr@svr /usr/bin/imapd ./Maildir 2> /dev/null"
[format]
headers = "To: wine-patches <[email protected]>\n"

Using Mozilla, you can just click on the "Edit Draft:" in the message, check/edit the message and then click send.

Alexandre doesn't mind if the subject line contains the ChangeLog entry. [The result is] No more missing patches, missing ChangeLog entries, or patches generated from the wrong directory. I think James Hawkins has written a program similar to git-imap-send that works with GMail...

M. Stefanuic queried about: What to put in here if one uses imaps? Though Mike said: Unfortunately it doesn't support imaps as yet. There was code to support imaps in the isync code that git-imap-send was derived from, however it was removed because libssl is non-GPL. ssh should do just as good a job as ssl, if not better. wine archive


J. Hawkins [wine devel nov 07] suggested : you have to get into the git mindset. That is, you make one fix, commit the fix, write the next fix, etc. It's really a development change.

M. Lankhorst [wine devel nov 07]: [at] times I first hack around till my app works before cleaning it up and sending in a proper fix. You are right that missing the clean-up will mean that the code won't be accepted into wine, sometimes that is even more work then the stuff needed to make it work.


Further Reading

A translator said: this is my first patch using git and I'm really unsure if I did it correctly. R. Shearman [Oct 07 wine devel] You need to use git-add to include the new files in the commit/patch you sent.


A developer queried for [dec 08]: tips for how to get legible emails out of hotmail without doubling up the newlines would be greatly appreciated Note: I cannot even read the emails I send from hotmail...

P. Nieminen: You can use these steps:

  1. Add your email and name to git config ($HOME/.gitconfig probably)
  2. git-format-patch --keep-subject HEAD^
  3. Now check that patch file looks correct in text editor. (You can also add some extra text to message still)
  4. git-send-email --smtp-server <your isp's smtp server> --to [email protected] 0001*

(You can first check that send-email will produce correct headers with --dry-run)


Further reading

Larger changes to Wine

These need to be eventually split up so they can be sent into wine in small pieces without breaking everything in wine. But posting to the wine-devel mailing list is a good idea as this will give your work exposure. Having a git repository with your work will help if any others want to join in. And when you think it is ready, but before you finally submit it for inclusion, you could ask for testers in the wine devel list to check and report any regressions. This would help build further trust. One example is the DIB engine. Several have tried. One recent effort didn't receive many comments and the developer asked about this. L. Rayhen [Feb 09]:BTW, if you get almost zero comments this isn't necessary bad thing - this usually means that nobody has anything bad to say about your implementation and this is good thing. However, if Alexandre see something wrong in your implementation you need to talk to him personally about this to understand what's wrong and how you can improve it. [...] try to talk about your implementation of DIB engine with Alexandre (on IRC or e-mail privately)[..]

Checking if your patch was accepted

UPDATE After the 2009 wine conference a patch tracker was implemented by Alexandre: http://source.winehq.org/patches/


It is the place to check about whether your patch was accepted, in the queue or even rejected. There are a good number of reasons mentioned in the legend on that page such as..

On the official page there are several hints about how to improve a patch and explains the different ratings. As S. Leichter wrote [jan 10] According to http://source.winehq.org/patches/ the patch is pending. You may have more luck if you send some test cases for the unit tests first.


The old way to check if your patch was accepted:S. Dossinger Jan 09 winedevel: You can see on source.winehq.org/git or with 'git log origin' if your patch was applied. If it wasn't, it is best to send a mail to wine-devel asking why, or ask Alexandre(nick 'julliard') on #winehackers. Quite often Alexandre sends a reply if he doesn't like the patch(or someone else does), but not always. Alexandre usually wants a confirmation from someone who knows the area(in the case of d3d Heri, Roderick or me) if a new contributor sends a patch, hence my "the patch is ok" mails.

He then noted: Your wined3d_gl.h patch was applied, the others weren't. Sometimes too much discussion makes AJ wait on a patch too, to see how the discussion turns out. If you resend your patches I'll just reply with an patch is ok" mail to avoid further confusion [More about git below]


The power of git can help you see what has gone through.

K. Blin [feb 08 wine devel]: I tend to use feature branches in git that I rebase on the top of Alexandre's tree. Simply starting gitk will show me which of my patches have been accepted and which of them are still on top of the tree. Of course that means I need to track which of those I sent.

Using more branches, this is even more clear. Assume you have two git branches, 'submitted' and 'my-cool-feature'

You develop in 'my-cool-feature'. Once a patch is ready to be committed, you git-cherry-pick it to the 'submitted' branch, git-format-patch | git-imap-send or whatever you do to send it to wine-patches. Now you can keep track of your submitted patches easily.

A web interface can be nice, so if anyone feels like hacking this up I'm definitely not going to stop them. All I want to say is that I think it's not needed for personal use.

How Long to Wait before the patch might be applied

It can be hard for a new developer wondering how long to wait to see if your patch has been accepted. Here are a few comments about how long to wait before asking, or even trying again. With the new patch queue, you can easily go and check the status.


A developer asked: What is an acceptable amount of time I should wait before asking AJ what is wrong with a patch? I see that patches get submitted and then the next day they are committed oftentimes. However I would like I am pressuring him if I submit a patch, it didnt get committed the next day, and then I shortly after ask him what was wrong. So, should I wait until day 2 or so?[...]

S. Dossinger: Usually aj commits at 2 pm central european time and 8 pm. Patches that are on wine-patches 6 hours before get in usually, except if there are just too many aj can handle in one go. AJ doesn't commit on weekends. I think if the patch isn't committed withhin 2 commit waves after you sent it its ok to ask. (Ok, I personally get impatient someimtes and ask earlier). As for channels beeing dead, quite often there is no talk if there is nothing to discuss, just ping people if you want to talk to them. Aj is usually around between 10 am central european time and 10-11 pm cet, but I'm not sure in the morning as I'm quite often not in then.

J. Liggett: If it's a big patch, it might take AJ 3-4 days to review it; with some of my previous stuff, I waited a week just to be sure. That being said, I second Stefan's advice.[...] I know it's tough, but it's doable--don't give up ;-) Personally I think IRC is hands-down the best way to get advice from Alexandre. [..]

Also, it may be a good idea to let this stuff cool down a bit. This may seem like a long time, but give it maybe three months or more before you start messing with this again. What Stefan says about discussing patches is true--it helps if there's not a lot of [controversy] around a patch. If you're still having issues with some technical aspects of the patch, try contacting the author of the original code. It's a bit of a long shot if said developer isn't around anymore, but you might get lucky and he may be able to give some guidance. I, for instance, was able to get some help from the original author of the XEmebed systray patch to help me get it in. I'm really rooting for you. If I can do it, I know you can. Just be patient and you'll pull through.


One new developer noted [feb 2008 wine devel]: 18 hours passed, and it looks like Alexandre decided to ignore this...why? This fix is required for Wine to be built with VC2005/2008

A. Julliard: There's no need to count the hours, or to ask every day, especially not with a mail to wine-patches. I have lots of pending patches from my vacation, I'll get to it eventually, and if I don't you should resubmit it after a week has passed. And if you want me to care about VC2005, maybe you should mention in your patch that this is the reason for your change, "fix invalid syntax" doesn't tell me anything.

M. Stefanuic: Wine uses git, a distributed SCM. You are free to publish your own Wine tree without waiting for Alexandre. People prefer to follow Alexandre's tree but nobody forces them to use it.

Using Bugzilla Bug Reports

V Margolen [Sept 06]: if some one wants to fix something, they should either provide a test (best choice) or open bug and describe the problem, and the resolution. This will not be noise (in bugzilla) but the _correct_ use of a bug database. Making good bug report is really helpful and 1/2 of the resolution.

A. Mohr [Sept 06]: If for some reason the Wine patches you submit fail to get applied, then we'd appreciate you taking the effort of submitting your current patch as a new item at bugzilla to help us track your work properly until it's fully applied."

J. Latimer: the person who makes the patch to resolve a Bug report will close the Bug. wine archive

Wiki Patches

T. Lambreghts: As alternative to bugzilla we have this section in the wiki. http://wiki.winehq.org/InterestingPatches .This has several hac^H^H^Hpatches that I found uesfull and have used over time. I particularly like the "Mouse Hack" patch http://wiki.winehq.org/PatchMouseHack The thing is that if a patch is useful it will have a life of its own and I am glad that I have an easy way of getting to them when I want to try them.


Right of Appeal

R. Shearman: you already have a right to appeal - it's a message to wine-devel with a well-reasoned argument.

A. Julliard [Jun 07 wine devel]: Things don't get decided by popularity, but by technical merit. If you can make a good technical argument for a change it will go in; if you can't, gathering votes for it won't help. [...] It's up to you to explain what the benefits are, why [...] and not others, why [the other Wine Developer] is wrong in his objections, etc. That's how you make a good technical argument.

And if you were wondering, yes he does accept a good argument as A Julilard wrote [Sept 10] Yes. My initial thought was that you could improve the global ANYSIZE_ARRAY, but as you pointed out that's not a good idea. What you should do then is...[snip].

Resending Patches

A developer wrote: Sorry, to clarify: this series of patches is a resend of the last without the tests, and with some new patches.

A. Julliard: When resending patches you have to first rebase them on the current tip.

One Developer asked: [July 07] What things should i change for the re-submit?

V. Margolen: Don't send the same patch more the 3 times. Alexandre doesn't like it ;-) You should work out all the problems then resubmit. You can send patches to wine-devel for review in the [mean] time.


dlls/wininet/internet.c       |    6 ++++++
dlls/wininet/tests/internet.c |   26 ++++++++++++++++++++++++++ 
2  files changed, 32 insertions(+), 0 deletions(-)

P. Vriens [feb10] when you sent a newer patch that has changes you should mark it as 'try x' instead of 'resend'. 'Resend' is used when you think the patch has been missed by AJ for example.

P. Vriens May 09: this is the third time you tried with the same patch. We usually add a 'resend' to the subject of a patch that is, uuh resend. If it's another revision we add 'try x'. In your case you should probably ask (on wine-devel) for comments or contact AJ directly on irc.

Rebuilding Trust

A programmer queried: Just curious if Alexandre missed my patches [...] or if there is a backlog of commits to be done, or if he even plans to commit them, and if not, why..

A. Julliard: You have been sending way too many patches lately, most of which are broken one way or another, so I just stopped looking at them. You need to spend more time on each patch to make sure it's correct, properly formatted, that the patch is included in the mail, etc. That's particularly true for cosmetic patches like typo fixes; if I have to spend more than 10 seconds on such a patch, chances are it will end up in /dev/null.

[...] resend just one, making absolutely sure that you got everything right, and wait for it to be committed before sending the next one. You have to make sure you can walk before you try to run. [wine devel 2007]


A. Julliard [may 09] The last time I rejected a simple patch from [Redacted] he basically said that he didn't have time to fix the patch and just dropped it. That doesn't encourage me to spend more effort on reviewing his more complex stuff.

Fixing Breakages

J. Lang [May 09] we generally end up [..] Watching test.winehq.org for test results once a patch gets committed, or waiting for an email from someone who watches the results, is a way to get feedback on test failures on other platforms. The alternative is to ask someone to test your tests on other platforms, by asking on wine-devel as you've done.

However, this has changed. Now there is available computers for testing. Ge van Geldorp May 2010 I have a whole bunch of VMs set up specifically for Wine testing, so feel free to send me patches you think might behave differently on different Windows versions. One caveat: I personally stopped caring about 9x when Win2000 came out, so I'm not testing on those versions.

Patch Tips

small can be beautiful
A. Julliard commended one developer for spliting up his patch but asked for assistance: Thanks for sending a series of small patches instead of a big one, but next time can you please number the patches? (git-format-patch -n can do it for you). You can't rely on the mail server to preserve the order, and since they all depend on each other I have to be able to apply them in the right sequence. wine archive

M. Hearn [Apr 06] The rule of thumb I use is actually *not* to break things up as small as they could possibly go, but rather to separate things into components that are logically related and can be applied without breaking Wine or causing any regressions. So for instance I wouldn't bother separating header changes from the "meat" of the changes. But everybody has their own approach and style[...]

M. McCormack: The point of having small patches is to make Wine easier to regression test, so we want things to work between every commit. wine archive

D. Kegel (reviewing a patch) [Jun 06]: It would help if he said in the preamble to the patch what functionality the patch adds. wine archive

Easy to apply
A. Julliard: [Aug 06] There's no point in sending a (corrupted) inline version of the patch plus another one as attachment, all it does is prevent the patch from being applied. Please send only one instance of the patch, in a non-corrupted format.


'Sending Patches In the Correct Order"
A developer queried [May 07]: Did you have any more to offer about the other patches?

A. Julliard: I didn't look much at them, but you need to do more work on the sequence, for instance the makefile changes are in patch 7/10 this means the code in previous patches doesn't even get compiled, that's very wrong.


Heeding Code Reviews
A. Julliard [Jul 07 wine devel]: You still need to use symbolic constants here, like Misha suggested. Note: you shouldn't wait for me to tell you that, the point of a public review is that everybody can comment on a patch, and you are expected to take these comments into account. In fact if I see that someone has commented on a patch already, usually I don't bother to review it, I just wait for you to resubmit an improved version.

Rewrite vs Fixing A user asked whether to completely rewrite or fix a feature in wine.

A. Julliard [ Aug 06]: [it] certainly needs a lot of love, and improvements would be welcome. But it's usually not a good idea to start with the "throw it away and rewrite" attitude; and to be blunt, I'm not going to accept a rewrite unless you demonstrate that a) you thorougly understand the existing code, and b) you can really do better. And the only way to demonstrate both is by first submitting good patches against the existing code. wine archive


Building Trust R. Shearman [Sept 06]: Another way of demonstrating that you understand the problem is to demonstrate that you understand the code by submitting patches to it, fixing smaller and easier to solve bugs.

D. Timoskov [Sept 06]: If he sees that a person really grasps and understands the code, he reviews his patches much more freely, and allows some sloppiness in his code, hoping that it improves with time.

D.Kegel [May 2007 wine devel] while advising a new developer:[...] Getting conformance tests committed to the WineHQ tree is a good rite of passage. Then, once you have the hang of that, pick one of the bugs you've written a good conformance test for, and try to fix it.


Test Cases build trust: S. Dossinger [wine devel sept 07]: If wine behaves incorrectly and your test fails on wine, you have to send a fix before or in the same patch as the test. Alexandre has some scripts that run the conformance tests after each patch and refuses all patches that cause in test failures. If you do not know how to fix the bug, or want to keep it unfixed for some other reason(e.g. the incorrect behavior is hard to fix and considered harmless), then you can mark the test as todo_wine. Then the tests script will ignore failures in the test. However, Alexandre will drop a note and ask for further explanation if a test marked todo_wine succeeds(e.g. we had a test that succeeded or failed for some time based on the features graphics driver).


What are the implications of Wine 0.9?

A. Julliard [Oct 2005]: Apart from the version numbering, it won't change much. There will still be regular CVS snapshots (I'm hoping to do them more frequently than in the past, but don't hold me to that ;-), and binary packages built from these snapshots. I have no plans to create stable/unstable branches, at least not until we reach 1.0.

[There has been] a progressive process that has been going on for some time now. As code matures I'm getting increasingly reluctant to accept large changes to it, that's why I insist on small patches and test cases for changes to areas like the wineserver, or the messaging code; while for dlls that are still pretty much in prototype stage almost anything can get in. So as we progress towards 1.0, more and more code will get in a "frozen" state.

At some point we'll have to decide that some features need to be postponed until after 1.0, but I don't think we are quite there yet. At this point I don't see anything that is currently being worked on that would be unacceptable for 0.9.x. Wine Archive


Wine defacto maintainers

S. Edwards [Sept 05] explained why it is best to work in with those who are defacto maintiners: With DirectX hacking I would be more inclined to send my patch to Oliver first rather than sending it to Julliard because 1) I know Oliver [was] the defacto maintainer [...] and 2) If he has uncommitted patches in his tree then my changes could result in conflict or duplication of work. [You could use git to find who has contributed to the area you want to work in] Insert non-formatted text here

Wine and Patchwatcher, the Patch Robot

Patchwatcher is offline at present, however you can check to see the status of your patches that A. Julliard has recieved: http://source.winehq.org/patches/

For a bit of history Aug 08 D. Kegel: After some discussion with Jeremy and Alexandre about how nice it would be to have some automated way to check patches before Alexandre commits them, I decided to have a go at creating one. snip

A. Julliard appears to approve of the concept. Aug 08 he wrote: what should really happen is that patches that fail the validation don't even make it to wine-patches, they get bounced to the submitter. Of course the bot needs to be reliable enough for this not to become a nuisance. Maybe there can be a magic bypass keyword to add to a patch to override the bot.

Patchwatcher has quickly grown into a handy tool. If someone comments upon the patch in the Wine Devel mailing list the plan in Aug 08 was to set it up so that unless several key words are used to show they agree with the patch - the patch is dropped until you resend, hopefully taking into account the suggested changes. [update, this has been offline for a while. it not the patch display by A. Julliard which is in a slightly different format and shows the status of patches he has reviewed]

Patchwatcher Reject Messages

patching file dlls/wined3d/drawprim.c Hunk #2 FAILED at 448. 1 out of 2 hunks FAILED

D. Timoshkov Aug 08: That means that the source you have used to generate the patch doesn't match what patch watcher uses (latest git). [..] Update to current git and regenerate the patch:

V. Margolen: The proper way to do it is (For more information on how to use git see http://wiki.winehq.org/GitWine and of course git's manual.):

git fetch
git rebase origin


Wine Official Patch Status

P. Vriens noted one patch [jan 10]: I'm not sure if our automated patch watcher (http://source.winehq.org/patches/) can cope with non-ascii characters in the "From:" field but your patches don't show up over there.

A. Julliard: The reason they don't show up is that the patches were qualified as spam by the winehq filter. They probably need more real text, or a correct utf-8 encoding.


According to the wine devel list the patchwatcher site dislikes non Git patches. A. Julliard: Yes, the filtering script has some issues with non-git patches that only create new files. I put in some fixes, but using git patches when adding files is strongly recommended.

Wine, Copyright and your Employer

Ed. As this is a wiki, where changes can be made by those without adequate expertise, you need to check the source of these quotes.

A developer wrote [Jun 2007]: I have been planning to do some work on wine for a while now, but after I started working I got myself a new programming job. I'm worried about the copyright of any external work I do, so I need a little advice. What do I need from my employer to clear me to work on wine? Is something verbal ok, or should I get it in writing?

J. White asked James Vasile, of the Software Freedom Law Center, to comment on this. ([...] the SFLC officially represents the Wine project on legal matters): This is essentially what he had to say (and James, correct me if I get anything wrong :-/):

If you are employed to do programming (even at a university), or have made an agreement with your employer, school or anyone else saying it owns software you write, then you and we need a signed document from them disclaiming any rights they may have to the software. The disclaimer should be signed by a vice president, general manager, or anyone else who is authorized to assign software owned by them. [...]

As a general rule, get everything in writing. The below will suffice. Email is fine, paper is better. The project needs a copy (or, better yet, the original) of the document.

Here's some sample text:

ACME Corporation ("Company") hereby disclaims all copyright interest in the code written by Jane Doe for the program "[insert name of program]" ("Program"), including original Program code and documentation and support files, changes and enhancements to the Program and files, and any future modifications of the Program and files. We do not consider the code as work made for hire for us. Company affirms that it has no other intellectual property interest that would undermine this release, or the use of the Program, and will do nothing to undermine it in the future.
[signature of John Smith], 30 March 2006 John Smith, Vice President, ACME Corp.


Another Developer queried: Would a paper saying they keep their rights, but approve the LGPL distribution also work?

J. Vasile Provided Sample text for LGPL release:

To the extent ACME Corporation ("Company") has any copyright interest in the contribution written by Jane Doe for the program "Wine", including original Wine code and documentation and support files, changes and enhancements to Wine and files, and any future modifications of Wine and files, Company hereby licenses that contribution under the GNU Lesser General Public License. Company affirms that it has no other obligation or interest that would undermine this license, or the use of the Wine, and will do nothing to undermine it in the future.
[signature of John Smith], 30 March 2006 John Smith, Vice President, ACME Corp.

And, yes, a writing is still needed. Documentation of the right to release code is important. Employers are usually different from authors,both in their interaction with the copyrights and their interaction with the project, hence the different treatment. [wine devel list June 2007 subject: Work legalities]

Common Reasons for a patch to be rejected

There is only one man at the top with wine and the more you do to make his job easier, the better. Because he is so busy he cant notify you if the patch was not applied but not to worry, here are some common reasons patches are rejected.

Patches in Limbo

Patches in Limbo are close to being included. Ask in the forum what is required.

At Wineconf 2004 A. Julliard mentioned 'that although some patches get initially rejected, it's actually a compliment if a patch is in limbo. It means it wasn't bad enough to be immediately rejected. If it gets put aside there's a reasonable chance some changes can be made to get it included. His patch queue stretches about a week long, so if no feedback is given after a week it might be worth asking for an explanation why or send in a resubmission.' Wine Documentation: WWN Issue 208
A. Jullard cautioned against merely resending patches [Nov 05]: The more you send it, the less likely it is to get through. When I see the same patch more than 3 times I just stop looking at it, as it clearly indicates that the patch is not ready to go in. You should stop sending it, split it in smaller chunks, and take the time to get it right before you resubmit.

D. Timoshkov [Nov 06]: The best thing is to ask AJ directly why he didn't apply the patches. From my personal experiance it is best to ask him on irc

Patches without your correct name

You [..] need to use your real name for copyright-tracking purposes. J. McKenzie: [..] we need, for legal reasons, to know the originator of each and every line of code that exists in the Wine project code base. [..] if you do not supply your real name, your code will not be accepted into the project no matter how critically it is needed.

Patch does not match Windows

A. Julliard [sept 08 winedevel] rejected one patch and explained why:[..] it doesn't match the Windows definitions.

The programmer queried this as he had carefully checked msdn: i compared again with [the] definition at http://msdn.microsoft.com/en-us/library/aa370385(VS.85).aspx, but i don't see a problem there. [..] I'm happy to fix up [the] patch , if only i knew what was wrong [..]

J. Hawkins explained the reason and the solution: msdn is often wrong. The only standard we adhere to is the PSDK.

Patch Controversy and Feedback

A developer queried why his patch was not applied and the answers were illuminating:

May 2007: I'd really like to get the wine uninstaller program up to speed with the patch I have been working on, but it is not going as I had hoped. One person offered to review my patches, but I am admittedly impatient sometimes (I haven't been able to get a response from him since Thursday, and I had hoped for one by Friday night), and all in all, I just dont like having something so small take so long. I am making changes to code I didn't intend to change, which I don't mind doing, but then when I make the change, I get told to do it 3 different ways, and none of the ways I submit gets committed, even after being told that it looks ready to go in. I'd like someone who is really in touch with what Alexandre is looking for to help me. I only have 4 patches, all less than 50 lines each to get this thing in, and I will be done submitting patches for a while, [...]

S. Dossinger: I am afraid you have come accross an unmaintained part of wine. There is maybe nobody who can competently help you. Nobody minds helping you, but nobody can help you. What happens then is that you write a mail and get no answers. You get frustrated(not your fault), write a mail in which you complain nobody has answered. 3 persons who do no know the part of wine you're modifying reply 3 different answers. Discussions start, and then AJ stops applying patches until the discussions have come to a conclusion.

Thats just what I think has happened, but I don't know the uninstaller eiter. So count this as a 4th answer to "why does nobody answer me"

As for finding out what aj disliked about your patches my experience is that it is best to ask him on irc(#winehackers on freenode). But I also know that aj stops looking at patches if there is way too much dispute about them. In this case you should send single patches to wine-patches starting that you want to start the process over. Of course the patches should have the suggestions implemented that were made before, or if not a good reason why you have chosen to do otherwise(Not all suggestions are good).


Patch isn't Clean

Copyright

A. Julliard: Copying text from MSDN is not acceptable. If you are going to write documentation you have to do it in your own words, without looking at MSDN. wine archive

This has become formalized in Sept 06 when A. Juliard said in a code review: This documentation is copied straight from MSDN, that's not acceptable. Since many people don't seem to understand this, from now on I'm going to reject all patches that add documentation, unless the submitter explicitly mentions that he didn't look at MSDN to write it. I'm sorry to penalize people who do the right thing, but I can't continue to waste time checking every single doc patch against MSDN.

S. Edwards: [...] Julliards point of view seems to be when you write the interface your free to look at MSDN however don't write the API docs at the same time. Go back later, look at what you have done and base your documentation on your own work. [...] I've updated the IMPLEMENTING NEW API CALLS section of the DEVELOPER HINTS on the wiki to reflect this new policy change in the example section "NOTE: When submitting documentation changes, you must clearly state that when creating your patch that you did not copy the function documentation from MSDN. When implementing a new function it is fine to look at the API documentation on MSDN however the api documentation must be written in your own words." wine archive


A developer queried about: I am looking at writing a piece of IDL and a header file I am wrestling with how to do it without infringing copyright. IDL looks particularly hard to write from first principles and is probably hard to write in a different way as the functions being implemented are defined and require IDL similar code. Is there any advice as to what type of code can be copied and what constitutes a rewrite to avoid copyright?

R. Shearman: Obviously, the function names and parameter order and types have to be the same. The function attributes also have to be the same. However, the order of attributes, indentation and the parameter names can be different. [wine devel jun 07]


On queried how to refer to a web page reference for the API [feb 09]: Please confirm you don't actually want a web-page reference in the Wine source. Would that not raise "copyright-tracking" issues ?

D. Timoshkov: I said "point out to the source in the patch comment" not in the patch itself.

Reverse Engineering

M. McCormack[Jun 06] [...] This is not simply a problem of what is legal and not in different countries. We don't want to be associated project that is perceived to be pushing the edge of what's legally (or even socially) acceptable. Doing so would encourage FUD about Wine and that's a good way to lose developers and users. [...] Using test cases to determine the behaviour of Windows provides a way to verify both the code that's written today, and code that will be written a year later. It's not just legally better, it's easier to do, easier for others to understand, and gives us a way to verify our code.

D. Kegel [Sept 06] The stakes are pretty high; if we include code, and it's later shown to be tainted, the whole project could be endangered.

[Jun 06] how would you write a test for a badly implemented function, when you don't know its name or even name of a DLL it resides in? M. McCormack [Jun 06]: If there's no program that uses that function, then it's not necessary that for Wine to implement it, so it doesn't matter. If there's a program that uses it, then we can get a sample input. [...] Do you really need to look at assembly to figure out that you can just return FALSE/0/NULL and make the program or dll that's calling this function happy? If that doesn't make it happy, you could always write a test program that calls the function and prints out the return value. That is not hard either. [...] He then qualified his comments: I am *not* giving legal advice. This is my interpretation of how interoperability with Windows programs should be legitimately achieved, and what standards Wine contributors should live up to.

K. Blin [Aug 07 wine devel]: All Wine cares about is "What's the output of function X when I put in Y and Z as parameters?". That's why you write a conformance test that will run on Windows. Then you make Wine behave the same. No need to disassemble anything there.

D. Timoskov advised: Make yourself an expert in the area you are trying to improve. Read MSDN,books, find code samples on the net, write the tests on your own.

D. Riekenburg cautioned: MSDN is not complete and not Error-Free. [...]Do not forget the Oldies [Books] (Oldies are goldies!): They do not depend on MFC / DotNet / C# wine archive

This recently bit one developer [Nov 08] who apparently restricted his use of debug to check merely the name of the function called on windows. J. Lang: There are certain kinds of reverse engineering that are allowed, but looking at disassembly is not. I don't think this is going to go in, sorry. [writing up a description of what the function needs to do, so that someone else can do a clean-room implementation] I think that would be acceptable, yes. We have implemented things with hints from people who've studied disassembly before. [...] You can describe your findings [in bugzilla]. [ Patches to add tests that verify the behaviors of the functions that you have seen the dissasembly] In general, writing tests is okay, as they are an exercise in black-box reverse engineering, which is what's allowed.

A. Julliard jan 08 wine devel: You are not allowed to look at what calls the native [Windows] dll makes, that's an implementation detail. The only thing you can do with native is to write test cases to determine the externally visible behaviour. One person was excluded for disassembly and a developer suggested [Dec 08]: If you would like to send me that high level overview then I can see if it works into wine. That way you can continue to contribute and we do not compromise the source code. But A. Julliard instructed: No, please don't do that. We don't want to have people disassemble Windows code at all, even to write high level descriptions. There's simply no need for it, just write test cases.

Reactos and Wine

A Developer wrote: This patch bases on reactos [xxxxx].dll.

A. Juliard [Jun 06]: No ReactOS-derived code will be accepted in Wine at this point.

A poster [Sept 06] asked was there any proof of ReactOS code being bad? D, Kegel: I'm afraid it's the other way around at this point - the burden of proof is on ReactOS to show that their code is clean :-( [...] [Their] audit [...] was done without a neutral third party. [...] I hope they get their audit completed and verified by a group like the SFLC soon. In the meantime, I hope they can benefit from merging in code from Wine, and I hope they reciprocate by filing bug reports against Wine as they find missing features & apps that need them. In Jan 2009 A. English: So no matter how tempting it is, if you'd like to contribute to wine, don't look at ReactOS code. Wine has to be squeaky clean.

Windows Source Code and Wine

A question was raised those who have seen the leaked copies of the Windows Source Code. A. Julliard [Aug 07 wine devel] No, if you have seen parts of the Windows source code, you can't write code for Wine. We can't afford to take any risks in that area. This was queried by a developer in September. J. Lang replied: No contributions from anyone that's looked at Windows source code, whether stolen, under NDA, or disassembled. No contributions to ATL or msvcrt (or MFC, if we ever implement one) from anyone that's seen their sources. I think that about covers "the rules" as I know them, and turning that into FAQ form would be helpful!

This was added to the faq here: http://wiki.winehq.org/DeveloperFaq


The progress of Wine's Audit

Wine's been doing its own audit with the help of the Software Freedom Law Center and Dan gave a timeline for the Wine audit [Sept 06]:

J. White updated [Mar 06 Wine devel] The code audit continues. However, the SFLC has turned its attention to building a set of tools to make it easier to automate the audit, and to drive for 100% coverage. The first prototypes are done, now it's just a matter of setting up web servers and infrastructure and getting the kinks out.

So, in short, it is ongoing, if slowly.

Regarding the SFLC audit one developer said [Sept 07 wine devel] I believe this would make an excellent topic at Wineconf. Jeremy, how about a status report there?

J. White [Sept 06 Wine devel]: Good idea! We'll make that a useful deadline for bringing forward the ideas we've been working with the SFLC for the past year or two. (And yes, I'm painfully aware it's been a long time). I'll post a separate note on the state of the SFLC wrt Wine as I see it as well.


Patches without tests

A. Julliard: there is a policy [about not allowing tests that do not yet pass with current code]. All the tests must always pass in CVS, otherwise the test suite is useless. If you want the test to get in before the code that's certainly possible, but you have to add todo_wine around all the failing tests. Wine Archive
F. Gouget: Alexandre almost always tells everyone that Wine needs to behave like Windows does, and if in doubt - implement it the same way as in Windows. The only problem which arises with that aproach is that we often don't know how some functionality is done in low level in Windows. Alexandre sometimes doesn't know either, but that's not the reason to blame him for that. Whatever the project or development model there is the risk that the patch will have to be reworked before it can be accepted.


D. Kegel winedev aug 08: It's a common misconception that tests should be written after the code. In Wine, it's best to write them before the code, and get the tests working on Windows. You can even submit the tests first before even starting on the code. Incremental development - improve the tests a bit, then fix the code to pass the tests - is a good way to go. Initial tests can be really trivial, just checking superficial error behavior. See also http://en.wikipedia.org/wiki/Test-driven_development Believe it or not, this makes development go faster!

The patch fails/breaks the test suite or generates configure warnings

This is important. Try to check patches againse the test suit before sending them in, because A. Julliard will.

A. Julliard: [This patch] breaks make test because one of the todo_wine tests now partially succeed. Could you please fix the test and resubmit? Wine Archive

A. Julliard: You are assigning a const string to a non-const variable, that causes warnings. A developer then asked: what CGLAGS are you using so we can use them too and detect such compiler warnings in future? Would you reveal them, please? :)

A. Julliard: I don't use special flags, just a standard configure. You may need a more recent gcc to see all the warnings. wine archive


J. Lang [sept 08 wine devel] C++-style comments are not allowed in Wine source. The developer wrote: Sorry I missed this one out. I didn't notice any compiler warnings.

J. Lang: No, you wouldn't notice any on a recent version of gcc, as it will silently accept C++ comments unless you specify -fpedantic or some such. The trouble is, not everyone uses a recent version of gcc, and their builds will fail with C++ comments left in.


D. Timoshkov [Nov 06] Did you run 'make test' after [making] this patch?

Patch is not finished

D. Remenak [Oct 07 wine devel0: When you fix bugs that are documented in comments, please fix the comments also.


The patch needs more context. (you lost him somewhere)

A developer asked: I've got several additional patches prepared already that depend on [a pending patch] [...] If it's just not being applied because it's considered dead code I can submit the other patches that actually use it.
A. Julliard: That would be good. The [...] functions that are not used in a vtbl don't look right to me, but I'm not sure what you are planning to do with that. Wine Archive

Incorrect Patch format

Plain text required
D. Kegel advised regarding a patch [Aug 06]: . Your message was sent as HTML mail. Please send it as plaintext. (In gmail, just click "Plain Text" on the compose window.) wine archive

R. Shearman [Sept 05]: Please send patches as text attachments. Usually, renaming to .diff or .txt before attaching is enough to make this happen.

P. Millar: [Oct 05] The changelog entry normally goes before the patch, not included as part of the patch. Wine archive
P. Millar Please [...]be careful to avoid line-wraps. This helps for reviewing patches.
A. Julliard [July 05] also explained: I'd appreciate if you could send a proper patch instead of a zip file.
One developer noted sending tarballs to wine-devel is not a good idea [Aug06]: such a practice discourages reviews.

Line endings
A. Mohr pointed out a concern [Nov 05]: Your diff file has strange DOS line endings (^M) and thus might fail to get applied by Alexandre. Try dos2unix or unix2dos or so. wine archive

A developer was requested [Nov 06] to make sure a new line be at the end of the file. He asked :Is that a coding style issue?

J. Stolk:it seems to be "official" This is in section 2.1.1.2 of the ANSI C 1989 standard. Section 5.1.1.2 of the ISO C 1999 standard (and probably also the ISO C 1990 standard).

M. Finnicum: [Nov 06] two good reasons why end of file new-line characters are a good idea. First, if you were to concatenate two files together (such as how i think the prepreocessor handles a #include), without that newline the last line of the first file and the first line of the second file would be appended together as one long line, instead of being one after another. Secondly, the lack of a newline could indicate a corrupt file / broken pipe.

White space damamged
A programmer reported [Dec 05]: Mozilla Thunderbird was forcing encoding to UTF and removing much of the original file's whitespace. I've since resubmitted the patch, but am having to send it as an attachment. wine archive

Another reported difficulty with Outlook Express [Dec 05]. The wine developer guide has a section regarding enabling outlook to send attachments as text: http://www.winehq.org/docs/en/winedev-guide.html#PATCH-FORMAT

Sending patches in Git format preferred
M. McCormack [Aug 06]: I think Alexandre would prefer to receive patch submissions in Git format, as they are easier to apply and waste less of his time.

D. Kegel [Aug 06]:The safest way to go for you is probably to generate the patch with

git diff > ~/my.patch

then attach my.patch to your message, and [do] not include a second copy inline. wine archive

A. Julliard on the rare event you need to compress the patch [Feb 07]: Please don't send patches in zip format, that's a pain to extract. If you really have to compress please use gzip or bzip2.

Further Reading

Testing your patch
D. Kegel: To make sure your patch is not corrupt, verify that your message in the archives can be applied as a patch.To check this yourself, try to apply your message as a patch using the program 'patch', e.g. in gmail, do Show Original, File / Save; or go to the archive, e.g. http://marc.theaimsgroup.com/?l=wine-patches&m=115643527030192&w=2 and click on the "download message raw" link. Then try applying the patch to a clean wine tree using the 'patch' program (see http://kegel.com/academy/opensource.html#patches.using ). wine archive

Credits

A. Julliard [Aug 06]: You need to put your full name in the mail header or changelog so that you get credited correctly in the commit log. I don't apply patches sent under a pseudonym/alias.

D. Kegel [Aug 06] provided some suggestions for a patch which may help others:

  1. The patch is missing the filename header, and cannot be applied to the sources by running 'patch'.
  2. the patch was posted with the following legal notice:
    "Notice: The information contained in this e-mail message and/or attachments to it may contain confidential or privileged information. If you are not the intended recipient, any dissemination, use, review, distribution, printing or copying of the information contained in this e-mail message and/or attachments to it are strictly prohibited."
    That tells us "You may not use this patch." You have to post patches without that. [Ed: emphasis added]
  3. the message did not contain any license information. Adding that would help. e.g.
    Author: [redacted]
    Copyright [redacted]
    License: LGPL.
  4. The message essentially repeated much of the bug report in bugzilla. That's not needed. Just refer to the bug report.

Any one of the above four might have been enough to cause the patch to be ignored. Fix them all and you've got a good shot.[He then gave..] Two more tips:

  1. The bug report in bugzilla does not link to your proposed fix. Often after I post a proposed fix, I add a comment to bugzilla saying something like "Patch sent: http://www.winehq.org/pipermail/wine-patches/2006-July/029145.html"
    Not only does this give another way for people to stumble across and review your patch, it also helps avoid duplication of effort. I had no idea you

had sent in a patch for this bug already!

  1. Patches that do not get applied need to be resent after a few days. Every time you resend it, you should try to improve it or the test case a

bit, and make sure it still applies against current cvs or git. Use the same subject line as before, but append "(resend)" to the end.

Patches are like TCP/IP. There is no ACK when a packet is corrupt, you have to resend after correcting a bit error! wine archive

Patch Size (usually too big)

Note: emphasis has been added to these quotes

Seperate patches for separate changes

A. Jullard:[Sept05] Please submit separate changes as separate patches. A. Julliard: Breaking things up is always preferred. Wine Archives

A. Julliard: splitting patches is good, but don't overdo it, if you have a C file and its associated header it makes no sense to send them as two separate patches, each one is useless without the other. Wine Archive
F. Gouget: Alexandre once mentioned at WineConf, patches that get 'ignored' are those he is not sure about, or that would take too long to review (e.g. because of size) so that he goes on with the smaller/obvious patches first hoping to come back to the tricky ones later. But the result is that the tricky patches get to the bottom of the pile with the new patches piling on top
R. Shearman [Jan 06]: patches are meant to be independent atomic changes that improve the code. wine archive
P. d'Herbemont [Jan 06]: Use git to create your separate patches (git format-patch) [1] from the small incremental changes you've committed to your own git repository.
[1] http://wiki.winehq.org/GitWine


May 2005 M. Hearn:It's usually easier to review these things in smaller chunks. An easy way to do that is to submit :
  1. header changes first,
  2. then stub functions,
  3. implementation of each function and
  4. a test program, all as seperate patches.
if [the patch is] not applied, please consider breaking it up. At least that way, you'll have less to submit next time round.
April 2006: A. Julliard:Yes, each separate change must be a separate patch. If you are moving code, you should send a patch that does just that, without changing anything else. If you are fixing bugs send one patch per bug. wine archive
A. Julliard:[Aug 05] [after examining and declining a patch] It looks a bit over designed to me. With this patch you add even more infrastructure, generic interfaces, etc. and it still doesn't do anything. I'd suggest going the other way around: implement one working version of the code without worrying about being generic, without trying to abstract interfaces or anything, just a simple source file implementing the COM interface directly. Then if there's really a need for other implementations to share a lot of code we can look at abstracting parts of it. Wine Archive
O. Stieber: I initially had a lot of patched declined, hell it was annoying, and my work would have made it into winecvs a lot faster if they had been committed earlier, but I think breaking patches down into small, defined manageable chunks is far more important in the long run, especially if it causes regression.


This was highlighted [Nov 05] when a programmer asked about a patch was rejected: A. Julliard explained: The changes look ok, but please send separate fixes as separate patches, there are really 3 different fixes in your patch. wine archive


This appears a guideline as there seems to be the odd exception as suggested by S. Dossinger [Sept 07 wine devel]: you can send test and fix in one patch since both are small.

One Line Patches

A developer mentioned a one liner patch, on wine-devel [Mar 06]: (Patch not included due to the simplicity of code changes). M. McCormack: Please include the patch and send this mail to wine-patches instead, otherwise you're just making more work for somebody else.

Gzip

R. Shearman: don't gzip a patch unless it is over ~100KB since it makes it much harder to review.

Send Patches via Email

A developer said of a particularly large patch [May 20 wine-devel]: here's a link to the patch instead of inlining it.

A. Julliard: Please don't do that. You can gzip the patch if necessary, but please send it by mail.

No HTML
D. Timoshkov [Sept 07 wine devel]: For some reason your mailer attaches the patch in text and html formats, it would save some time/traffic by avoiding html in wine-patches.

Unnessesary large formatting changes

D. Timoshkov reviewed one patch and said : Don't you think that your patch is a bit too large although the only thing it does is adding the only a single memset call? Please get rid of all not necessary formatting changes and resubmit.


There is a known limitation of the existing system which was highlighted by T. Rollo: with the existing system - if a patch has been rejected, it should be a necessary consequence that the submitter is informed with reasons [...] What is needed is a system that records all patches, together with their current status (NEW, APPLIED, REJECTED (with reasons), and whatever other status), informs the submitter of any change, and does not allow for a patch merely to be forgotten. [...] if submitters received immediate feedback once there is a decision, they could turn around a new version of the patch much more quickly (and an ideal system would link the new submission to the original to put it in more complete context). Then others who may be depending on the patch can get on with their work too.

A. Julliard: I don't really care who builds it, as long as I can use it from inside Emacs <g>

Ideas for the new system were suggested: Ivan Leo Pouti listed some potential standard replies for patchs :Rejected, reason: tests needed" or "will review later" or whatever. F. Gouget thought: [A] 'Needs review' email to wine-devel would be enough. It would also be the clue to the other Wine developpers:

However A. Julliard felt: That should really be the default behavior, all patches need review; there's no reason to wait until I have looked at a patch to look at it. If you see a patch in an area that you know anything about, please review it, don't wait to see my reaction first. [The discussion ended there, with a number of the developers making a call for volunteers]

Governance

Wine each Wine Conference, Governance is something often considered. After the 2006 conference a few snippets were taken from the mailing lists list:

we had fairly clear agreement that so long as he can handle it, it is most efficient to have Alexandre as the sole maintainer. Obviously, the more help he gets from component maintainers (e.g.Mike/MSI, Rob/COM), the better.
the assembled 30 or so of the most core Wine developers, could not think of a single case where Alexandre had been proven wrong. Nor could we think of a single instance when he had failed to be persuaded by reasonable argument; making a rather compelling case that he is generally right.
We also talk, from time to time, about building some sort of patch tracking system to allow for better management of patches. Something like a 'ticket' system, so people could see the status of their email, whether or not it had been reviewed, etc, etc. I think there is some sense that this might be useful, but it's a sufficiently complex problem, and it has to be written in emacs, that we always defer it for the future.

[update] 2009, this was implemented by Alexandre!

Personal tools
Namespaces
Variants
Actions
Navigation
Toolbox