Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In Windows host, use WriteConsoleW for stdout and stderr, and locale enabled print for file output #102295

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented May 16, 2024

Fixes #102174

Per #102174 (comment), this uses WriteConsoleW for writing to stdout and stderr, and the locale enabled _vfwprintf_l for writing to a file.

Copy link
Contributor

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

@jtschuster jtschuster changed the title Pass locale with output printing method in Windows host In Windows host, use WriteConsoleW for stdout and stderr, and locale enabled print for file output May 16, 2024
@jtschuster jtschuster marked this pull request as ready for review May 16, 2024 16:08
@jtschuster jtschuster marked this pull request as draft May 16, 2024 18:30
src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.h Outdated Show resolved Hide resolved
- Move print functions to pal.windows.cpp
- Rename print functions
- Use va_copy on the args when passed to two functions
- Add comment for why the methods are more complex
@jtschuster
Copy link
Member Author

Looks like the tests are failing because WriteConsole fails when stdout or stderr is redirected, so we need to write to a file in that case. Working on the fix.

@jtschuster jtschuster marked this pull request as ready for review May 24, 2024 17:49
@jtschuster
Copy link
Member Author

There's a number of other string related functions in the pal that have locale variants, is there any reason not to update those to use UTF-8 everywhere in a follow-up?

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a simple test we could add for this?
Maybe something similar to the below, but using DotNetBuilder to set up the install in a directory with characters that don't work properly without the change in this PR:

src/native/corehost/hostmisc/pal.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.windows.cpp Outdated Show resolved Hide resolved
src/native/corehost/hostmisc/pal.windows.cpp Outdated Show resolved Hide resolved
@elinor-fung
Copy link
Member

There's a number of other string related functions in the pal that have locale variants, is there any reason not to update those to use UTF-8 everywhere in a follow-up?

That may depend on the specific function. For example, for things like strcmp, we do just want the ordinal comparison. If we don't need it, I'd rather not have to create/free the locale (for the printing to file/console here, I'm not concerned, since it is not 'normal' app startup - just either explicitly printing info to the terminal or verbose logs enabled).

@elinor-fung
Copy link
Member

I believe this PR will also fix dotnet/sdk#17451

- move helper to anonymous namespace
- add test for utf-8/GB18030 characters
- add comments for why we need to use a locale
Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/native/corehost/hostmisc/pal.windows.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GB1030 dotnet info
2 participants