June 2011 - Wine-Devel - list.winehq.org

Re: TaskDialog implementation

by Hans Leidekker

On Fri, 2011-06-10 at 23:29 -0500, Vincent Povirk wrote: > Well, I think you may be right that changing it to E_NOTIMPL at this > point would break something and might be a bad idea. Sadly, Hans > didn't say what the stub was originally for. Hopefully, we'll soon > have a working implementation and it won't matter. ;) I added it for the Internet Explorer 9 installer but it has been broken already when someone changed it to return IDYES instead of IDOK in pnButton. I don't know which app that was for.

June 11, 2011

Re: TaskDialog implementation

by Vincent Povirk

> On 06/09/11 10:55, Vincent Povirk wrote: >> + if (pnButton) *pnButton = IDYES; >> + if (pnRadioButton) *pnRadioButton = pTaskConfig->nDefaultButton; >> + if (pfVerificationFlagChecked) *pfVerificationFlagChecked = TRUE; >> + return S_OK; >> >> I don't think it's appropriate to make selections for the user like >> this. If you can't present all of the choices to the user, you should >> probably return E_NOTIMPL. > > Yes, I agree, if I had made that stub myself I would have returned > E_NOTIMPL, but I merely moved it as-is from commctrl.c, I assume it was > put there that way quickly to make some application work, I did not want > to break anything. Ah, you're right, I didn't notice that. > Thing is, as I mentioned earlier, the code is already all written and > working. (You can download the DLL from my website and see for yourself) Yeah, splitting an existing implementation can be tough. On the other hand, the various features (progress bars, radio buttons, that dialog has a lot of stuff going on..) should be written so that they're independent from the other features and you can split them out in a sensible way (though it's still a hassle to actually do it). I guess many of the elements could be shown or hidden, and each one affects the dialog size and the placement of everything else. But if you do this cleanly, it should still be easy to add each element. I was hoping to read your existing code and see if anything struck me, but it seems you haven't provided it. > I would like some guidance on what constitutes an adequate patch size > then. According to this, I shoud make a commit into my local git every > time I do something simple. I can (and will) split this patch as you > mention, but I fail to see how I could split my patches as I get into > the meat of my implementation (as I mentioned earlier). Input in this > area would be appreciated. As I see it, the following guidelines apply: * The code must compile without warnings and pass all the tests after each patch is applied. Any non-passing tests must be marked as todo_wine. (As a consequence of this, if you fix Wine so that a failing test passes, you must remove the todo_wine as part of that same commit.) * No patch should introduce a memory leak, regression, or other bad thing. If you absolutely cannot avoid doing this (I think it's happened to me once), make a note that you've done this so that your badness doesn't get accepted without the later fix. * Try to avoid introducing unused code, resources, etc. That means adding internal functions as you begin to use them. (However, things like dialog or menu resources can be added as a whole, even if individual controls/menus are unused/unimplemented.) I've had to break this rule once or twice as well. * Each patch should contain a single logical change. * If you can split your patch into multiple logical changes without breaking anything in between, you probably should. * In some cases, you can get away with having two logical changes in a patch, if one of the changes is sufficiently small and related to the other. An example of a small change might be adding a function that only forwards to another that you implement (like TaskDialog/TaskDialogIndirect), or adding a small test that you also fix in Wine. But it's always OK to send multiple patches instead. There's not really a guideline in terms of lines of code. It's more about how much logic you have in each patch, and how much it relates to the other logic. If it takes a long time for a reader (who we assume is already familiar with the code you're modifying) to understand all the logic you've introduced and how it all relates to each other, that's a sign that your patch is too big. I would like to point out that your first patch was marked "Pending", not "Needs Splitting" (and also not "Needs tests"). That means Alexandre didn't necessarily object to the number of different things you changed in the patch, but he was uncomfortable with something about one of those changes, and now we don't know which it was. Anything you hear from anyone other than Alexandre is, of course, speculation. (I see "Pending" as meaning "This makes me nervous" while "Rejected" means "I know for sure this is wrong".) > Also how do I write a test that tests the ordinals? By checking that > GetProcAddress(hMod, "TaskDialogIndirect") == GetProcAddress(hMod, "#345") ? No, that would be silly. The most you should need to do there is explain in more detail why you think the ordinal is important and unlikely to change. But it seems to me you already explained that well enough (though I personally wouldn't know how to verify it), and so I really don't think the ordinals are the sticking point. (Come to think of it, there's a way to specify in the .spec file that programs should link by ordinal, and if MS does it then we probably should too. I think it's -ordinal.) Now that I understand you're not making the existing stub worse, and given that your patch doesn't do very much, the "Pending" status is baffling to me. It makes me think I missed something obvious. Maybe the problem is that your TaskDialog function doesn't set dwFlags? That would likely result in other invalid fields being used when they shouldn't. From MSDN, it appears many of the other fields should be initialized as well. (But that should have resulted in a "Rejected" status, so maybe he didn't notice that. Hmm.) > What I seem to get from your input and reading SubmittingPatches, is > basically "tests, tests, tests". I thought a while about "how does one > automate tests on GUI stuff?" and "how can I ensure that my dialog > renders right in a test, short of using a flaky thing like GetPixel()?", > etc. > > Then it came to me. What I will do is first change the implementation to > return E_NOTIMPL as Vincent Povirk suggested. Then I will start writing > tests that show use TaskDialogIndirect and sends a bunch of TDM_xxx > messages and tests that the callback procedures receives all the right > TDN_xxx notifications, at least. It won't be able to test that the > dialog _displays_ right, but at least that it functions right from an > API point of view. As long as I can close the dialog when done testing, > the tests should not require any kind of user interaction. Well, I think you may be right that changing it to E_NOTIMPL at this point would break something and might be a bad idea. Sadly, Hans didn't say what the stub was originally for. Hopefully, we'll soon have a working implementation and it won't matter. ;) I definitely think testing the notifications is the right idea, and making sure the display and interaction are correct is not as worthwhile. It's still possible to test display and interaction by accessing individual controls by id and using messages to interact with them. But I don't think it's worth doing that unless real programs are known to do that (they often do this with other common dialogs). I also don't know to what extent it's worth making your dialog template compatible with windows, or how to avoid potential copyright infringement when doing that. I hope someone else can chime in on this issue. I think we may overemphasize the importance of tests. They are not always necessary for getting code into Wine, as our current spotty test coverage proves. Still, they are really nice to have.

June 11, 2011

Re: Ge (Greg) van Geldorp

by Roderick Colenbrander

I'm sad to hear that Ge has passed away. He was really a nice guy and his contributions to Wine have been very helpful. We zullen hem missen. Roderick

June 11, 2011

Re: Ge (Greg) van Geldorp

by James McKenzie

On 6/10/11 2:59 PM, Austin English wrote: > On Fri, Jun 10, 2011 at 16:37, Wolfram Sang<wolfram(a)the-dreams.de> wrote: >>> The sad news reached me two days ago that Ge (Greg) van Geldorp passed >>> away. Please find below the mail from his brother. >> This is sad news indeed. The testbot he created is tremendously useful and a >> great achievement. He also was always helpful and nice when I asked him >> about it. His presence will be missed, yet he will be remembered. >> Condolences to his family and friends. > My condolences to his family and friends as well. His contributions > were greatly appreciated and won't be forgotten. > > As a more permanent memorial, perhaps we could dedicate the next > (stable) Wine release to his memory. > Ge's efforts with VMWare got all of the Wine Developers access for no cost to VMWare Fusion and his construction and maintenance of the Testbot system are appreciated by all the developers who use it. Without it, we would not be where we are today in the level and depth of Wine development and conformance testing. My condolences to his family, his friends and those in the Wine community that met him, either in person or 'over the wire'. His presence here will be missed and is one to be emulated. James McKenzie

June 11, 2011

Re: [PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

by Juan Lang

> It would be a lot easier to find and fix real things if there weren't a > thousand false ones hanging around. But if you feel it's better to have a > lot of warnings and some possible bugs than a lot of checks and no bugs, > then maybe I'll not waste any more time "fixing" them. This is a false dichotomy. LLVM/Clang uses static analysis, which is a sound technique, i.e. it produces no false negatives. The downside is that it will always produce false positives. Filtering out false positives is a necessary component of any bug fixing exercise using static analysis. I'd argue that this is one such example, that the fix isn't worth the noise, as there's no actual bug being fixed. --Juan

June 11, 2011

Re: TaskDialog implementation

by Patrick Gauthier

(Sorry for the late reply. BSD dev machine had some hard disk issues.) Thanks for all your input. Here are some comments: On 06/09/11 10:51, Juan Lang wrote: > The patch was marked as "Pending." That usually means Alexandre's > waiting for something, e.g. for you to fix something obvious, or to > see what else you're planning to do. Since you say the patch is > preparatory work, it doesn't make much sense to go in unless the > remaining patches are also to go in, so I'd suggest sending at least > another patch to show where you're going with it. Alright, I will follow-up with more first then. > Regarding splitting, sometimes it's useful to introduce code that will > only be removed later. I know this isn't the usual advice, but if you > introduce a new implementation of something, sometimes a stub > implementation can appear first. E.g., if it's an interface you're > implementing, introduce an implementation that does nothing but return > E_NOTIMPL from each method. Then one by one introduce implementation > for each function. Thing is, however, that I already got the full implementation written "as a block", short of "replaying my coding process", I don't see how I could efficiently do that. I could always make different patches for different parts, like, dialog creation, then layout, then message handling, etc, but you would not get a working TaskDialog until the last patch was integrated. On 06/09/11 10:55, Vincent Povirk wrote: > + if (pnButton) *pnButton = IDYES; > + if (pnRadioButton) *pnRadioButton = pTaskConfig->nDefaultButton; > + if (pfVerificationFlagChecked) *pfVerificationFlagChecked = TRUE; > + return S_OK; > > I don't think it's appropriate to make selections for the user like > this. If you can't present all of the choices to the user, you should > probably return E_NOTIMPL. Yes, I agree, if I had made that stub myself I would have returned E_NOTIMPL, but I merely moved it as-is from commctrl.c, I assume it was put there that way quickly to make some application work, I did not want to break anything. > I think Lats' advice here is sound. You might start with a dialog with > only an OK button (that only displays if the dialog offers no choices > to the user), then add features one at a time. > Well, you shouldn't write it all at once anyway, as you'd likely have > to change all of your later work. Thing is, as I mentioned earlier, the code is already all written and working. (You can download the DLL from my website and see for yourself) On 06/09/11 11:15, Dan Kegel wrote: > An iota of guidance was provided at > http://source.winehq.org/patches/ > Scroll down to your patch, you can see it's in "Pending" state. > Scroll down further, you'll see that "Pending" means > "The patch is not obviously correct at first glance. Making a more > convincing argument, preferably in the form of a test case, may help." > See also http://wiki.winehq.org/SubmittingPatches for a generic checklist. Thanks for those two sites, I did not know / forgot they existed. They have been bookmarked so I will check them first before asking. > Your patch seemed to do three things: > - moving old stub into new file > - adding a TaskDialog function > - adding ordinals > > Those three things could all be separate patches, > and some of them could have tests. I would like some guidance on what constitutes an adequate patch size then. According to this, I shoud make a commit into my local git every time I do something simple. I can (and will) split this patch as you mention, but I fail to see how I could split my patches as I get into the meat of my implementation (as I mentioned earlier). Input in this area would be appreciated. Also how do I write a test that tests the ordinals? By checking that GetProcAddress(hMod, "TaskDialogIndirect") == GetProcAddress(hMod, "#345") ? > I'd suggest sending just one of the three first (probably > not the one that moves code to a new file), > and including a test with the first patch. > - Dan Alright, will do. What I seem to get from your input and reading SubmittingPatches, is basically "tests, tests, tests". I thought a while about "how does one automate tests on GUI stuff?" and "how can I ensure that my dialog renders right in a test, short of using a flaky thing like GetPixel()?", etc. Then it came to me. What I will do is first change the implementation to return E_NOTIMPL as Vincent Povirk suggested. Then I will start writing tests that show use TaskDialogIndirect and sends a bunch of TDM_xxx messages and tests that the callback procedures receives all the right TDN_xxx notifications, at least. It won't be able to test that the dialog _displays_ right, but at least that it functions right from an API point of view. As long as I can close the dialog when done testing, the tests should not require any kind of user interaction. Of course any input is appreciated, I am still a newbie at this. - Patrick

June 10, 2011

Re: Ge (Greg) van Geldorp

by Maarten Lankhorst

Sad news indeed. His work on getting winetestbot integrated into wine was amazing. He was also nice every time I talked to him. I'll miss him. Hierbij mijn steunbetuiging aan zijn nabestaanden. ~Maarten

June 10, 2011

Re: [PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

by Lauri Kenttä

On Fri, 10 Jun 2011 14:19:43 -0700, Juan Lang wrote: > This change doesn't accomplish anything. Yes it does. It makes Clang happy and avoids a null pointer dereference in case the tests are failing. (Even if they're passing now, someone might break them in future.) > In the first place, the previous ok which you did not change already > ensures pdst is not NULL. Yes, I'm sure it does, but that has really nothing to do with this. The next line is run in either case, and if pdst is null, it will cause a segfault. > Second, the ok output message also dereferences pdst by printing it That's not true. "%s" formats null pointers as "(null)" instead of dereferencing anything. Feel free to try it yourself or see ntdll/printf.c:225 for such a check. > Really, I suggest you just ignore this warning, it's not worth the > extra baggage. It would be a lot easier to find and fix real things if there weren't a thousand false ones hanging around. But if you feel it's better to have a lot of warnings and some possible bugs than a lot of checks and no bugs, then maybe I'll not waste any more time "fixing" them. -- Lauri

June 10, 2011

Re: Ge (Greg) van Geldorp

by Austin English

On Fri, Jun 10, 2011 at 16:37, Wolfram Sang <wolfram(a)the-dreams.de> wrote: > >> The sad news reached me two days ago that Ge (Greg) van Geldorp passed >> away. Please find below the mail from his brother. > > This is sad news indeed. The testbot he created is tremendously useful and a > great achievement. He also was always helpful and nice when I asked him > about it. His presence will be missed, yet he will be remembered. > Condolences to his family and friends. My condolences to his family and friends as well. His contributions were greatly appreciated and won't be forgotten. As a more permanent memorial, perhaps we could dedicate the next (stable) Wine release to his memory. -- -Austin

June 10, 2011

Re: Ge (Greg) van Geldorp

by Wolfram Sang

> The sad news reached me two days ago that Ge (Greg) van Geldorp passed > away. Please find below the mail from his brother. This is sad news indeed. The testbot he created is tremendously useful and a great achievement. He also was always helpful and nice when I asked him about it. His presence will be missed, yet he will be remembered. Condolences to his family and friends. Wolfram

June 10, 2011

Re: [PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

by Juan Lang

Hi Lauri, ok(pdst != NULL, "inet_ntoa failed %s\n", dst); - ok(!strcmp(pdst, addr0_Str),"Address %s != %s\n", pdst, addr0_Str); + ok(pdst && !strcmp(pdst, addr0_Str),"Address %s != %s\n", pdst, addr0_Str); This change doesn't accomplish anything. In the first place, the previous ok which you did not change already ensures pdst is not NULL. While it's true that this ok could fail, leading to a following crash in the tests, the fact that the tests pass indicates this isn't happening. Second, the ok output message also dereferences pdst by printing it, so in the case that it is NULL, you haven't fixed anything. Really, I suggest you just ignore this warning, it's not worth the extra baggage. --Juan

June 10, 2011

Re: [PATCH 4/4] Make sure to use a return value. (LLVM/Clang)

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11621 Your paranoid android. === WNT4WSSP6 (32 bit sock) === sock.c:524: Test failed: oob_server (82): received less data than expected: 2024 of 2040 === WXPPROSP3 (32 bit sock) === sock.c:524: Test failed: oob_server (70c): received less data than expected: 2024 of 2040 === W2K3R2SESP2 (32 bit sock) === sock.c:524: Test failed: oob_server (b90): received less data than expected: 2024 of 2040 === WVISTAADM (32 bit sock) === sock.c:524: Test failed: oob_server (948): received less data than expected: 2024 of 2040 === W2K8SE (32 bit sock) === sock.c:524: Test failed: oob_server (bb0): received less data than expected: 2024 of 2040 === W7PRO (32 bit sock) === sock.c:524: Test failed: oob_server (2fc): received less data than expected: 2024 of 2040 === W7PROX64 (32 bit sock) === sock.c:524: Test failed: oob_server (440): received less data than expected: 2024 of 2040 === W7PROX64 (64 bit sock) === sock.c:524: Test failed: oob_server (3ec): received less data than expected: 2024 of 2040

June 10, 2011

Ge (Greg) van Geldorp

by Paul Vriens

Hi, The sad news reached me two days ago that Ge (Greg) van Geldorp passed away. Please find below the mail from his brother. I've briefly talked to Jeremy and Alexandre about this loss and the idea came up to have something on the winehq frontpage for a while. Jeremey also mentioned to mark Greg's life and contributions to Wine at Wineconf this year. Alexandre mentioned potentially hosting the winetestbot at codeweavers in the future. Both the frontpage remembrance and the potential hosting somewhere else has been discussed by me and Arno and will not be an issue. Cheers, Paul. -------- Original Message -------- Subject: RE: Ge van Geldorp Date: Thu, 9 Jun 2011 23:05:46 +0200 From: Erven Gé van Geldorp <erven.van.geldorp(a)gmail.com> To: 'Paul Vriens' <paul.vriens.wine(a)gmail.com> Hi all, My name is Arno van Geldorp. I'm the brother of Gé (Greg) van Geldorp. I'm very sorry that I have to inform you that Greg has passed away almost 3 weeks ago. 2 Months earlier he was diagnosed with cancer in a very aggressive form. I know he made his contribution to the Wine project. And that one of them is the WineTestBot. His passing away went so fast that he didn't have the time to inform me all about it. What he did ask me is to make sure that the server where the WineTestBot is running on will be hosted for at least 2 more years. So we will take care of that. But I don't know if anyone took over the administration off the server. If anyone took over the administration from Greg could you please contact me at erven.van.geldorp at gmail.com Kind regards, Arno van Geldorp

June 10, 2011

Re: testbot flakiness?

by Frédéric Delanoy

On Mon, Jun 6, 2011 at 03:17, Dan Kegel <dank(a)kegel.com> wrote: > https://testbot.winehq.org/JobDetails.pl?Key=11509 shows failures and > crashes, but the nearly identical > https://testbot.winehq.org/JobDetails.pl?Key=11510 does not. (The latter one > just adds a call to GetProcessMemoryInfo() in the xyz() logging function.) > > And the failure shown by the first patch has a last error > on creating the batch file of 8, ERROR_NOT_ENOUGH_MEMORY, which is suspicious. > > Not quite sure what's going on there. Actually it seems using "md" instead of "mkdir" gives those problems, although they should be perfect synonymous Compare https://testbot.winehq.org/JobDetails.pl?Key=11608 (md version; some failures) and https://testbot.winehq.org/JobDetails.pl?Key=11611 (mkdir; no failure)

June 10, 2011

Re: [1/2] d3dx9/test: Add UpdateSemantics test. (try 2)

by Michael Mc Donnell

On Fri, Jun 10, 2011 at 8:10 AM, Dylan Smith <dylan.ah.smith(a)gmail.com> wrote: >> +static void test_update_semantics(void) >> +{ >> ... >> + int offset = 12; > > sizeof(D3DXVECTOR3) might be clearer. > Ok, I'll change that along with your other comments about the patch for the UpdateSemantics implementation.

June 10, 2011

Re: [2/2] d3dx9: Implement UpdateSemantics mesh method. (try 2)

by Michael Mc Donnell

On Fri, Jun 10, 2011 at 8:08 AM, Dylan Smith <dylan.ah.smith(a)gmail.com> wrote: > Your patches have dos line endings (i.e. "\r\n") which causes git apply to > fail, although the patch command only gives a warning. Yeah I think this is because of git-imap was changed to convert the line endings on the fly. I only get the dos line endings if I downloaded the patch from my imap folder. Does anyone know how to make git-imap stop converting the line endings? > >> @@ -208,6 +221,12 @@ static DWORD WINAPI >> ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface) >> TRACE("iface (%p)\n", This); >> + if (!This->vertex_declaration) >> + { >> + WARN("Can't get number of bytes per vertex of an invalid vertex >> declaration.\n"); >> + return E_FAIL; >> + } > > GetNumBytesPerVertex returns a DWORD not an HRESULT, so I wouldn't expect > E_FAIL to ever be returned? Unusual behaviour like this should be covered by > a test. Thanks for catching that one. You're right I added the E_FAIL for GetNumBytesPerVertex too quickly with out testing. I'll write a test case for that. > >> + /* An application can pass an invalid declaration to UpdateSemantics >> and >> + * still expect D3D_OK (see tests). If the declaration is invalid, >> then >> + * subsequent calls to GetDeclaration, GetNumBytesPerVertex, and >> DrawSubset, >> + * which use the declaration, will fail. >> + */ > > D3DXCreateMesh fails when the declaration contains a non-zero Stream value. > I would expect UpdateSemantics to be as strict as D3DXCreateMesh, otherwise > a test could validate the different behaviour. Ok, I'll write a test for that too. > You tested a declaration with larger fields, but not with the vertex size > being smaller or greater. (E.g. Try removing the last field, or appending a > field.) Yeah I see I only test for larger overlapping fields, but not vertex declaration sizes being smaller or greater. I'll fix that. > I did some testing and couldn't get GetDeclaration or GetNumBytesPerVertex > to fail on windows. DrawSubset did seem to fail after UpdateSemantics is > called with a declaration that causes CreateVertexDeclaration to fail. This > suggests that the native implementation probably just stores the > D3DVERTEXELEMENT9 array and calls CreateVertexDeclaration on DrawSubset. You > can probably cache the created vertex declaration and release it on > UpdateSemantics for performance. You're right again. GetDeclaration returns the invalid declaration, which means that it is probably cached like you're describing. I'll add checks for that too.

June 10, 2011

Re: cmd: Ensure mkdir returns an error for pre-existing directories

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11601 Your paranoid android. === WVISTAADM (32 bit) === batch: unhandled exception c0000005 at 77496FCF === W2K8SE (32 bit) === batch: unhandled exception c0000005 at 778A8915 === W7PRO (32 bit) === batch.c:70: Test failed: CreateFile failed batch: unhandled exception c0000005 at 772D3067 === W7PROX64 (32 bit) === batch.c:96: Test failed: CreateProcess failed: 1450 === W7PROX64 (64 bit) === Timeout

June 10, 2011

Re: [2/2] advapi32: Use an architecture neutral structure to marshall service status information.

by Alexandre Julliard

Hans Leidekker <hans(a)codeweavers.com> writes: > ENUM_SERVICE_STATUSW and ENUM_SERVICE_STATUS_PROCESSW embed pointers so we cannot > use these structures on Wow64, where the service manager runs as a 64-bit process and > the querying process can be 32-bit. You should still keep the proper structure layout, for instance by using LONG_PTRs instead. You definitely don't want to make two server calls for every request. -- Alexandre Julliard julliard(a)winehq.org

June 10, 2011

Re: [2/5] msi: Pass the full custom action command to CreateProcess.

by Alexandre Julliard

Hans Leidekker <hans(a)codeweavers.com> writes: > @@ -772,10 +772,22 @@ static HANDLE execute_command( const WCHAR *exe, WCHAR *arg, const WCHAR *dir ) > { > STARTUPINFOW si; > PROCESS_INFORMATION info; > + int len; > + WCHAR *cmd; > BOOL ret; > > + len = strlenW( exe ); > + if (arg) len += strlenW( arg ) + 1; > + if (!(cmd = msi_alloc( (len + 1) * sizeof(WCHAR) ))) return INVALID_HANDLE_VALUE; > + strcpyW( cmd, exe ); > + if (arg) > + { > + strcatW( cmd, szSpace ); > + strcatW( cmd, arg ); > + } > memset( &si, 0, sizeof(STARTUPINFOW) ); > - ret = CreateProcessW( exe, arg, NULL, NULL, FALSE, 0, NULL, dir, &si, &info ); > + ret = CreateProcessW( NULL, cmd, NULL, NULL, FALSE, 0, NULL, dir, &si, &info ); You should still pass the exe name to CreateProcess in addition to the command line. You probably also want to quote it in the command line if it contains spaces. Also some callers pass a NULL exe. -- Alexandre Julliard julliard(a)winehq.org

June 10, 2011

Re: [5/5] msi: Implement and test MsiSetFeatureAttributes.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11595 Your paranoid android. === WNT4WSSP6 (32 bit install) === install.c:5979: Test failed: Per-user icon file isn't where it's expected (C:\WINNT\Profiles\Default User\Application Data\Microsoft\Installer\{7DF88A49-996F-4EC8-A022-BF956F9B2CBB}\testicon)

June 10, 2011

Re: [4/5] msi: Implement and test MsiGetFeatureInfo.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11594 Your paranoid android. === WNT4WSSP6 (32 bit install) === install.c:5979: Test failed: Per-user icon file isn't where it's expected (C:\WINNT\Profiles\Default User\Application Data\Microsoft\Installer\{7DF88A49-996F-4EC8-A022-BF956F9B2CBB}\testicon)

June 10, 2011

Re: [1/2] d3dx9/test: Add UpdateSemantics test. (try 2)

by Dylan Smith

> +static void test_update_semantics(void) > +{ > ... > + int offset = 12; sizeof(D3DXVECTOR3) might be clearer.

June 10, 2011

Re: [2/2] d3dx9: Implement UpdateSemantics mesh method. (try 2)

by Dylan Smith

Your patches have dos line endings (i.e. "\r\n") which causes git apply to fail, although the patch command only gives a warning. @@ -208,6 +221,12 @@ static DWORD WINAPI > ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface) > TRACE("iface (%p)\n", This); > + if (!This->vertex_declaration) > + { > + WARN("Can't get number of bytes per vertex of an invalid vertex > declaration.\n"); > + return E_FAIL; > + } GetNumBytesPerVertex returns a DWORD not an HRESULT, so I wouldn't expect E_FAIL to ever be returned? Unusual behaviour like this should be covered by a test. + /* An application can pass an invalid declaration to UpdateSemantics > and > + * still expect D3D_OK (see tests). If the declaration is invalid, > then > + * subsequent calls to GetDeclaration, GetNumBytesPerVertex, and > DrawSubset, > + * which use the declaration, will fail. > + */ D3DXCreateMesh fails when the declaration contains a non-zero Stream value. I would expect UpdateSemantics to be as strict as D3DXCreateMesh, otherwise a test could validate the different behaviour. You tested a declaration with larger fields, but not with the vertex size being smaller or greater. (E.g. Try removing the last field, or appending a field.) I did some testing and couldn't get GetDeclaration or GetNumBytesPerVertex to fail on windows. DrawSubset did seem to fail after UpdateSemantics is called with a declaration that causes CreateVertexDeclaration to fail. This suggests that the native implementation probably just stores the D3DVERTEXELEMENT9 array and calls CreateVertexDeclaration on DrawSubset. You can probably cache the created vertex declaration and release it on UpdateSemantics for performance.

June 10, 2011

Re: [1/2] d3dx9/test: Add UpdateSemantics test. (try 2)

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Looks ok to me Am 09.06.2011 um 17:50 schrieb Michael Mc Donnell: > Removed a superfluous test. > Added NULL check before releasing mesh. > Changed struct declaration style. > Changed the struct declaration style again. > --- > dlls/d3dx9_36/tests/mesh.c | 279 ++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 279 insertions(+), 0 deletions(-) > <0001-d3dx9-test-Add-UpdateSemantics-test.patch> -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQIcBAEBAgAGBQJN8VlIAAoJEN0/YqbEcdMw9a0P/R7/63zSoVb7sig95T+WQDDX HkASb2vhp9dgs65zNCA6Vm6mCqPh62aNa3IKThtrVRQcPmmPFndgz8KmHXHzYZMj 6aQsiaa+vL4Yf4qcub+KwMej/S/A5txmVuXmOtreqJAGAKL6B/QA59KKw46JAlHT 15hggvXCHx0HaMBOP+bhcAlEOCI1QMZb1ygkzOILZ6ooy+0gOfOzSfl20Fk4Iy6X fPNhtE+goV6tYnP8I1hJEBg2TFPlN1YF25SxFJ/CIcHaJHYGAAL8LquMciVOdZ1C KsSrQzFFBw9fySvZC8rkaMAW5jIg27njRs4QB2CGEd5OkFc4I2KqZ4f1EwS/zdHp hAlvJmSRzB7Tq08ToW1a2ebX6dHyNfLinCKKpsmo5jk4V1FVN2DTWjsQs/mCIbCF EoUF0/NyIJHocOa71IV1Xj5uN7dvcZyqrJ3ZBVwDKU4o6G7uIDus0gbTVpliqty/ aBcvhwtHx6donMVt+KqPpM23tvomCr553fw93lCbB55vdZvjUUcWY5lL1HDp7qbI lkwz6aNjUhsA21u7uqANnThqm25pXLElnYR8O6LuU3AfsdkJAodK5JZ6/50aleI9 t3/Ke6ugHeywrpvXJVVzzJT3cOqsF0lQBvGegOYlSCdHm1MxtVDpdP1M2wKMce0z 2eXYa2k0EP7H0oMVERbP =WyeT -----END PGP SIGNATURE-----

June 9, 2011

Re: setupapi: Make sure machine name is non-empty before failing (try 2)

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=11589 Your paranoid android. === WNT4WSSP6 (32 bit devinst) === No test summary line found

June 9, 2011