On Tue, 12 Jan 2016, Hugh McMaster wrote: [...]
diff --git a/programs/regsvr32/tests/Makefile.in b/programs/regsvr32/tests/Makefile.in new file mode 100644 index 0000000..61d25fe --- /dev/null +++ b/programs/regsvr32/tests/Makefile.in
[...]
+ifneq (,$(filter crosstest mingw,$(MAKECMDGOALS) $(findstring mingw,$(CC))))
- TARGET_DLLS = $(addsuffix .dll, $(DLL_BASENAMES))
I believe the syntax on these last two lines is specific to GNU make which is something that Wine has tried to avoid so far.
Note that more generally getting a dll into this test will be pretty tricky. Would it be possible to do the tests using an existing dll?
Alternatively would it work to add the test's dll to dlls/ and grab and include it as a resource in programs/regsvr32/tests? This may help avoid makefile shenanigans.
diff --git a/programs/regsvr32/tests/regsvr32.c b/programs/regsvr32/tests/regsvr32.c new file mode 100644 index 0000000..746c81f --- /dev/null +++ b/programs/regsvr32/tests/regsvr32.c
[...]
+#define run_test(...) run_test_(output, &r, __VA_ARGS__)
A macro with a variable argument list? That does not seem portable (or maybe is a > C99 feature).
+static BOOL __cdecl run_test_(char *output, DWORD *r, const char *fmt, ...) +{
- __ms_va_list va_args;
Shouldn't this be using something like __winetest_va_list? To be checked...
+#define CMP(s) !strcmp(output, s)
This macro is totally unnecessary.
On Tue, 12 Jan 2016 12:58:59 +0100, Francois Gouget wrote:
On Tue, 12 Jan 2016, Hugh McMaster wrote: [...]
diff --git a/programs/regsvr32/tests/Makefile.in b/programs/regsvr32/tests/Makefile.in new file mode 100644 index 0000000..61d25fe --- /dev/null +++ b/programs/regsvr32/tests/Makefile.in
[...]
+ifneq (,$(filter crosstest mingw,$(MAKECMDGOALS) $(findstring mingw,$(CC))))
- TARGET_DLLS = $(addsuffix .dll, $(DLL_BASENAMES))
I believe the syntax on these last two lines is specific to GNU make which is something that Wine has tried to avoid so far.
The GNU make manual doesn't list 'filter', 'findstring' or 'addsuffix' in its comparison with BSD's (p)make, so I'm not sure about those functions being specific or not.
Note that more generally getting a dll into this test will be pretty tricky. Would it be possible to do the tests using an existing dll?
The makefile already creates the required DLL. Eventually, I'll need to use three DLLs, although fortunately I can use the same .c source.
My preference would be to only use mingw32, but I don't think the mingw32 package is a build-dependency for Wine. How likely is it that users building from source will have mingw32-w64 (or equivalent) installed?
I can't use an existing DLL because I need to know exactly what happens in the DLL* functions, so that I can test against that behaviour.
Alternatively would it work to add the test's dll to dlls/ and grab and include it as a resource in programs/regsvr32/tests? This may help avoid makefile shenanigans.
I quite like this idea. I could probably use a subfolder of the regsvr32/tests, although I'd need to work out a better method of building the .dll file, as we obviously only build .so files.
The existing code embeds the test DLLs in a resource file. The difficulty is whether to build two resource files (one for Wine, the other for Windows), or add the resource to .PHONY and have it rebuilt for each target.
diff --git a/programs/regsvr32/tests/regsvr32.c b/programs/regsvr32/tests/regsvr32.c new file mode 100644 index 0000000..746c81f --- /dev/null +++ b/programs/regsvr32/tests/regsvr32.c
[...]
+#define run_test(...) run_test_(output, &r, __VA_ARGS__)
A macro with a variable argument list? That does not seem portable (or maybe is a> C99 feature).
It is a C99 feature. We already use it in comctl32/tests, xaudio2_7/tests, the Mac driver and wine/debug.h.
+static BOOL __cdecl run_test_(char *output, DWORD *r, const char *fmt, ...) +{
- __ms_va_list va_args;
Shouldn't this be using something like __winetest_va_list? To be checked...
I have no idea. __winetest_va_list is not used in the source tree at all.
+#define CMP(s) !strcmp(output, s)
This macro is totally unnecessary.
Yes, you're right. I'll define a new macro re-using the string in the ok() test.
Thanks for the review.
-- Hugh McMaster
On Wed, 13 Jan 2016, Hugh McMaster wrote:
On Tue, 12 Jan 2016 12:58:59 +0100, Francois Gouget wrote:
On Tue, 12 Jan 2016, Hugh McMaster wrote: [...]
diff --git a/programs/regsvr32/tests/Makefile.in b/programs/regsvr32/tests/Makefile.in new file mode 100644 index 0000000..61d25fe --- /dev/null +++ b/programs/regsvr32/tests/Makefile.in
[...]
+ifneq (,$(filter crosstest mingw,$(MAKECMDGOALS) $(findstring mingw,$(CC))))
- TARGET_DLLS = $(addsuffix .dll, $(DLL_BASENAMES))
I believe the syntax on these last two lines is specific to GNU make which is something that Wine has tried to avoid so far.
The GNU make manual doesn't list 'filter', 'findstring' or 'addsuffix' in its comparison with BSD's (p)make, so I'm not sure about those functions being specific or not.
I believe a more common way to do what addsuffix does is:
TARGET_DLLS = $(DLL_BASENAMES:%=%.dll)
But there is also ifneq, it seems the syntax is different between GNU make and BSD make: https://stackoverflow.com/questions/9096018/make-gmake-compatible-if-else-st...
[...]
My preference would be to only use mingw32, but I don't think the mingw32 package is a build-dependency for Wine. How likely is it that users building from source will have mingw32-w64 (or equivalent) installed?
MinGW is not used to compile Wine or the conformance tests. It is only used for cross-compiling the tests for Windows so very few Wine developpers will have it. More importantly Wine should not depend on it.
[...]
The existing code embeds the test DLLs in a resource file. The difficulty is whether to build two resource files (one for Wine, the other for Windows), or add the resource to .PHONY and have it rebuilt for each target.
dlls/kernel32/tests uses the same .res file for both cases but it probably does not apply to your case.
Your case is probably more similar to programs/winetest and there is no cross-building there. Instead to build the Windows executable one has to have a separate tree and cross-build all of it (iirc).
[...]
+#define run_test(...) run_test_(output, &r, __VA_ARGS__)
A macro with a variable argument list? That does not seem portable (or maybe is a> C99 feature).
It is a C99 feature. We already use it in comctl32/tests, xaudio2_7/tests, the Mac driver and wine/debug.h.
I'd say the one trustworthy reference of those is wine/debug.h. Ok, so that's probably acceptable.
However, as far as I can tell its only purpose is to hide which variables are being modified by run_test_(). Furthermore it only works if the function using it has defined local variables with the right names. So all it does is obfuscate the source code making it a net negative. So it should go.
[...]
+#define CMP(s) !strcmp(output, s)
This macro is totally unnecessary.
Yes, you're right. I'll define a new macro re-using the string in the ok() test.
Macros have big drawbacks: they obscure what is really happening, have no type checking, can have side-effects, may execute expressions in their parameters more than once. They should only be used when really needed. The case at hand can simply be done with no macro at all:
ok(strcmp(s, "Register") == 0, "got '%s', expected 'Register'\n", output);
Now if there was really a lot of these string checks, one could argue for a macro of the form:
#define okString(s, e) ok(strcmp((s), (e)) == 0, "got '%s', expected '%s'\n", (s), (e))
Then you'd write: okString(s, "Register");
At least you could then argue that the resulting code really is shorter and that it does not hide which variable is compared to the expected value.
On Wed, 13 Jan 2016 11:18:07 +0100, Francois Gouget wrote:
On Wed, 13 Jan 2016, Hugh McMaster wrote:
On Tue, 12 Jan 2016 12:58:59 +0100, Francois Gouget wrote:
On Tue, 12 Jan 2016, Hugh McMaster wrote: [...]
diff --git a/programs/regsvr32/tests/Makefile.in b/programs/regsvr32/tests/Makefile.in new file mode 100644 index 0000000..61d25fe --- /dev/null +++ b/programs/regsvr32/tests/Makefile.in
[...]
+ifneq (,$(filter crosstest mingw,$(MAKECMDGOALS) $(findstring mingw,$(CC))))
- TARGET_DLLS = $(addsuffix .dll, $(DLL_BASENAMES))
I believe the syntax on these last two lines is specific to GNU make which is something that Wine has tried to avoid so far.
The GNU make manual doesn't list 'filter', 'findstring' or 'addsuffix' in its comparison with BSD's (p)make, so I'm not sure about those functions being specific or not.
I believe a more common way to do what addsuffix does is:
TARGET_DLLS = $(DLL_BASENAMES:%=%.dll)
But there is also ifneq, it seems the syntax is different between GNU make and BSD make: https://stackoverflow.com/questions/9096018/make-gmake-compatible-if-else-st...
Hmm. That's not so good.
[...]
My preference would be to only use mingw32, but I don't think the mingw32 package is a build-dependency for Wine. How likely is it that users building from source will have mingw32-w64 (or equivalent) installed?
MinGW is not used to compile Wine or the conformance tests. It is only used for cross-compiling the tests for Windows so very few Wine developpers will have it. More importantly Wine should not depend on it.
Fair enough.
[...]
The existing code embeds the test DLLs in a resource file. The difficulty is whether to build two resource files (one for Wine, the other for Windows), or add the resource to .PHONY and have it rebuilt for each target.
dlls/kernel32/tests uses the same .res file for both cases but it probably does not apply to your case.
Your case is probably more similar to programs/winetest and there is no cross-building there. Instead to build the Windows executable one has to have a separate tree and cross-build all of it (iirc).
It looks like makedep can be extended to do what I need (or most of what I need).
[...]
+#define run_test(...) run_test_(output, &r, __VA_ARGS__)
A macro with a variable argument list? That does not seem portable (or maybe is a> C99 feature).
It is a C99 feature. We already use it in comctl32/tests, xaudio2_7/tests, the Mac driver and wine/debug.h.
I'd say the one trustworthy reference of those is wine/debug.h. Ok, so that's probably acceptable.
However, as far as I can tell its only purpose is to hide which variables are being modified by run_test_(). Furthermore it only works if the function using it has defined local variables with the right names. So all it does is obfuscate the source code making it a net negative. So it should go.
If you feel that strongly about it, I don't have to use it.
[...]
+#define CMP(s) !strcmp(output, s)
This macro is totally unnecessary.
Yes, you're right. I'll define a new macro re-using the string in the ok() test.
Macros have big drawbacks: they obscure what is really happening, have no type checking, can have side-effects, may execute expressions in their parameters more than once. They should only be used when really needed. The case at hand can simply be done with no macro at all:
ok(strcmp(s, "Register") == 0, "got '%s', expected 'Register'\n", output);Now if there was really a lot of these string checks, one could argue for a macro of the form:
#define okString(s, e) ok(strcmp((s), (e)) == 0, "got '%s', expected '%s'\n", (s), (e))
Then you'd write: okString(s, "Register");
At least you could then argue that the resulting code really is shorter and that it does not hide which variable is compared to the expected value.
There will probably be around 150 tests, so using a macro would definitely be easier.
-- Hugh McMaster
-
Francois Gouget -
Hugh McMaster