[PATCH] winetest: Create processes with DETACHED_PROCESS flag.
So that it doesn't automatically create a console window when there is not one already. Fixing a regression from f034084d49b354811096524d472ae5172ac1cebf. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- Same as for the TestLauncher patch, I'm not completely sure what DETACHED_PROCESS does, but as it uses STARTF_USESTDHANDLES too I think the kernelbase create_process_params path taken should mostly be the same, except that it will never force CONSOLE_HANDLE_ALLOC. programs/winetest/main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/programs/winetest/main.c b/programs/winetest/main.c index eb86770afbf..4ff2995714f 100644 --- a/programs/winetest/main.c +++ b/programs/winetest/main.c @@ -619,7 +619,7 @@ run_ex (char *cmd, HANDLE out_file, const char *tempdir, DWORD ms, BOOL nocritic { STARTUPINFOA si; PROCESS_INFORMATION pi; - DWORD wait, status, flags; + DWORD wait, status, flags = DETACHED_PROCESS; UINT old_errmode; /* Flush to disk so we know which test caused Windows to crash if it does */ @@ -635,10 +635,9 @@ run_ex (char *cmd, HANDLE out_file, const char *tempdir, DWORD ms, BOOL nocritic { old_errmode = SetErrorMode(0); SetErrorMode(old_errmode | SEM_FAILCRITICALERRORS); - flags = 0; } else - flags = CREATE_DEFAULT_ERROR_MODE; + flags |= CREATE_DEFAULT_ERROR_MODE; if (!CreateProcessA (NULL, cmd, NULL, NULL, TRUE, flags, NULL, tempdir, &si, &pi)) -- 2.35.1
On 3/30/22 15:37, Eric Pouech wrote:
Le 30/03/2022 à 15:19, Rémi Bernon a écrit :
So that it doesn't automatically create a console window when there is not one already.
all the tests programs should be in GUI subsystem, and shouldn't create a console
for which tests do you see a new console pop up?
A+
I can't reproduce the issue locally, but reverting f034084d49b354811096524d472ae5172ac1cebf fixes the user32 failures for instance, which are explainable with a foreign window stealing focus and covering the test windows. -- Rémi Bernon <rbernon(a)codeweavers.com>
On 3/30/22 15:41, Rémi Bernon wrote:
On 3/30/22 15:37, Eric Pouech wrote:
Le 30/03/2022 à 15:19, Rémi Bernon a écrit :
So that it doesn't automatically create a console window when there is not one already.
all the tests programs should be in GUI subsystem, and shouldn't create a console
for which tests do you see a new console pop up?
A+
I can't reproduce the issue locally, but reverting f034084d49b354811096524d472ae5172ac1cebf fixes the user32 failures for instance, which are explainable with a foreign window stealing focus and covering the test windows.
Also I don't think test programs are using the GUI subsystem, user32_test.exe at least is using CUI subsystem afaics. -- Rémi Bernon <rbernon(a)codeweavers.com>
Fixing a regression from f034084d49b354811096524d472ae5172ac1cebf. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- programs/winetest/main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/programs/winetest/main.c b/programs/winetest/main.c index eb86770afbf..b40b775adb3 100644 --- a/programs/winetest/main.c +++ b/programs/winetest/main.c @@ -640,6 +640,8 @@ run_ex (char *cmd, HANDLE out_file, const char *tempdir, DWORD ms, BOOL nocritic else flags = CREATE_DEFAULT_ERROR_MODE; + /* don't (potentially) create a console when not attached to one */ + if (!GetConsoleOCP()) flags |= DETACHED_PROCESS; if (!CreateProcessA (NULL, cmd, NULL, NULL, TRUE, flags, NULL, tempdir, &si, &pi)) {
Hi, While running your changed tests, 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: https://testbot.winehq.org/JobDetails.pl?Key=111603 Your paranoid android. === debian11 (build log) === /home/winetest/tools/testbot/var/wine-win32/../wine/programs/winetest/main.c:644: undefined reference to `GetConsoleOCP' collect2: error: ld returned 1 exit status Task: The win32 Wine build failed === debian11 (build log) === /home/winetest/tools/testbot/var/wine-wow64/../wine/programs/winetest/main.c:644: undefined reference to `GetConsoleOCP' collect2: error: ld returned 1 exit status Task: The wow64 Wine build failed
Fixing a regression from f034084d49b354811096524d472ae5172ac1cebf. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> v2: only pass DETACH_PROCESS when not attached to a console v3: fix typo --- programs/winetest/main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/programs/winetest/main.c b/programs/winetest/main.c index eb86770afbf..b40b775adb3 100644 --- a/programs/winetest/main.c +++ b/programs/winetest/main.c @@ -640,6 +640,8 @@ run_ex (char *cmd, HANDLE out_file, const char *tempdir, DWORD ms, BOOL nocritic else flags = CREATE_DEFAULT_ERROR_MODE; + /* don't (potentially) create a console when not attached to one */ + if (!GetConsoleOCP()) flags |= DETACHED_PROCESS; if (!CreateProcessA (NULL, cmd, NULL, NULL, TRUE, flags, NULL, tempdir, &si, &pi)) {
Hi, While running your changed tests, 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: https://testbot.winehq.org/JobDetails.pl?Key=111604 Your paranoid android. === debian11 (build log) === /home/winetest/tools/testbot/var/wine-win32/../wine/programs/winetest/main.c:644: undefined reference to `GetConsoleOCP' collect2: error: ld returned 1 exit status Task: The win32 Wine build failed === debian11 (build log) === /home/winetest/tools/testbot/var/wine-wow64/../wine/programs/winetest/main.c:644: undefined reference to `GetConsoleOCP' collect2: error: ld returned 1 exit status Task: The wow64 Wine build failed
Fixing a regression from f034084d49b354811096524d472ae5172ac1cebf. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> v2: only pass DETACH_PROCESS when not attached to a console v3: fix typo v4: actually fixing the typo (sigh) --- programs/winetest/main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/programs/winetest/main.c b/programs/winetest/main.c index eb86770afbf..12c07551285 100644 --- a/programs/winetest/main.c +++ b/programs/winetest/main.c @@ -640,6 +640,8 @@ run_ex (char *cmd, HANDLE out_file, const char *tempdir, DWORD ms, BOOL nocritic else flags = CREATE_DEFAULT_ERROR_MODE; + /* don't (potentially) create a console when not attached to one */ + if (!GetConsoleCP()) flags |= DETACHED_PROCESS; if (!CreateProcessA (NULL, cmd, NULL, NULL, TRUE, flags, NULL, tempdir, &si, &pi)) {
Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> That's great. I can now reproduce these failures. The easiest way to do so from a terminal is something like this: nohup ./wine ... 2>&1 | cat With this winetest patch user32:input succeeds... sometimes. I don't think this means the patch is wrong. Rather the issue is that user32:input itself 'forks' without passing DETACHED_PROCESS to CreateProcess(), causing consoles to appear in the middle of the test which randomly leads to failures. So it looks like we now will have to patch a bunch of tests for similar issues. -- Francois Gouget <fgouget(a)codeweavers.com>
Francois Gouget <fgouget(a)codeweavers.com> writes:
Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
That's great. I can now reproduce these failures. The easiest way to do so from a terminal is something like this:
nohup ./wine ... 2>&1 | cat
With this winetest patch user32:input succeeds... sometimes. I don't think this means the patch is wrong.
Rather the issue is that user32:input itself 'forks' without passing DETACHED_PROCESS to CreateProcess(), causing consoles to appear in the middle of the test which randomly leads to failures.
Do these consoles also appear on Windows then? -- Alexandre Julliard julliard(a)winehq.org
Le 31/03/2022 à 20:16, Alexandre Julliard a écrit :
Francois Gouget<fgouget(a)codeweavers.com> writes:
Signed-off-by: Francois Gouget<fgouget(a)codeweavers.com>
That's great. I can now reproduce these failures. The easiest way to do so from a terminal is something like this:
nohup ./wine ... 2>&1 | cat
With this winetest patch user32:input succeeds... sometimes. I don't think this means the patch is wrong.
Rather the issue is that user32:input itself 'forks' without passing DETACHED_PROCESS to CreateProcess(), causing consoles to appear in the middle of the test which randomly leads to failures. Do these consoles also appear on Windows then?
basically we should have the following hierarchy of processes (for a testbot invocation) testagentd Unix/windows \- TestLauncher (CUI) \- unit test (CUI) \- unit test can recurse upon itself (CUI) according to https://testbot.winehq.org/JobDetails.pl?Key=111732&f201=exe64.report&f301=t... (it's a program to output various information about console (attached / detached, other processes attached to console, user32 information about console, info about std handles...) the info is written using various ways (prefix gives the way: w> using write(1), W> using WriteFile(STD_OUTPUT_HANDLE), d> wine_dbg_output...) a) when the console is created under Windows, the console is created when TestLaunched is spawned. Seems logical: first CUI process, testagentd is likely run detached. in wine env, the console is created at least for the unit test (test as written cannot get information from testlauncher back) as testagentd (on unix) is likely detached (from unix tty), TestLauncher is then created detached from unix but also windows standpoint ; meaning the the w-console is only created for unit test. in both cases, the respawn from within the unit test should reuse the same console b) it then means that the unit test (because it's launched with console creation) does not (currently) honor the std handle inheritance (hence providing different output information) that would explain the discrepancies on output prefix (windows with wW> as those handles are inherited from TestLauncher), and wine (d> using unix fd 1) (under the assumption that in both wine and windows the redirection for running a tests are done in a similar way) we exchanged with Jacek what to do when wine foo is started as initial wine process and foo is a CUI on one hand, it would be a bad idea to let CreateProcess create a console. cases when script is run in a headless context, or detached from any tty... on the other hand (as this case shows), not creating a console is not always the best situation from these considerations, I would recommand that we mimic windows' behavior: when run under wine only, testagend should spawn TestLauncher with wineconsole (we know we can force a user32/wine console creation): ./wine wineconsole TestLauncher... (instead of ./wine TestLauncher) François: what would be the best way to do it? (tweaking platform_unix/platform_run() seems to be more widely used than just running wine? A+
Hi Eric, On 4/1/22 13:55, Eric Pouech wrote:
Le 31/03/2022 à 20:16, Alexandre Julliard a écrit :
Francois Gouget<fgouget(a)codeweavers.com> writes:
Signed-off-by: Francois Gouget<fgouget(a)codeweavers.com>
That's great. I can now reproduce these failures. The easiest way to do so from a terminal is something like this:
nohup ./wine ... 2>&1 | cat
With this winetest patch user32:input succeeds... sometimes. I don't think this means the patch is wrong.
Rather the issue is that user32:input itself 'forks' without passing DETACHED_PROCESS to CreateProcess(), causing consoles to appear in the middle of the test which randomly leads to failures. Do these consoles also appear on Windows then?
basically we should have the following hierarchy of processes (for a testbot invocation)
testagentd Unix/windows \- TestLauncher (CUI) \- unit test (CUI) \- unit test can recurse upon itself (CUI)
according to https://testbot.winehq.org/JobDetails.pl?Key=111732&f201=exe64.report&f301=t...
(it's a program to output various information about console (attached / detached, other processes attached to console, user32 information about console, info about std handles...)
the info is written using various ways (prefix gives the way: w> using write(1), W> using WriteFile(STD_OUTPUT_HANDLE), d> wine_dbg_output...)
a) when the console is created
under Windows, the console is created when TestLaunched is spawned. Seems logical: first CUI process, testagentd is likely run detached.
in wine env, the console is created at least for the unit test (test as written cannot get information from testlauncher back)
as testagentd (on unix) is likely detached (from unix tty), TestLauncher is then created detached from unix but also windows standpoint ; meaning the the w-console is only created for unit test.
in both cases, the respawn from within the unit test should reuse the same console
b) it then means that the unit test (because it's launched with console creation) does not (currently) honor the std handle inheritance (hence providing different output information)
that would explain the discrepancies on output prefix (windows with wW> as those handles are inherited from TestLauncher), and wine (d> using unix fd 1)
(under the assumption that in both wine and windows the redirection for running a tests are done in a similar way)
Nice, that explains the situation.
we exchanged with Jacek what to do when wine foo is started as initial wine process and foo is a CUI
on one hand, it would be a bad idea to let CreateProcess create a console. cases when script is run in a headless context, or detached from any tty...
on the other hand (as this case shows), not creating a console is not always the best situation
Yeah, that solution is not perfect, but it seems to me that there is no solution that could satisfy both cases out of the box.
from these considerations, I would recommand that we mimic windows' behavior: when run under wine only, testagend should spawn TestLauncher with wineconsole (we know we can force a user32/wine console creation):
./wine wineconsole TestLauncher... (instead of ./wine TestLauncher)
François: what would be the best way to do it? (tweaking platform_unix/platform_run() seems to be more widely used than just running wine?
One other solution that we may consider is to build tests as GUI applications and add AttachConsole(ATTACH_PARENT_PROCESS) to main() in test.h. That should preserve desired behaviour. We'd probably need an opt-out mechanism for console tests, but it should be fine otherwise, unless I'm missing something. Thanks, Jacek
from these considerations, I would recommand that we mimic windows' behavior: when run under wine only, testagend should spawn TestLauncher with wineconsole (we know we can force a user32/wine console creation):
./wine wineconsole TestLauncher... (instead of ./wine TestLauncher)
François: what would be the best way to do it? (tweaking platform_unix/platform_run() seems to be more widely used than just running wine?
sigh... running TestLauncher in wineconsole will not work out of the box.. since testagentd currently does 'wine TestLauncher cmd args > foo', so we would need to change also wineconsole to redirect the streams in subprocess
One other solution that we may consider is to build tests as GUI applications and add AttachConsole(ATTACH_PARENT_PROCESS) to main() in test.h. That should preserve desired behaviour. We'd probably need an opt-out mechanism for console tests, but it should be fine otherwise, unless I'm missing something.
maybe... a) we have to check for every test that moving to gui will not break (more) things <g> b) actually I wonder if the console attachment is needed at all (in most of the cases, the std handles would suffice) (console tests apart of course) to demonstrate: simple run on user32/tests:input - yesterday from Rémi: https://testbot.winehq.org/JobDetails.pl?Key=111683 - today, with user32_test.exe regenerated as gui (no other tricks): https://testbot.winehq.org/JobDetails.pl?Key=111744&f101=exe64.report&f201=t... (cannot tell where unique failure under windows comes from, but looks way better, perhaps not perfect) A+
On 4/1/22 15:26, Eric Pouech wrote:
b) actually I wonder if the console attachment is needed at all (in most of the cases, the std handles would suffice) (console tests apart of course)
On Windows, GUI applications never inherit console (we're not compatible with that...), so if you run the test built as GUI application without AttachConsole() directly from console, with no redirection, it will not print anything. Jacek
One other solution that we may consider is to build tests as GUI applications and add AttachConsole(ATTACH_PARENT_PROCESS) to main() in test.h. That should preserve desired behaviour. We'd probably need an opt-out mechanism for console tests, but it should be fine otherwise, unless I'm missing something.
looking at https://bugs.winehq.org/show_bug.cgi?id=52743, I don't think we can force every app out there to move to GUI subsystem maybe the proposed patch (in https://bugs.winehq.org/show_bug.cgi?id=52761) would help A+
On Fri, 1 Apr 2022, Eric Pouech wrote: [...]
Do these consoles also appear on Windows then?
basically we should have the following hierarchy of processes (for a testbot invocation)
testagentd Unix/windows \- TestLauncher (CUI) \- unit test (CUI) \- unit test can recurse upon itself (CUI)
Almost. It's actually: testagentd windows (DETACHED) \- script.bat (cmd.exe -> CUI) -> creates a console \- TestLauncher (CUI) \- unit test (CUI) \- unit test can recurse upon itself (CUI) So TestLauncher is actually started from a batch script: that's just because it's the easiest way to set environment variables like WINETEST_*. That functionality could be moved to TestAgentd if needed.
From that point all processes reuse the script.bat console.
Even if I manually test running user32_test.exe directly from TestAgentd, a console is created when user32_test.exe is started and the test succeeds. But if I modify TestAgentd so it uses DETACHED_PROCESS, then the console is only created when user32_test.exe forks, and then I get 2 failures on Windows: input.c:791: Test failed: 0 (a4/0): 00 from 00 -> 80 unexpected input.c:791: Test failed: 0 (a4/0): 41 from 01 -> 00 unexpected On Unix the process is like this: testagentd (Unix, no terminal) \- shell script (sh Unix, no terminal) \- WineTest.pl (perl Unix, no terminal) \- ./wine TestLauncher (Windows CUI, no console) \- unit test (Windows CUI) -> creates a console \- unit test can recurse upon itself (CUI) The './wine TestLauncher' step is replaced with './wine winetest.exe' for the daily full WineTest suite and for the mailing list patches. My understanding is that as long as the console window is started before the unit test 'main' starts it does not cause trouble. That seems to be how it works on Windows. So the fact we get failures in Wine seems to indicate that the console window is created after the unit test has started, so sorta asynchronously? Should that be considered to be a bug? [...]
a) when the console is created
under Windows, the console is created when TestLaunched is spawned. Seems logical: first CUI process, testagentd is likely run detached.
Modulo the extra cmd process, yes.
in wine env, the console is created at least for the unit test (test as written cannot get information from testlauncher back)
as testagentd (on unix) is likely detached (from unix tty),
On Unix testagentd is started by systemd on boot. So you are right, it does not have a terminal.
TestLauncher is then created detached from unix but also windows standpoint ;
That's because testagentd being a Unix application it does a regular fork()+exec()? Should ./wine create a console for TestLauncher in such a case? Does that mean that ./wine should have an option or environment variable to specify whether that's desirable?
meaning the the w-console is only created for unit test.
in both cases, the respawn from within the unit test should reuse the same console
That has me a bit confused. Doesn't that mean the console window exists before the unit test main() starts running? So why would it make a difference whether that console is created for TestLauncher or for the test unit? [...]
from these considerations, I would recommand that we mimic windows' behavior: when run under wine only, testagend should spawn TestLauncher with wineconsole (we know we can force a user32/wine console creation):
./wine wineconsole TestLauncher... (instead of ./wine TestLauncher)
François: what would be the best way to do it? (tweaking platform_unix/platform_run() seems to be more widely used than just running wine?
That could be done by modifying the wine command that build/WineTest.pl uses. But my understanding from later emails is that this may not be desirable? -- Francois Gouget <fgouget(a)codeweavers.com>
Le 01/04/2022 à 17:27, Francois Gouget a écrit :
On Fri, 1 Apr 2022, Eric Pouech wrote: [...]
Do these consoles also appear on Windows then?
basically we should have the following hierarchy of processes (for a testbot invocation)
testagentd Unix/windows \- TestLauncher (CUI) \- unit test (CUI) \- unit test can recurse upon itself (CUI) Almost. It's actually: testagentd windows (DETACHED) \- script.bat (cmd.exe -> CUI) -> creates a console \- TestLauncher (CUI) \- unit test (CUI) \- unit test can recurse upon itself (CUI)
So TestLauncher is actually started from a batch script: that's just because it's the easiest way to set environment variables like WINETEST_*. That functionality could be moved to TestAgentd if needed. no that's fine, and IMO the way it should be...
From that point all processes reuse the script.bat console.
Even if I manually test running user32_test.exe directly from TestAgentd, a console is created when user32_test.exe is started and the test succeeds.
But if I modify TestAgentd so it uses DETACHED_PROCESS, then the console is only created when user32_test.exe forks, and then I get 2 failures on Windows:
input.c:791: Test failed: 0 (a4/0): 00 from 00 -> 80 unexpected input.c:791: Test failed: 0 (a4/0): 41 from 01 -> 00 unexpected yeah that's logical (testagentd tells it's child not to have a console, so it defers the console creation when a new process is created... hence the errors in input)
On Unix the process is like this:
testagentd (Unix, no terminal) \- shell script (sh Unix, no terminal) \- WineTest.pl (perl Unix, no terminal) \- ./wine TestLauncher (Windows CUI, no console) \- unit test (Windows CUI) -> creates a console \- unit test can recurse upon itself (CUI)
The './wine TestLauncher' step is replaced with './wine winetest.exe' for the daily full WineTest suite and for the mailing list patches.
My understanding is that as long as the console window is started before the unit test 'main' starts it does not cause trouble. That seems to be how it works on Windows.
So the fact we get failures in Wine seems to indicate that the console window is created after the unit test has started, so sorta asynchronously? Should that be considered to be a bug?
see below
TestLauncher is then created detached from unix but also windows standpoint ; That's because testagentd being a Unix application it does a regular fork()+exec()? Should ./wine create a console for TestLauncher in such a case? Does that mean that ./wine should have an option or environment variable to specify whether that's desirable?
yes that's one of the questions ; using wineconsole would help here (even if not fully ok out of the box)
meaning the the w-console is only created for unit test.
in both cases, the respawn from within the unit test should reuse the same console That has me a bit confused. Doesn't that mean the console window exists before the unit test main() starts running? So why would it make a difference whether that console is created for TestLauncher or for the test unit?
[below] we need to distinguish between: - creation of the object console in wineserver (done synchronously at process startup) - the user32 representation of that object (*) ; which is done asynchronously => hence depending on timing of creation of window/focus could impact tests (even if I don't have direct proof of that) and as depicted above, cmd.exe is started on windows before TestLaunch, meaning full window realisation before the bat script is run... and the unit tests while on wine, conhost is started in // of unit tests... so moving console creation to TestLauncher creation could help, even if not 100% bullet proof so the rendering of the console is always done in a (cmd.exe) window on windows ; in wine, it can be that way also, but also done by the unix tty when wine is started from unix command line
[...]
from these considerations, I would recommand that we mimic windows' behavior: when run under wine only, testagend should spawn TestLauncher with wineconsole (we know we can force a user32/wine console creation):
./wine wineconsole TestLauncher... (instead of ./wine TestLauncher)
François: what would be the best way to do it? (tweaking platform_unix/platform_run() seems to be more widely used than just running wine? That could be done by modifying the wine command that build/WineTest.pl uses. But my understanding from later emails is that this may not be desirable?
at this point yes a) using wineconsole doesn't support out of box the redirections needed by TestLauncher b) it can be put on hold for now, until we're sure that moving everything to GUI is doable and acceptable thanks for looking into it
-- Francois Gouget <fgouget(a)codeweavers.com>
participants (8)
-
Alexandre Julliard -
Eric Pouech -
Eric Pouech -
Francois Gouget -
Francois Gouget -
Jacek Caban -
Marvin -
Rémi Bernon