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

Keep the encoding of standard output & error consistent with the console code page #9539

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

GangWang01
Copy link
Member

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1388756/

Context

With OS language English and VS language JPN, building newly created CMake project named プロジェクト got garbage characters in the output.

Changes Made

Make the encoding of process standard output & error consistent with the console code page.

Testing

Added unit tests.

Notes

@ghost
Copy link

ghost commented Dec 14, 2023

Hello! I noticed that you're targeting one of our servicing branches. Please consider updating the version.

@GangWang01 GangWang01 force-pushed the proc-stdout-encoding branch 2 times, most recently from c4fd4ee to 3e29a5a Compare December 14, 2023 10:17
@GangWang01 GangWang01 marked this pull request as ready for review December 15, 2023 01:50
@Forgind
Copy link
Member

Forgind commented Dec 21, 2023

This doesn't seem like a bad change, and I haven't checked whether it actually fixes the bug or not. It looks like I looked at this bug over two years ago and thought the problem may have been because the Message task didn't support changing the code page directly and may have been that the user needed to change how they were using a ToolTask. Assuming this does fix the problem, though this sounds like a positive change, I'd say it suggests the problem should have been fixed by whoever implemented the ToolTask that is ultimately broken; all they would've had to do is set that property as you did here except in their ToolTask extension. In fact, I'd argue that's a better fix, as this one changes fairly fundamental behavior in MSBuild, which always has potential to cause someone issues even if the change looks benign.

Just my 2c 🙂

@JanKrivanek JanKrivanek changed the base branch from vs17.9 to main January 4, 2024 17:14
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Overall looks good

src/Utilities/ToolTask.cs Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Please try to see if we can can move the logic to property initialization.

If not - let's discuss other alternatives

src/Utilities/ToolTask.cs Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good!

@GangWang01 GangWang01 merged commit 1a342f9 into dotnet:main Mar 18, 2024
9 checks passed
@GangWang01 GangWang01 deleted the proc-stdout-encoding branch March 18, 2024 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants