[PATCH 0/2] MR6885: cmd: Use the console output codepage to read batch files.
To prevent performance degradation, we will cache the result returned by GetConsoleOutputCP() after executing any external command. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6885
From: Akihiro Sagawa <sagawa.aki(a)gmail.com> --- programs/cmd/tests/batch.c | 26 +++++++++++++++++++++++++- programs/cmd/tests/rsrc.rc | 6 ++++++ programs/cmd/tests/test_utf8.cmd | 4 ++++ programs/cmd/tests/test_utf8.cmd.exp | 2 ++ 4 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 programs/cmd/tests/test_utf8.cmd create mode 100644 programs/cmd/tests/test_utf8.cmd.exp diff --git a/programs/cmd/tests/batch.c b/programs/cmd/tests/batch.c index 6bc5e916ea9..c6016733709 100644 --- a/programs/cmd/tests/batch.c +++ b/programs/cmd/tests/batch.c @@ -500,6 +500,28 @@ void create_nul_test_file(void) CloseHandle(file); } +static void test_utf8_cmd(void) +{ + UINT prev_cp = GetConsoleOutputCP(); + BOOL ret; + + if (!prev_cp) { + /* Detach Wine's shell-no-window kind of console. + See test_Console() in kernel32/tests/process.c for details. */ + FreeConsole(); + AllocConsole(); + prev_cp = GetConsoleOutputCP(); + } + ok(prev_cp, "Can't get the console codepage, err %lu\n", GetLastError()); + + ret = SetConsoleOutputCP(CP_UTF8); + ok(ret, "Can't update the console codepage, err %lu\n", GetLastError()); + + EnumResourceNamesA(NULL, "TESTCMD_UTF8", test_enum_proc, 0); + + SetConsoleOutputCP(prev_cp); +} + START_TEST(batch) { int argc; @@ -529,8 +551,10 @@ START_TEST(batch) argc = winetest_get_mainargs(&argv); if(argc > 2) run_from_file(argv[2]); - else + else { EnumResourceNamesA(NULL, "TESTCMD", test_enum_proc, 0); + test_utf8_cmd(); + } DeleteFileA("nul_test_file"); } diff --git a/programs/cmd/tests/rsrc.rc b/programs/cmd/tests/rsrc.rc index 0336863963f..6626c968a68 100644 --- a/programs/cmd/tests/rsrc.rc +++ b/programs/cmd/tests/rsrc.rc @@ -33,3 +33,9 @@ test_cmdline.cmd TESTCMD "test_cmdline.cmd" /* @makedep: test_cmdline.cmd.exp */ test_cmdline.cmd.exp TESTOUT "test_cmdline.cmd.exp" + +/* @makedep: test_utf8.cmd */ +test_utf8.cmd TESTCMD_UTF8 "test_utf8.cmd" + +/* @makedep: test_utf8.cmd.exp */ +test_utf8.cmd.exp TESTOUT "test_utf8.cmd.exp" diff --git a/programs/cmd/tests/test_utf8.cmd b/programs/cmd/tests/test_utf8.cmd new file mode 100644 index 00000000000..c71e51b5ee6 --- /dev/null +++ b/programs/cmd/tests/test_utf8.cmd @@ -0,0 +1,4 @@ +(a)echo off +echo --- Test 1 +set ALPHA=α +if %ALPHA:~0,1% == α (echo alpha) else echo not alpha diff --git a/programs/cmd/tests/test_utf8.cmd.exp b/programs/cmd/tests/test_utf8.cmd.exp new file mode 100644 index 00000000000..0b767fed5d9 --- /dev/null +++ b/programs/cmd/tests/test_utf8.cmd.exp @@ -0,0 +1,2 @@ +--- Test 1 +(a)todo_wine@alpha -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6885
From: Akihiro Sagawa <sagawa.aki(a)gmail.com> To prevent performance degradation, we will cache the result returned by GetConsoleOutputCP() after executing any external command. --- programs/cmd/batch.c | 20 ++++++++++++++++---- programs/cmd/builtins.c | 1 + programs/cmd/tests/test_utf8.cmd.exp | 2 +- programs/cmd/wcmd.h | 1 + programs/cmd/wcmdmain.c | 6 ++++++ 5 files changed, 25 insertions(+), 5 deletions(-) diff --git a/programs/cmd/batch.c b/programs/cmd/batch.c index 15c01de4b89..89789cd0223 100644 --- a/programs/cmd/batch.c +++ b/programs/cmd/batch.c @@ -24,6 +24,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(cmd); +static UINT file_codepage; /* codepage for reading batch file */ + static RETURN_CODE WCMD_batch_main_loop(void) { RETURN_CODE return_code = NO_ERROR; @@ -200,6 +202,18 @@ WCHAR *WCMD_parameter (WCHAR *s, int n, WCHAR **start, BOOL raw, return WCMD_parameter_with_delims (s, n, start, raw, wholecmdline, L" \t,=;"); } +/**************************************************************************** + * WCMD_update_file_codepage + * + * Update a file codepage used in WCMD_fgets. + * Since GetConsoleOutputCP is slow, we cache the result in advance. + */ +void WCMD_update_file_codepage(void) +{ + UINT cp = GetConsoleOutputCP(); + file_codepage = cp ? cp : CP_OEMCP; +} + /**************************************************************************** * WCMD_fgets * @@ -234,10 +248,8 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) else { LARGE_INTEGER filepos; char *bufA; - UINT cp; const char *p; - cp = GetOEMCP(); bufA = xalloc(noChars); /* Save current file position */ @@ -251,7 +263,7 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) } /* Find first EOL */ - for (p = bufA; p < (bufA + charsRead); p = CharNextExA(cp, p, 0)) { + for (p = bufA; p < (bufA + charsRead); p = CharNextExA(file_codepage, p, 0)) { if (*p == '\n' || *p == '\r') break; } @@ -260,7 +272,7 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h) filepos.QuadPart += p - bufA + 1 + (*p == '\r' ? 1 : 0); SetFilePointerEx(h, filepos, NULL, FILE_BEGIN); - i = MultiByteToWideChar(cp, 0, bufA, p - bufA, buf, noChars); + i = MultiByteToWideChar(file_codepage, 0, bufA, p - bufA, buf, noChars); free(bufA); } diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index d7c5090d17f..d1a646b98a2 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -3393,6 +3393,7 @@ RETURN_CODE WCMD_start(WCHAR *args) WaitForSingleObject( pi.hProcess, INFINITE ); GetExitCodeProcess( pi.hProcess, &exit_code ); errorlevel = (exit_code == STILL_ACTIVE) ? NO_ERROR : exit_code; + WCMD_update_file_codepage(); CloseHandle(pi.hProcess); CloseHandle(pi.hThread); } diff --git a/programs/cmd/tests/test_utf8.cmd.exp b/programs/cmd/tests/test_utf8.cmd.exp index 0b767fed5d9..4ac22345138 100644 --- a/programs/cmd/tests/test_utf8.cmd.exp +++ b/programs/cmd/tests/test_utf8.cmd.exp @@ -1,2 +1,2 @@ --- Test 1 -(a)todo_wine@alpha +alpha diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index caacd44995d..031f08f7088 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -199,6 +199,7 @@ RETURN_CODE WCMD_volume(void); RETURN_CODE WCMD_mklink(WCHAR *args); RETURN_CODE WCMD_change_drive(WCHAR drive); +void WCMD_update_file_codepage (void); WCHAR *WCMD_fgets (WCHAR *buf, DWORD n, HANDLE stream); WCHAR *WCMD_parameter (WCHAR *s, int n, WCHAR **start, BOOL raw, BOOL wholecmdline); WCHAR *WCMD_parameter_with_delims (WCHAR *s, int n, WCHAR **start, BOOL raw, diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 1018fd56624..42c6d2d33ef 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -1373,7 +1373,10 @@ static RETURN_CODE run_full_path(const WCHAR *file, WCHAR *full_cmdline, BOOL ca } if (!interactive || (console && !HIWORD(console))) + { WaitForSingleObject(handle, INFINITE); + WCMD_update_file_codepage(); + } GetExitCodeProcess(handle, &exit_code); errorlevel = (exit_code == STILL_ACTIVE) ? NO_ERROR : exit_code; @@ -3890,6 +3893,9 @@ int __cdecl wmain (int argc, WCHAR *argvW[]) forloopcontext = NULL; WCMD_save_for_loop_context(TRUE); + /* init batch file codepage */ + WCMD_update_file_codepage(); + /* Can't use argc/argv as it will have stripped quotes from parameters * meaning cmd.exe /C echo "quoted string" is impossible */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6885
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149880 Your paranoid android. === build (build log) === error: patch failed: programs/cmd/tests/batch.c:500 error: patch failed: programs/cmd/tests/rsrc.rc:33 error: patch failed: programs/cmd/tests/test_utf8.cmd.exp:1 Task: Patch failed to apply === debian11 (build log) === error: patch failed: programs/cmd/tests/batch.c:500 error: patch failed: programs/cmd/tests/rsrc.rc:33 error: patch failed: programs/cmd/tests/test_utf8.cmd.exp:1 Task: Patch failed to apply === debian11b (build log) === error: patch failed: programs/cmd/tests/batch.c:500 error: patch failed: programs/cmd/tests/rsrc.rc:33 error: patch failed: programs/cmd/tests/test_utf8.cmd.exp:1 Task: Patch failed to apply
I'm puzzled by a couple of items: * use of (Get|Set)ConsoleOutputCP(), while the changes are mostly on input, not output * changing SetConsoleOutputCP() into SetConsoleCP() let the test also pass * I would have preferred toying with the CP inside the .cmd files themselves, but 'chcp 65001' as a command doesn't help here (as I hoped it would; and yet does it set the console cp to utf8) so I feel like we're missing something -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6885#note_88572
On Sun Nov 24 12:41:00 2024 +0000, eric pouech wrote:
I'm puzzled by a couple of items: * use of (Get|Set)ConsoleOutputCP(), while the changes are mostly on input, not output * changing SetConsoleOutputCP() into SetConsoleCP() let the test also pass * I would have preferred toying with the CP inside the .cmd files themselves, but 'chcp 65001' as a command doesn't help here (as I hoped it would; and yet does it set the console cp to utf8) so I feel like we're missing something Thank you for reviewing.
- use of (Get|Set)ConsoleOutputCP(), while the changes are mostly on input, not output
Yes, this is counter intuitive. So why not think of (Get|Set)ConsoleCP() as controlling keyboard input and (Get|Set)ConsoleOutputCP() as controlling the rest?
- changing SetConsoleOutputCP() into SetConsoleCP() let the test also pass
That surprises me. I have tried as well and see some failures --- see https://testbot.winehq.org/JobDetails.pl?Key=149930 .
- I would have preferred toying with the CP inside the .cmd files themselves, but 'chcp 65001' as a command doesn't help here (as I hoped it would; and yet does it set the console cp to utf8)
I was similarly perplexed. Though the original code that showed symptoms called chcp, I prefer to call SetConsoleOuputCP() explicitly to avoid confusion between ConsoleCP and ConsoleOutputCP. Don't you think so? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6885#note_88765
That surprises me. I have tried as well and see some failures --- see https://testbot.winehq.org/JobDetails.pl?Key=149930 .
sorry, looks like I messed up with my tests.
I was similarly perplexed. Though the original code that showed symptoms called chcp, I prefer to call SetConsoleOuputCP() explicitly to avoid confusion between ConsoleCP and ConsoleOutputCP. Don't you think so?
as your original test case comes from using chcp (which sets both cp), and all the testing done shows that cmd.exe relies on output cp, I don't think it matters too much one the previous failure I was seeing comes from the fact that native cmd in cp65001 interprets CR (alone) wrongly and fails parsing (so be sure to always use CRLF \<g\>) I've tried to generate the test files inside the current tests, and made some interesting progress: https://testbot.winehq.org/JobDetails.pl?Key=149935 yes it's very convoluted to say the least, but we need to: * run the test in another cmd instance so that we don't temper current cmd's instance codepage * ensure that the lines that generate the test.cmd (which contain multibyte characters for the 65001 cp) are not wrongly interpreted in current cp even if it's convoluted, I'd rather go that way as we control all bits from withing the tests\_\*.cmd and don't rely on external parameters -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6885#note_88791
participants (4)
-
Akihiro Sagawa -
Akihiro Sagawa (＠sgwaki)
-
eric pouech (＠epo)
-
Marvin