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

Re: Coverity reports for test suite

by Alex Henrie

2017-02-26 15:11 GMT-07:00 Amine Khaldi <amine.khaldi(a)reactos.org>: > I have created a "wine-tests" component using .*/tests/.* and a "wine" > component for the remaining. Please let me know if you need anything else. Cool, thanks. I'm hoping to use this to debug and resolve some of the bug reports we have about running the tests with Valgrind. 2017-02-26 13:53 GMT-07:00 Alexandre Julliard <julliard(a)winehq.org>: > There's probably no harm in getting a scan once, but I'm concerned that > it would make people start "fixing" things that don't matter. There are a limited number of developers with access to Coverity, so I don't think we'll get too many people trying to fix things that aren't actually a problem. If it becomes a problem, we should be able to limit access or go back to hiding the test file reports. -Alex

Feb. 26, 2017

Re: Proposal: winetest 'context'

by Sebastian Lackner

On 25.02.2017 13:34, Józef Kucia wrote: > On Tue, Feb 14, 2017 at 5:04 PM, Mark Jansen <learn0more+wine(a)gmail.com> wrote: >> Hello, >> >> Often when testing something, helper functions are introduced that are >> called with different data (for example when iterating over tables). >> >> Attached patch (winetest_set_context.diff) is a proposal that allows >> tests to specify a 'context', that will be printed in the failure >> message. >> This way, the actual test code does not have to prefix every case with >> a trace, or include the data in every test macro. >> The patch is written so that tests do not have to use this mechanism, >> so there is no need to convert all tests at once. >> Second attachment (winetest_ctx_example.diff) shows how an existing >> test could be converted to use this new technique. > > For what it's worth, if we had a mechanism to set a context for test > messages, I would gladly use it in d3d11 tests. > > Cheers, > Józef I would also like to see such a feature, however I'm not really happy with a single context string which has to be set and reset manually. So, here is an alternative proposal which allows to use a block-syntax like: --- snip --- with_context("i = %d, j = %d", i, j) { // here some tests } --- snip --- Blocks can also be nested, and its easy to combine them with a loop: --- snip --- for (i = 0; i < 100; i++) with_context("test %d", i) { // here some tests (don't use break!) } --- snip --- The only disadvantage is that we need variadic macros (__VA_ARGS__), but it looks like they are already used in a bunch of other tests, so thats probably not a real problem. Please let me know what you think about this method, and also if you have suggestions for a better name. Best regards, Sebastian

Feb. 26, 2017

Re: [PATCH] rasapi32: Ensure device count matches data returned

by Bruno Jesus

On Sun, Feb 26, 2017 at 5:11 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > On 26.02.2017 20:41, Bruno Jesus wrote: >> From: Bruno Jesus <bjesus(a)codeweavers.com> >> >> Based on original patch by Qian Hong. >> >> Fixes bug https://bugs.winehq.org/show_bug.cgi?id=30378 >> >> Signed-off-by: Bruno Jesus <bjesus(a)codeweavers.com> >> --- >> dlls/rasapi32/rasapi.c | 1 + >> dlls/rasapi32/tests/rasapi.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/dlls/rasapi32/rasapi.c b/dlls/rasapi32/rasapi.c >> index fcd0cb8..f97d4c1 100644 >> --- a/dlls/rasapi32/rasapi.c >> +++ b/dlls/rasapi32/rasapi.c >> @@ -254,6 +254,7 @@ DWORD WINAPI RasEnumDevicesA(LPRASDEVINFOA lpRasDevinfo, LPDWORD lpcb, LPDWORD l >> if(lpRasDevinfo && lpRasDevinfo->dwSize != sizeof(RASDEVINFOA)) >> return ERROR_INVALID_SIZE; >> >> + *lpcDevices = 1; >> if (!lpRasDevinfo || (*lpcb < sizeof(RASDEVINFOA))) { >> *lpcb = sizeof(RASDEVINFOA); >> return ERROR_BUFFER_TOO_SMALL; >> diff --git a/dlls/rasapi32/tests/rasapi.c b/dlls/rasapi32/tests/rasapi.c >> index e0ff25f..e982f6c 100644 >> --- a/dlls/rasapi32/tests/rasapi.c >> +++ b/dlls/rasapi32/tests/rasapi.c >> @@ -77,80 +77,104 @@ static void test_rasenum(void) >> >> /* test first parameter */ >> cb = bufsize; >> + cDevices = 0xdead; >> result = pRasEnumDevicesA(NULL, &cb, &cDevices); >> ok(result == ERROR_BUFFER_TOO_SMALL || >> result == ERROR_INVALID_USER_BUFFER, /* win98 */ >> "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result); >> + ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices); >> >> rasDevInfo[0].dwSize = 0; >> cb = bufsize; >> + cDevices = 0xdead; >> result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); >> ok(result == ERROR_INVALID_SIZE || >> result == ERROR_INVALID_USER_BUFFER, /* win98 */ >> "Expected ERROR_INVALID_SIZE, got %08d\n", result); >> + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); >> >> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) -1; >> cb = bufsize; >> + cDevices = 0xdead; >> result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); >> ok(result == ERROR_INVALID_SIZE || >> result == ERROR_INVALID_USER_BUFFER, /* win98 */ >> "Expected ERROR_INVALID_SIZE, got %08d\n", result); >> + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); >> >> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) +1; >> cb = bufsize; >> + cDevices = 0xdead; >> result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); >> ok(result == ERROR_INVALID_SIZE || >> result == ERROR_INVALID_USER_BUFFER, /* win98 */ >> "Expected ERROR_INVALID_SIZE, got %08d\n", result); >> + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); >> >> /* test second parameter */ >> rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); >> result = pRasEnumDevicesA(rasDevInfo, NULL, &cDevices); >> + cDevices = 0xdead; > > Whats the point to set this after the function call? And also, how is this > better than Qian Hongs original patch? Using %u is more correct for unsigned > values, and the empty line after the assignment of *lpcDevices also didn't > hurt. Hi, thanks for the review. That was certainly a copy & paste issue. I see now that you have a staged patch for this that I completely missed I don't know how, sunday hours wasted for me... The main difference to Qian's patch is the place where lpcDevices = 1 is set but the staged patch is already correct too. Please send your patch in ;-)

Feb. 26, 2017

Re: include: Avoid overflow (warning) in RGBA_MAKE.

by Gerald Pfeifer

On Sun, 26 Feb 2017, Henri Verbeet wrote: >>>>> -#define RGBA_MAKE(r, g, b, a) ((D3DCOLOR) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))) >>>>> +#define RGBA_MAKE(r, g, b, a) ((D3DCOLOR) (((unsigned)(a) << 24) | ((r) << 16) | ((g) << 8) | (b))) > Commit 060ea15ae2c1604d51d3df1460e99c7d6e6211cf should have avoided the > issue. Yes, indeed, that looks like it. Thank you, Henri! Gerald

Feb. 26, 2017

Re: Coverity reports for test suite

by Amine Khaldi

On 26/02/2017 21:53, Alexandre Julliard wrote: > Nikolay Sivov <bunglehead(a)gmail.com> writes: > >> On 26.02.2017 21:32, Austin English wrote: >>> On Sun, Feb 26, 2017 at 12:09 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >>> FWIW, coverity supports putting issues into different components based >>> on filename. It would be trivial to set up two different components >>> for wine, e.g., 'wine' and 'wine-tests', then have wine/*/*/tests/* go >>> to wine-tests, and wine/* to wine. >>> >>> For those not familiar with coverity administration, this would not >>> require two separate builds, the results are separated when results >>> are displayed on the server. >>> >> If it's possible to make them appear separately, so tests don't affect >> our main reports, I see no harm in scanning tests too. > There's probably no harm in getting a scan once, but I'm concerned that > it would make people start "fixing" things that don't matter. > I have created a "wine-tests" component using .*/tests/.* and a "wine" component for the remaining. Please let me know if you need anything else. Regards, Amine.

Feb. 26, 2017

Re: [PATCH 2/2] wineinstall - Add support for parallel builds using 'nproc' to detect cpu count

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > On 25.02.2017 01:40, Austin English wrote: >> On Fri, Feb 24, 2017 at 6:30 PM, Chris Morgan <chmorgan(a)gmail.com> wrote: >>> Parallel builds can greatly reduce the overall build time on modern multi-core processors. >>> >>> Fall back to two parallel builds in the case where nproc is unavailable, most modern >>> processors have at least two cores. >>> >>> Use the 'NPROC' value defined by the environment if it is not null. >>> >>> Signed-off-by: Chris Morgan <chmorgan(a)gmail.com> >>> --- >>> tools/wineinstall | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/wineinstall b/tools/wineinstall >>> index bfd62e3..080d590 100755 >>> --- a/tools/wineinstall >>> +++ b/tools/wineinstall >>> @@ -143,10 +143,19 @@ echo "in the meantime..." >>> echo >>> std_sleep >>> >>> +# If the NPROC environment variable isn't set use nproc, if available, to determine >>> +# the number of processors in the system, if not, fall back to a default NPROC value >>> +if type nproc2 &>/dev/null >> >> I don't think this will do what you want ;) >> >> Also, I'm pretty sure &> isn't Posix. You'd want something like: >> if type nproc > /dev/null 2>&1 > > Actually, shouldn't we first discuss if we really want to keep wineinstall as part of the > repo before we merge any improvements? I don't know how many people are using it, but it > lacks many important features. It does not install any dependencies, does not warn about > problems during configure, and also does not support a proper wow64 build, ... Basically, > it could be replaced by a one-liner "./configure && make && su root -c 'make install'"... > Do we really want to keep it? I didn't mind keeping it as long as it was just sitting there, but if we are going to start adding complexity or bikeshedding it, then it's probably better to remove it. -- Alexandre Julliard julliard(a)winehq.org

Feb. 26, 2017

Re: Coverity reports for test suite

by Alexandre Julliard

Nikolay Sivov <bunglehead(a)gmail.com> writes: > On 26.02.2017 21:32, Austin English wrote: >> On Sun, Feb 26, 2017 at 12:09 PM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >> FWIW, coverity supports putting issues into different components based >> on filename. It would be trivial to set up two different components >> for wine, e.g., 'wine' and 'wine-tests', then have wine/*/*/tests/* go >> to wine-tests, and wine/* to wine. >> >> For those not familiar with coverity administration, this would not >> require two separate builds, the results are separated when results >> are displayed on the server. >> > > If it's possible to make them appear separately, so tests don't affect > our main reports, I see no harm in scanning tests too. There's probably no harm in getting a scan once, but I'm concerned that it would make people start "fixing" things that don't matter. -- Alexandre Julliard julliard(a)winehq.org

Feb. 26, 2017

Re: [PATCH 2/2] wineinstall - Add support for parallel builds using 'nproc' to detect cpu count

by Sebastian Lackner

On 25.02.2017 01:40, Austin English wrote: > On Fri, Feb 24, 2017 at 6:30 PM, Chris Morgan <chmorgan(a)gmail.com> wrote: >> Parallel builds can greatly reduce the overall build time on modern multi-core processors. >> >> Fall back to two parallel builds in the case where nproc is unavailable, most modern >> processors have at least two cores. >> >> Use the 'NPROC' value defined by the environment if it is not null. >> >> Signed-off-by: Chris Morgan <chmorgan(a)gmail.com> >> --- >> tools/wineinstall | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/tools/wineinstall b/tools/wineinstall >> index bfd62e3..080d590 100755 >> --- a/tools/wineinstall >> +++ b/tools/wineinstall >> @@ -143,10 +143,19 @@ echo "in the meantime..." >> echo >> std_sleep >> >> +# If the NPROC environment variable isn't set use nproc, if available, to determine >> +# the number of processors in the system, if not, fall back to a default NPROC value >> +if type nproc2 &>/dev/null > > I don't think this will do what you want ;) > > Also, I'm pretty sure &> isn't Posix. You'd want something like: > if type nproc > /dev/null 2>&1 > Actually, shouldn't we first discuss if we really want to keep wineinstall as part of the repo before we merge any improvements? I don't know how many people are using it, but it lacks many important features. It does not install any dependencies, does not warn about problems during configure, and also does not support a proper wow64 build, ... Basically, it could be replaced by a one-liner "./configure && make && su root -c 'make install'"... Do we really want to keep it? Best regards, Sebastian

Feb. 26, 2017

Re: [PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

by Fabian Maurer

Thanks for explaining, I'll implement it like that. Regards, Fabian Maurer

Feb. 26, 2017

Re: [PATCH 04/18] comctl32: Extend TaskDialog, add simple callback and tests

by Nikolay Sivov

On 26.02.2017 22:17, Fabian Maurer wrote: >>> For that please use msg.h from comctl32/tests. >> >> I'd love to, but I'm not sure how this would work. >> After all, the message lists for the taskdialogs use a different struct with >> additional values, like the return value it should give, and a sync value. >> I agree that the current way isn't too elegant, how would you go about >> that? > > Thought about it a while longer, but it's not easy. > The current code uses >> while (expected->message && actual->message) > to process the messages. > However, the problem is that TDN_CREATED is zero, terminating the loop way to > early. That's why I added TDN_NO_MORE_MESSAGES in the first place. If I should > use msg.h from comctl32, then I need to rewrite a bunch of its code. > > How do we want to handle it? Right, that's because it was meant for testing actual window messages. We can easily abuse it. You can make up a new message above WM_USER or even WM_APP, like WM_TD_CALLBACK, and use existing 'id' flag like WM_NOTIFY messages are already using. After that you'll only need to set id to TDN_CREATED, and add_message() with resulting structure. This 'id' field does not have any special meaning in generic code in msg.h and is only checked for equality. > > Regards, > Fabian Maurer >

Feb. 26, 2017