Re: Patchwatcher online
Dan Kegel wrote: On Mon, Aug 11, 2008 at 4:31 PM, Vijay Kiran Kamuju [EMAIL PROTECTED] wrote: When running tests for the patch, I think we should just run the tests of the dlls that are affected direct;y or indirectly by that change. its running the tests for entire wine, which is very time consuming. True, but hey, it was easier to code. And getting anything like this working at all is pretty hard. Figuring out which tests a give patch affects is an extra challenge I'd rather not face just now. Once it's up and working well we can refine it. I'd argue that testing just the affected dll is correct. What about things like patches to ntdll/kernel32/advapi32 (and the likes). They could influence far more tests then just the ones for it's own dll. -- Cheers, Paul.
Re: Patchwatcher online
On Tue, Aug 12, 2008 at 1:01 AM, Paul Vriens [EMAIL PROTECTED] wrote: Dan Kegel wrote: On Mon, Aug 11, 2008 at 4:31 PM, Vijay Kiran Kamuju [EMAIL PROTECTED] wrote: When running tests for the patch, I think we should just run the tests of the dlls that are affected direct;y or indirectly by that change. its running the tests for entire wine, which is very time consuming. True, but hey, it was easier to code. And getting anything like this working at all is pretty hard. Figuring out which tests a give patch affects is an extra challenge I'd rather not face just now. Once it's up and working well we can refine it. I'd argue that testing just the affected dll is correct. What about things like patches to ntdll/kernel32/advapi32 (and the likes). They could influence far more tests then just the ones for it's own dll. Which is an argument for why you should test the entire tree for each patch. If a patch to ntdll makes tests in another dll fail, the patch should be rejected just as if the ntdll tests failed. This is how Julliard's work flow goes, so the test bot should do the same. -- James Hawkins
Re: [TRY 2] Add tests for checking the parent notification for editcontrol
Ilya Shpigor [EMAIL PROTECTED] wrote: - /* why do we notify to es-hwndParent, and we send this one to GetParent()? */ + /* All notifies are send to es-hwndParent + * except the WM_CTLCOLORSTATIC and WM_CTLCOLOREDIT. + * Its are send to the current parent. + */ hbrush = (HBRUSH)SendMessageW(GetParent(es-hwndSelf), msg, (WPARAM)hdc, (LPARAM)es-hwndSelf); if (!hbrush) hbrush = (HBRUSH)DefWindowProcW(GetParent(es-hwndSelf), msg, (WPARAM)hdc, (LPARAM)es-hwndSelf); My question regarding this part left unanswered. Are there the tests confirming your statement in the comment? Also, please avoid adding not related message tests. If you are trying to show which parent receives the notification you need to create your own parent window procs or somehow tell an existing one to mark the notifications, so that it would be easy to distinguish the source of a notification. -- Dmitry.
Re: [TRY 3] Add tests for checking the parent notification for editcontrol
Ilya Shpigor [EMAIL PROTECTED] wrote: What is the difference between wpParent1 and wpParent2 besides slightly different indentation and 1 superfluous 'break' statement? - /* why do we notify to es-hwndParent, and we send this one to GetParent()? */ + /* All notifies are send to es-hwndParent + * except the WM_CTLCOLORSTATIC and WM_CTLCOLOREDIT. + * Its are send to the current parent. + */ hbrush = (HBRUSH)SendMessageW(GetParent(es-hwndSelf), msg, (WPARAM)hdc, (LPARAM)es-hwndSelf); if (!hbrush) hbrush = (HBRUSH)DefWindowProcW(GetParent(es-hwndSelf), msg, (WPARAM)hdc, (LPARAM)es-hwndSelf); This tests confirming my statement in the comment. If we change GetParent(es-hwndSelf) to es-hwndParent the tests will be failed. I don't see the tests confirming the comment. Why are you adding the whole new message handling code to edit.c (which complicates things a lot, and makes it less clear what you are trying to do) while msg.c already has everything required, and it already contains the edit message tests? -- Dmitry.
patchwatcher online
couldn't you instead when the patchwatcher takes the patch it assigns it a patch # and require if there is a patch dependency? that the person put into a comment REQ_PATCH: 123456,15456, etc.. ? That way when a diff is done for the patch it would appear in the patch diff? Then patchwatcher would make sure thats in the tree if its accepted and if that req. patch was not accepted then the new patch is automatically rejected? Chris
Re: Patchwatcher online
Am Montag, den 11.08.2008, 17:34 -0700 schrieb Dan Kegel:
Yes. I already changed the success message to make more sense,
and added background colors of green and red for success and failure.
I dislike the implementation, while I like the idea. You now have:
a:visited { color: #FF; }
.fail { background color: #ff5050; }
At least on my laptop display, #ff5050 on #ff is quite hard to read.
Regards,
Michael Karcher
Re: patchwatcher online
On Tue, Aug 12, 2008 at 6:47 AM, [EMAIL PROTECTED] wrote: couldn't you instead when the patchwatcher takes the patch it assigns it a patch # and require if there is a patch dependency that the person put into a comment REQ_PATCH: 123456,15456, etc.. ? Yes, perhaps if patchwatcher catches on and becomes a central part of Wine's workflow, that would be worth it.
Re: Patchwatcher online
Yes. I already changed the success message to make more sense,
and added background colors of green and red for success and failure.
I dislike the implementation, while I like the idea. You now have:
a:visited { color: #FF; }
.fail { background color: #ff5050; }
At least on my laptop display, #ff5050 on #ff is quite hard to read.
I think making the a:visited link for pass or fail to be balck is good enough.
I think this should do the trick.
.pass a:visited { color: #00; }
.fail a:visited { color: #00; }
---
VJ
Re: Patchwatcher online
Michael Karcher [EMAIL PROTECTED] wrote:
Yes. I already changed the success message to make more sense,
and added background colors of green and red for success and failure.
I dislike the implementation, while I like the idea. You now have:
a:visited { color: #FF; }
.fail { background color: #ff5050; }
At least on my laptop display, #ff5050 on #ff is quite hard to read.
Yeah, I know. I fiddled with the colors for a while, but not very effectively.
I'm partly color-blind, and am not really the best person to
work on the look of the reports page. If somebody else would like
to get the colors right, I'd gladly accept patches.
Would using black for foreground uniformly be more acceptable?
Re: [advapi32] - Add stub for ConvertToAutoInheritPrivateObjectSecurity
On Mon, Aug 11, 2008 at 11:59 PM, Dmitry Timoshkov
[EMAIL PROTECTED] wrote:
Vijay Kiran Kamuju [EMAIL PROTECTED] wrote:
+BOOL WINAPI ConvertToAutoInheritPrivateObjectSecurity(
+PSECURITY_DESCRIPTOR ParentDescriptor,
+PSECURITY_DESCRIPTOR CreatorDescriptor,
+PSECURITY_DESCRIPTOR* NewDescriptor,
+GUID* ObjectType,
+BOOL IsDirectoryObject,
+PGENERIC_MAPPING GenericMapping )
+{
+FIXME(%p %p %p %p %d %p - stub\n, ParentDescriptor,
CreatorDescriptor,
+ NewDescriptor, ObjectType, IsDirectoryObject, GenericMapping);
+
+return FALSE;
+}
It would be better to not copy variable names from MSDN/PSDK and come up
with your own naming. Say, (parent, creator, new, type, is_directory,
mapping)
would work.
I just used the CreatePrivateObjectSecurity as an example, it used
MSDN/PSDK naming.
--- a/include/winbase.h
+++ b/include/winbase.h
@@ -1373,6 +1373,7 @@ WINBASEAPI HANDLE WINAPI
CreateNamedPipeW(LPCWSTR,DWORD,DWORD,DWORD,DWORD,D
#define CreateNamedPipe
WINELIB_NAME_AW(CreateNamedPipe)
WINBASEAPI BOOLWINAPI
CreatePipe(PHANDLE,PHANDLE,LPSECURITY_ATTRIBUTES,DWORD);
WINADVAPI BOOLWINAPI
CreatePrivateObjectSecurity(PSECURITY_DESCRIPTOR,PSECURITY_DESCRIPTOR,PSECURITY_DESCRIPTOR*,BOOL,HANDLE,PGENERIC_MAPPING);
+WINADVAPI BOOLWINAPI
ConvertToAutoInheritPrivateObjectSecurity(PSECURITY_DESCRIPTOR,PSECURITY_DESCRIPTOR,PSECURITY_DESCRIPTOR*,GUID*,BOOL,PGENERIC_MAPPING);
WINBASEAPI BOOLWINAPI
CreateProcessA(LPCSTR,LPSTR,LPSECURITY_ATTRIBUTES,LPSECURITY_ATTRIBUTES,BOOL,DWORD,LPVOID,LPCSTR,LPSTARTUPINFOA,LPPROCESS_INFORMATION);
WINBASEAPI BOOLWINAPI
CreateProcessW(LPCWSTR,LPWSTR,LPSECURITY_ATTRIBUTES,LPSECURITY_ATTRIBUTES,BOOL,DWORD,LPVOID,LPCWSTR,LPSTARTUPINFOW,LPPROCESS_INFORMATION);
#define CreateProcess
WINELIB_NAME_AW(CreateProcess)
Please keep the entries alphabetically sorted.
Ok.
I will send another version with the changes soon.
---
VJ
Re: Patchwatcher online
Am Dienstag, den 12.08.2008, 08:26 -0700 schrieb Dan Kegel: Yeah, I know. I fiddled with the colors for a while, but not very effectively. I'm partly color-blind, and am not really the best person to work on the look of the reports page. If somebody else would like to get the colors right, I'd gladly accept patches. Would using black for foreground uniformly be more acceptable? Looks better now. UI suggestion: If tests fail, format the status column like this instead: a href=results/1.diff5 regressions/a in a href=results/1.logtests/a where results/1.diff just contains the diff output that is currently appended to the log file. I have the impression that the whole table just is too wide. For me, real name would definitely be shorter than e-mail address, but using Yet it would be even greater if my patches would get green. They seem to fail afoul flaky tests, but you are already aware of that problem. adresses might be advantageous because they are (a) unique and (b) definitely ASCII charset. Also subjects might be shortened by removing PATCH (I am sorry, but I didn't get git to number the series and not output PATCH, any hints welcome!) and cutting the subject after 40 characters. Still, besides all the critique: Great work! Regards, Michael Karcher PS: Yet patchwatcher would be even greater if my patches would get green. They seem to fail afoul flaky tests, but you are already aware of that problem.
Re: patchwatcher online
On Tue, Aug 12, 2008 at 10:29 AM, Reece Dunn [EMAIL PROTECTED] wrote: I use GMail to do something similar - tag mail I send to wine-patches with a 'wine-tracking' label, as well as the 'wine-patches' label it gets from the mailing list filter I have. This allows me to see all active patches I currently have. Once Alexandre commits them, I manually remove the 'wine-tracking' label; similarly if there are comments on it that mean I need to provide a revised patch. Potentially patchwatcher could monitor wine-devel for replies to the patch. The other patch monitoring systems out there do something similar; see http://bundlebuggy.aaronbentley.com/ and http://patchwork.ozlabs.org/linuxppc/ Where patchwatcher would be useful is to know whether Alexandre has looked at the patch yet (no more dly on #winehackers) and has rejected it - hopefully with comments as to why! Yes, but that's harder than: It should also help catch the patches that do not apply, or cause regressions in the tests, which should help Alexandre. which is my primary goal. It will also mean that patches are not lost in a flood (e.g. coming back off holiday). Perhaps, if it becomes a real workflow application, but resubmitting fresh patches is often required anyway... - Dan
Flaky tests are starting to get real annoying...
So, one of the things one learns when writing a patch robot is that flaky tests are very annoying. Each time it gets a new git tree, the robot does five baseline make -k test runs, remembers the tests that fail, and doesn't penalize patches for failing any of those tests. See http://code.google.com/p/winezeug/source/browse/trunk/patchwatcher/patchwatcher.sh#92 Annoyingly, that's not enough. Some tests stubbornly refuse to fail during the baseline test runs. So I added a second, manual blacklist for those tests; see http://code.google.com/p/winezeug/source/browse/trunk/patchwatcher/patchwatcher.sh#63 The list is currently user32:msg.c user32:input.c d3d9:visual.c ddraw:visual.c urlmon:protocol.c kernel32:thread.c and will continue growing as I keep plugging away at getting the patch robot happy. Is anybody else seeing this kind of flakiness? (If you're not, try running patchwatcher for a while :-) FWIW, I'm running the tests on hardy with a fresh metacity (as described in http://wiki.winehq.org/MakeTestFailures ).
Re: Patchwatcher online
On Mon, 2008-08-11 at 22:24 -0700, Dan Kegel wrote: Dmitry Timoshkov [EMAIL PROTECTED] wrote: Zachary Goldberg [EMAIL PROTECTED] wrote: Policy is that all patches should be independent, no? There is no such a policy. Dependent patches are marked as 1/xx, 2/xx, ... xx/xx. That's a patch series, and patchwatcher handles that ok. There's another kind of dependent patch, where somebody says This requires Harold's patch from yesterday. Patchwatcher probably isn't going to handle that ever. What about checking for a string in the email like patchwatchignore; if for some reason the patch is known to cause a failure the e-mail might read like: patchwatchignore This depends on Harald's patch from yesterday... [patch] This also might be useful for patches which update comments or README files, etc.
Re: Patchwatcher online
On Tue, Aug 12, 2008 at 1:39 PM, Adam Petaccia [EMAIL PROTECTED] wrote: What about checking for a string in the email like patchwatchignore; if for some reason the patch is known to cause a failure the e-mail might read like: patchwatchignore This depends on Harald's patch from yesterday... [patch] If patchwatcher becomes a gatekeeper, we will add a bypass like that. This also might be useful for patches which update comments or README files, etc. It's a lot easier to let the hardware run the tests, and as long as the hardware is fast enough to not get backlogged, the payoff for using the bypass for doc-only changes is small.
Re: patchwatcher online
2008/8/12 Dan Kegel [EMAIL PROTECTED]: On Tue, Aug 12, 2008 at 6:47 AM, [EMAIL PROTECTED] wrote: couldn't you instead when the patchwatcher takes the patch it assigns it a patch # and require if there is a patch dependency that the person put into a comment REQ_PATCH: 123456,15456, etc.. ? Yes, perhaps if patchwatcher catches on and becomes a central part of Wine's workflow, that would be worth it. I like the idea of patchwatcher. I use GMail to do something similar - tag mail I send to wine-patches with a 'wine-tracking' label, as well as the 'wine-patches' label it gets from the mailing list filter I have. This allows me to see all active patches I currently have. Once Alexandre commits them, I manually remove the 'wine-tracking' label; similarly if there are comments on it that mean I need to provide a revised patch. Where patchwatcher would be useful is to know whether Alexandre has looked at the patch yet (no more dly on #winehackers) and has rejected it - hopefully with comments as to why! It should also help catch the patches that do not apply, or cause regressions in the tests, which should help Alexandre. It will also mean that patches are not lost in a flood (e.g. coming back off holiday). So yeah, I like the idea. - Reece
Re: Flaky tests are starting to get real annoying...
Am Dienstag, den 12.08.2008, 10:58 -0700 schrieb Dan Kegel: The list is currently user32:msg.c user32:input.c Same problems here. Metacity 2.23.21, compiled myself. d3d9:visual.c ddraw:visual.c The last two are really nasty. Take a look at ddraw/tests/visual.c:2624. It makes a kind of basic assurance test whether it might be going to work. If this basic test fails, the whole visual test is skipped. The test passes only on my machine if it already fails the sanity check, otherwise I get some failures. Usually rerunning make test after a failed visual test runs into a sanity check failure. [That is on Intel 945 graphics hardware] urlmon:protocol.c kernel32:thread.c I don't rember these, but I most test runs I did were around 1.0, and these might have changed. Is anybody else seeing this kind of flakiness? (If you're not, try running patchwatcher for a while :-) Yes, I do, see above. Regards, Michael Karcher
Re: dplayx tests hanging
On 8/11/08, Ismael Barros [EMAIL PROTECTED] wrote: Actually the thread stuff is a very good idea, I'll take a look. I tried launching each tests in a new thread, but a lot of tests failures arise, sometimes even with segfaults or deadlocks. Looks like the original implementation of dplay doesn't like concurrency too much. The good news are that playing with the timeout value of EnumSessions I already managed to cut the time of some tests in half. -- ...yet even then, we ran like the wind, whilst our laughter echoed, under cerulean skies...
What to do when size of the struct differs on 32 and 64 bit?
While debugging some force-feedback issues ran into an interesting problem.
The size of one struct from include/linux differs between 32-bit and 64-bit.
That wouldn't be a major problem except that size is the part of the ioctl()
request. Which results in EINVAL.
In more details:
input.h:
#define EVIOCSFF _IOC(_IOC_WRITE, 'E', 0x80, sizeof(struct ff_effect))
The simple test program:
#include linux/input.h
#include stdio.h
int main(int argc, char * argv[])
{
printf(sizeof(struct ff_effect) = %d EVIOCSFF=%#x\n, sizeof(struct
ff_effect), EVIOCSFF);
return 0;
}
$ gcc test_size.c -o test_size ./test_size
sizeof(struct ff_effect) = 48 EVIOCSFF=0x40304580
$ gcc -m32 test_size.c -o test_size32 ./test_size32
sizeof(struct ff_effect) = 44 EVIOCSFF=0x402c4580
The question is what do we do about it? I'm sure there are might be more
cases like that.
Vitaliy
