February 2008 - Wine-Devel - list.winehq.org

Patch tracking system - again...

by Steven Edwards

Hi, I'm not sure if this would even really be of use with most of the development being done on git anyway, but after re-reading the governance thread when the point of patch tracking was brought up, It got me thinking. last time around it got shelved as normal due to not wanting to disrupt Alexandre's work flow. Thinking about it a bit more I think I've come up with a solution to the problem that should not disrupt Alexandre's flow if I understand git properly. Let me preface this with saying that I don't. At first I thought we might be able to use the sha1 hashes that are the patch names themselves to track if a commit had be accepted or not, but then I guess that would only work if we were pushing and pulling directly in to each others tree's as I see the commit hash differs from the patch hash in the patches I've reviewed. I can only assume this is because of the way git views looks at the entire repo as one chunk of data rather than dealing with single patches. Then as I dug in to it a bit more I noticed the index's did match provided Alexandre did not make any changes before he committed. I was thinking this could be a use for a tracking system. Lets imagine there is this patch daemon that sits out there and reads wine-patches, storing the diff and parsing the index in to a database. This deamon could do a git fetch every 30 mins or so to compare and see if a patch has been merged in based on the index. It seems like we already have most of the tools we need to do it now if someone really wants the tracking system. We know about the patches and we know if its been merged or not, defining rules for accepted or rejected seem pretty trivial to me as we already have the rule, if its not merged in a week then resend. So here is what it would look like as far as work flow goes, someone submits a patch, it goes to this database and up to a website with a listview showing all the patches. Anything not merged yet still in the two week window would show up in the list as white meaning still pending. The further up the list we would have patches that were merged and accepted as is, like the following (showing up in green) http://www.winehq.org/pipermail/wine-patches/2008-February/049719.html http://source.winehq.org/git/wine.git/?a=commit;h=ac61b90f87a2062b99f6f7068… The following patch would show up in yellow as the index differs for the Makefile part of the patch as Julliard made local changes http://www.winehq.org/pipermail/wine-patches/2008-February/049720.html http://source.winehq.org/git/wine.git/?a=commit;h=5bad12c6bc3072a2676f0d9ed… Any other patches that don't get merged would show up in the list as red after the one week window went by. The tracker would assume they were rejected. The cool thing about this is that we could directly link it in to bugzilla so that when a patch is applied the bug could get automatically updated. Now this all hinges on being able to use the diff index to do the tracking. I imagine there might be conflicts if your trying to use that as the only data point as its not as long as a real hash, but assuming your only doing a compare over as period of a few weeks or a month I think it would work. Am I wack? Is there even still the desire to have a graphical patch tracking system? -- Steven Edwards "There is one thing stronger than all the armies in the world, and that is an idea whose time has come." - Victor Hugo

Feb. 8, 2008

Re: Code quality (was Re: comctl32: Fix invalid syntax)

by Dmitry Timoshkov

"Bang Jun-young" <junyoung(a)mogua.com> wrote: >> People prefer to follow Alexandre's tree but nobody forces them to use it. > > If winehq.org is Alexandre's own property, this is something > fundametally wrong again. You may try to learn how things work in the Wine project by reading the following thread: http://www.winehq.org/pipermail/wine-devel/2006-September/050915.html and I'd suggest you very carefully read what Mike said: http://www.winehq.org/pipermail/wine-devel/2006-September/051163.html -- Dmitry.

Feb. 8, 2008

re: Code quality (was Re: comctl32: Fix invalid syntax)

by Dan Kegel

Bang Jun-young wrote: >In fact, this is a well known mistake many newbie Win32 developers >make (and fix in minutes). It shouldn't have been in the tree in the >first place if he actually have read the patch. There are a lot of >easily catchable bugs in the tree, for example, potential security >holes like buffer overrun, meaningless comparison of unsigned < 0 (or > > 0), misuse of BOOL vs. HRESULT, misuse of functions such as > strcasecmp(), use of non-portable syntax, etc. Most of them (if not > all) could be filtered out if he have read the patches carefully. >That's the main reason why Wine keeps crashing every time I give it a >try with my Windows apps. I don't think so. In my experience, it's either a missing feature or something more subtle. Outright bugs of the kind you describe are the minority. I think. > Since 1993, Wine has never gotten to the > point where everybody could rely on it for his daily work. It has as > awfully many bugs as Win95. I see something fundamentally > wrong with development process. Nah, Alexandre usually does a good job of screening patches. We're lucky to have such a good maintainer, IMHO. (I say this knowing full well that newbies are put off by how difficult it is to get feedback because I have seen much, much worse maintainers. Imagine if, say, that guy who always yells at the newbies to not paste logs in bug reports was the maintainer. We'd probably lose all our developers in a month.) So you're peeved you don't have commit access. That's understandable, but it's time to get over it. IMHO the single-committer system works pretty well once you get into the swing of it. It actually forces at least one code review on every patch! - Dan

