Re: Suggestions for improvement of the emulator
This of course points to another problem with the existing system - if a patch has been rejected, it should be a necessary consequence that the submitter is informed with reasons - they shouldn't have to be chasing up Alexandre to find out if the patch was rejected or merely missed (which happens often). (snip) 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.
What this misses is the most common status that causes us all to argue: uncomitted, because Alexandre's not sure about it. Perhaps he has a gut feeling that the approach is not right, but hasn't taken the time to identify any particular flaw. Perhaps it merits additional thought. Perhaps it's too big to review quickly. Perhaps he hopes the submitter will realize it's not up to snuff, and resubmit a better patch. There are probably other reasons, but these all come to mind from my own experience. And no system can appropriately capture that, when the answer is not as clearly defined as one of the above states. This is frustrating, and some people do seem to be put off by it. I have no idea how many potential contributors we lose this way. On the other hand, this is Alexandre's style, and we've all had to get used to it. Any alternative that involves Alexandre doing more work will get summarily rejected--he does a great deal as it is, more I think than any one of us would be willing to do (or perhaps could do). The only credible alternative is a multi-maintainer system. This could allow faster development of some larger features, or more invasive ones, or more exploratory ones. The problem I see with this is one of quality: We have a large code churn rate, and, corresponding with that, a high bug churn rate. That makes regression testing and choosing an appropriate wine version both very difficult. Having multiple maintainers multiplies the number of possible configurations. Yes, I know we could have official releases and the like, but who of us would resist trying out some as-yet-unmerged DX9 patch from Oliver, or experimenting with safedisc? Continuing in that vein, we're all lazy about merging in large patch sets. I'm merging in a fairly large set of changes at the moment, and the slower pace has allowed me to catch issues that may not have surfaced otherwise. Having several competing trees with substantial differences would, in my opinion, reduce the overall quality of the project. So I see a tradeoff between development speed and quality. We may be perceived as having issues with speed, but I think we do pretty well with our small stable of developers. We do have issues with quality, IMHO, so I'd be reluctant to endorse any radical changes. What does seem to work is picking up for Alexandre by reviewing patches on wine-devel, and helping newcomers along. We seem to have picked up a few lately that don't mind the public thrashing :) There does seem to be some more collaborative development going on lately, too. Perhaps we should be more vocal about how we do business, what kind of feedback to expect, and how to get help? --Juan ______________________________________________________ Click here to donate to the Hurricane Katrina relief effort. http://store.yahoo.com/redcross-donate3/
Juan Lang wrote:
What this misses is the most common status that causes us all to argue: uncomitted, because Alexandre's not sure about it. Perhaps he has a gut feeling that the approach is not right, but hasn't taken the time to identify any particular flaw. Perhaps it merits additional thought. Perhaps it's too big to review quickly. Perhaps he hopes the submitter will realize it's not up to snuff, and resubmit a better patch.
So what? just set "Rejected, reason: tests needed" or "will review later" or whatever. Ivan.
On Wed, 7 Sep 2005, Ivan Leo Puoti wrote:
Juan Lang wrote:
What this misses is the most common status that causes us all to argue: uncomitted, because Alexandre's not sure about it. Perhaps he has a gut feeling that the approach is not right, but hasn't taken the time to identify any particular flaw. Perhaps it merits additional thought. Perhaps it's too big to review quickly. Perhaps he hopes the submitter will realize it's not up to snuff, and resubmit a better patch.
So what? just set "Rejected, reason: tests needed" or "will review later" or whatever.
Yeah, maybe a very generic 'Needs review' email to wine-devel would be enough. It would also be the clue to the other Wine developpers: * that you're not going to be duplicating Alexandre's work if you review this patch * to look at the patch, dissect it to see what is wrong * if it is in your domain of competence and it looks good, post an approval message * to test the patch * and help its author get it accepted And the reason I'm proposing a very generic message is so Alexandre does not have to spend time to pinpoint exactly what is wrong with the patch (otherwise he could just as well send a detailed message). As 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. So assuming these are still in his 'pending' mailbox, maybe Alexandre could send this generic 'Needs review' message for all patches that are more than one or two days old in his mailbox. Another thing he mentioned at WineConf is that it is ok to ask 'why was my patch not committed?'. Just don't do it an hour after posting the patch of course<g>, wait a day or two. I wonder if this is well documented... -- Francois Gouget fgouget(a)free.fr http://fgouget.free.fr/ tcA thgirypoC muinelliM latigiD eht detaloiv tsuj evah uoY
Francois Gouget <fgouget(a)free.fr> writes:
Yeah, maybe a very generic 'Needs review' email to wine-devel would be enough. It would also be the clue to the other Wine developpers: * that you're not going to be duplicating Alexandre's work if you review this patch * to look at the patch, dissect it to see what is wrong * if it is in your domain of competence and it looks good, post an approval message * to test the patch * and help its author get it accepted
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. -- Alexandre Julliard julliard(a)winehq.org
On Wed, 7 Sep 2005, Alexandre Julliard wrote:
Francois Gouget <fgouget(a)free.fr> writes:
Yeah, maybe a very generic 'Needs review' email to wine-devel would be enough. It would also be the clue to the other Wine developpers: * that you're not going to be duplicating Alexandre's work if you review this patch * to look at the patch, dissect it to see what is wrong * if it is in your domain of competence and it looks good, post an approval message * to test the patch * and help its author get it accepted
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.
Agreed. That's how it should work in an ideal world. The problem is that when reading wine-patches it is easy to look at a patch and think "I don't really know anything about this, I'll let someone more knowledgeable look into it" (and really reviewing patches takes time). Patches that get dropped deserve more effort, if only because by then we know noone else came to a definitive conclusion on them. Yet most of the patches that get dropped don't get a comment from other Wine developpers either and part of the reason is that these patches are difficult to identify. Of course clearly identifying dropped patches could also encourage developpers to only review those patches and ignore wine-patches which would be bad. It's all a tradeoff. I'm willing to accept that we currently have the best tradeoff possible. -- Francois Gouget fgouget(a)free.fr http://fgouget.free.fr/ You can have my guns when you pry them from my kids cold, dead hands.
On Wednesday 07 September 2005 19:35, Alexandre Julliard wrote:
Francois Gouget <fgouget(a)free.fr> writes:
Yeah, maybe a very generic 'Needs review' email to wine-devel would be enough. It would also be the clue to the other Wine developpers: * that you're not going to be duplicating Alexandre's work if you review this patch * to look at the patch, dissect it to see what is wrong * if it is in your domain of competence and it looks good, post an approval message * to test the patch * and help its author get it accepted
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.
There is a problem here, you are presupposing the submitter is interested in reviewing the patch to the projects specification. This subverts the value of collective development if the submitter is unwilling then you lose the value of the improvement AND potentially the developer. It would be better to commit it to a branch to open it up to all to consider.
Robert Lunnon wrote:
On Wednesday 07 September 2005 19:35, Alexandre Julliard wrote:
Francois Gouget <fgouget(a)free.fr> writes:
Yeah, maybe a very generic 'Needs review' email to wine-devel would be enough. It would also be the clue to the other Wine developpers: * that you're not going to be duplicating Alexandre's work if you review this patch * to look at the patch, dissect it to see what is wrong * if it is in your domain of competence and it looks good, post an approval message * to test the patch * and help its author get it accepted
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.
There is a problem here, you are presupposing the submitter is interested in reviewing the patch to the projects specification. This subverts the value of collective development if the submitter is unwilling then you lose the value of the improvement AND potentially the developer.
If the developer isn't willing to get the patch up to a high enough level of quality, then they aren't going to get their patch in unless someone else takes over their patch. I don't see how anything is going to change that.
It would be better to commit it to a branch to open it up to all to consider.
There isn't really much of a difference between having a branch and having a patch in the wine-patches archives. Having it in a branch may in fact be detrimental as it makes it easier to not do the work to get it committed to the main tree. You could end up with 5 different branches, all with different goals and none of the developers being bothered to merge anything into the main tree so all of the features can be used together. -- Rob Shearman
On Tue, 06 Sep 2005 21:29:22 -0700, Juan Lang wrote:
This is frustrating, and some people do seem to be put off by it. I have no idea how many potential contributors we lose this way.
FWIW, I share these concerns though given my non-existant patch writing speed lately I'm not sure how much my opinion counts here ;) Let's take a few examples of patches that were rejected but I feel ought not to have been: * System tray patch. I wrote this when I was 18, I'm now 21. It's taken over three _years_ and this patch is still not deemed correct. In the intervening three years our users have constantly asked why the system tray simply does not work for them. I have no idea what is wrong with this patch. Rob tried to get it merged too, again, he came away empty handed. Last I remember, Alexandre felt the desktop integration code needed better design, or more precise design, or something but I'm not really sure what. I don't care anymore, long since given up on this one. While I'm all for getting desktop integration right, I don't feel the system tray is so vital that it's an all-or-nothing scenario. It doesn't need to be perfect first time around, it needs to work and make our users happy which by and large it does (did?) * Delay logging. This is the patch where you could press set an environment variable and then F12 would toggle logging on/off. It's helpful when the debug output is so verbose it prevents you getting to the part of the app that is buggy. IIRC not merged because it was putting policy into the core code, or it was felt a system based on piping/awk etc would achieve the same thing. I don't remember which but at the time, I remember showing that the performance benefits that motivated it didn't stay with a pipe/awk based approach. Yesterday a newbie Wine hacker was asking how he could debug an app given that he couldn't reach the buggy part with debug logging on. Needless frustration for new developers! Exactly what a project facing manpower issues doesn't need. * Building winetest by default, despite it crushing less powerful machines (gas really hates trying to assemble 20-30mb of input). The reasoning given was that otherwise it might bitrot or fail to build, but we already have automated systems to compile winetest that would notify us if it failed to build. At the very least it could be a configure option. Now, like everybody here I have huge respect for Alexandre and the work he does. I could not do it. But, I agree with Troy and Juan here. I see no reason not to push parallel branches or version control systems as a way to scale up the project. This would have two simple benefits: 1) It would allow sub-maintainership of certain DLLs, if we wanted to do that. 2) It would add some flexibility to the system so that if Alexandre makes a bad judgement call, it's possible to notice (because patches he rejects are being merged in to other trees, or vice-versa). This is what the kernel project does, and it works well for them. Linus tends to be quite conservative, so other maintainers can merge in more risky patches and find out if they work. At the very least we should try and do more to attack the 90 degree learning curve associated with Wine development. Documentation is good but it seems nobody is really working on pulling new developers in any more. A sentiment I've heard echoed in the past is that Wine is just hard - well, it *is* hard, but then so is kernel (technical) and GNOME (user interface) development yet these projects have many developers. We should try and figure out why. thanks -mike
participants (7)
-
Alexandre Julliard -
Francois Gouget -
Ivan Leo Puoti -
Juan Lang -
Mike Hearn -
Robert Lunnon -
Robert Shearman