Feb. 8, 2008

Code quality (was Re: comctl32: Fix invalid syntax)

by Bang Jun-young

On 2/7/08, Michael Stefaniuc <mstefani(a)redhat.com> wrote: > Bang Jun-young wrote: > > On 2/7/08, Alexandre Julliard <julliard(a)winehq.org> wrote: > >> "Bang Jun-young" <junyoung(a)mogua.com> writes: > >> > >>> 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 (although > >>> Alexandre doesn't seem to care about it). > >> 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. > > > > That's because you wanted yourself to be the only committer in the > > project. Why don't you give commit privilege to other Wine > > contributors as well? > Huh? Why would you need that? Because I'm concerned with the code quality of Wine. When Alexandre commits what he doesn't understand, it's a red sign to the project. Months ago I posted a patch similar to this one to wine-patches, and he committed it: typedef struct _OLEOBJECTVTBL { - void CALLBACK *(*QueryProtocol)(_LPOLEOBJECT,LPCOLESTR16); + void * (CALLBACK *QueryProtocol)(_LPOLEOBJECT,LPCOLESTR16); How did he know what the patch does from the same message "Fix invalid syntax?" IIRC, he didn't tell anything about what he doesn't understand. In fact, this is a well known mistake many newbie Win32 developers make (and fix in minutes). It shouldn't have been in the tree in the first place if he actually have read the patch. There are a lot of easily catchable bugs in the tree, for example, potential security holes like buffer overrun, meaningless comparison of unsigned < 0 (or > 0), misuse of BOOL vs. HRESULT, misuse of functions such as strcasecmp(), use of non-portable syntax, etc. Most of them (if not all) could be filtered out if he have read the patches carefully. That's the main reason why Wine keeps crashing every time I give it a try with my Windows apps. Since 1993, Wine has never gotten to the point where everybody could rely on it for his daily work. It has as awfully many bugs as Win95. I see something fundamentally wrong with development process. > Wine uses git, a distributed SCM. You are > free to publish your own Wine tree without waiting for Alexandre. 99% of the world won't benefit from what I have done in my own local repository. People usually don't have a much time to keep track of repositories of hundreds of developers around the world. That's why every open source project has the main repository even if developers usually maintain their own repostories at home. > People prefer to follow Alexandre's tree but nobody forces them to use it. If winehq.org is Alexandre's own property, this is something fundametally wrong again.

Feb. 8, 2008

Re: War on conformance test suite failures!

by Reece Dunn

On 05/02/2008, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > Am Dienstag, 5. Februar 2008 07:59:58 schrieb Reece Dunn: > > On 05/02/2008, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > > > Am Dienstag, 5. Februar 2008 00:50:55 schrieb Dan Kegel: > > > > Great progress was made at Wineconf last year on > > > > getting our test suite closer to always passing on wine, but > > > > http://test.winehq.org/data/200801301937/#Wine shows > > > > that, according to the results collected from people who run > > > > programs/winetest on Wine, twelve DLLs' tests are currently failing. > > > > (Let alone the test failures on Windows!) > > > > http://bugs.winehq.org/show_bug.cgi?id=9916 also > > > > talks about the fact that "make test" shows failures. > > > > > > This is only half on-topic, but the d3d9:visual test ran on this machine > > > has some interesting results: > > > > > > Vista: Vista-HomePrem-NoUAC > > > > > > Do we have any track who reported them? > > > > That one is mine, along with Vista-HomePrem-UAC. They are both on the > > same high-spec recent HP laptop (HP Pavilion DV95000) machine with an > > NVidia GeForce 8600M GS graphics card. > Do you have Half Life 2? If yes, can you start a new game and take a few > screenshots from Doktor Kleiner's lab? If you don't have the game I'll try to > find a place to reproduce the fog bug at a place that can be reached in the > demo. No, I don't have the game, sorry. - Reece

Feb. 7, 2008

Re: comctl32: Fix invalid syntax.

by Michael Stefaniuc

Bang Jun-young wrote: > On 2/7/08, Alexandre Julliard <julliard(a)winehq.org> wrote: >> "Bang Jun-young" <junyoung(a)mogua.com> writes: >> >>> 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 (although >>> Alexandre doesn't seem to care about it). >> 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. > > That's because you wanted yourself to be the only committer in the > project. Why don't you give commit privilege to other Wine > contributors as well? Huh? Why would you need that? 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. bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 Consulting Communications Engineer Fax.: +49-711-96437-111 -------------------------------------------------------------------- Reg. Adresse: Red Hat GmbH, Hauptstätter Strasse 58, 70178 Stuttgart Handelsregister: Amtsgericht Stuttgart HRB 153243 Geschäftsführer: Brendan Lane, Charlie Peters, Michael Cunningham, Werner Knoblich

Feb. 7, 2008

Re: comctl32: Fix invalid syntax.

by Bang Jun-young

On 2/7/08, Alexandre Julliard <julliard(a)winehq.org> wrote: > "Bang Jun-young" <junyoung(a)mogua.com> writes: > > > 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 (although > > Alexandre doesn't seem to care about it). > > 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. That's because you wanted yourself to be the only committer in the project. Why don't you give commit privilege to other Wine contributors as well? > 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. The patch is so obvious that I presumed you need no explanation.

Feb. 7, 2008

Re: gdi32: correct the test message in font.c that was out of sync with the test. [RESEND]

by Reece Dunn

On 07/02/2008, Alexandre Julliard <julliard(a)winehq.org> wrote: > "Reece Dunn" <msclrhd(a)googlemail.com> writes: > > > diff --git a/dlls/gdi32/tests/font.c b/dlls/gdi32/tests/font.c > > index 7e560d2..bfdb0af 100644 > > --- a/dlls/gdi32/tests/font.c > > +++ b/dlls/gdi32/tests/font.c > > @@ -604,7 +604,7 @@ static void test_GetGlyphIndices(void) > > flags |= GGI_MARK_NONEXISTING_GLYPHS; > > charcount = pGetGlyphIndicesW(hdc, testtext, (sizeof(testtext)/2)-1, glyphs, flags); > > ok(charcount == 5, "GetGlyphIndices count of glyphs should = 5 not %d\n", charcount); > > - ok((glyphs[4] == 0x001f || glyphs[4] == UNICODE_NOCHAR /* Vista */), "GetGlyphIndices should have returned a nonexistent char not %04x\n", glyphs[4]); > > + ok((glyphs[4] == 0x001f || glyphs[4] == UNICODE_NOCHAR /* Vista */), "GetGlyphIndices should have returned a nonexistent char or UNICODE_NOCHAR, not %04x\n", glyphs[4]); > > UNICODE_NOCHAR is a nonexistent char too. And if someone really cares > about the exact details they can always look at the test, we don't need > to duplicate all of it in the error message. Ok, I'll drop this patch from my list. - Reece

Feb. 7, 2008

Re: user32: make the listbox tests pass in directories other than dlls/user32/tests. [RESEND]

by Alexandre Julliard

"Reece Dunn" <msclrhd(a)googlemail.com> writes: > diff --git a/dlls/user32/tests/listbox.c b/dlls/user32/tests/listbox.c > index 3645190..f1a7b2b 100644 > --- a/dlls/user32/tests/listbox.c > +++ b/dlls/user32/tests/listbox.c > @@ -560,6 +560,8 @@ static void test_listbox_LB_DIR() > char * p; > char driveletter; > > + fclose( fopen( "wtest1.tmp.c", "w" ) ); Using stdio is not really appropriate here, CreateFile() would be a better choice. -- Alexandre Julliard julliard(a)winehq.org

Feb. 7, 2008

Re: gdi32: correct the test message in font.c that was out of sync with the test. [RESEND]

by Alexandre Julliard

"Reece Dunn" <msclrhd(a)googlemail.com> writes: > diff --git a/dlls/gdi32/tests/font.c b/dlls/gdi32/tests/font.c > index 7e560d2..bfdb0af 100644 > --- a/dlls/gdi32/tests/font.c > +++ b/dlls/gdi32/tests/font.c > @@ -604,7 +604,7 @@ static void test_GetGlyphIndices(void) > flags |= GGI_MARK_NONEXISTING_GLYPHS; > charcount = pGetGlyphIndicesW(hdc, testtext, (sizeof(testtext)/2)-1, glyphs, flags); > ok(charcount == 5, "GetGlyphIndices count of glyphs should = 5 not %d\n", charcount); > - ok((glyphs[4] == 0x001f || glyphs[4] == UNICODE_NOCHAR /* Vista */), "GetGlyphIndices should have returned a nonexistent char not %04x\n", glyphs[4]); > + ok((glyphs[4] == 0x001f || glyphs[4] == UNICODE_NOCHAR /* Vista */), "GetGlyphIndices should have returned a nonexistent char or UNICODE_NOCHAR, not %04x\n", glyphs[4]); UNICODE_NOCHAR is a nonexistent char too. And if someone really cares about the exact details they can always look at the test, we don't need to duplicate all of it in the error message. -- Alexandre Julliard julliard(a)winehq.org

Feb. 7, 2008