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

Detect .NET SDK & VSLANG Custom Language Settings & Apply To MSBuild #8503

Merged
merged 33 commits into from May 12, 2023

Conversation

nagilson
Copy link
Member

Fixes #1596

Changes Made

SetConsoleUI now calls into a helper which sets the encoding to support non-en languages and checks if an environment variable exists to change the language to.

Testing

Setting DOTNET_CLI_UI_LANGUAGE=ja now changes msbuild correctly:
image

Doing a complicated build (aka building MSBuild) to use multiple threads shows other threads seem to use the same UI culture:

image

See that chcp remains the same after execution:
image

(Was set to 65001 temporarily but back to the original page before execution.)

Notes

Much of this code is a port of this code: dotnet/sdk#29755
There are some details about the code here.

[!] In addition, it will introduce a breaking change for msbuild just like the SDK.
The break is documented here for the sdk: dotnet/docs#34250

…other programs on the same console.

Note that we could probably do this better. I tried to create a function to do so but because the SetConsoleUI is static this was the least ugly/simple way to do it.
Open to other proposals.
@nagilson nagilson marked this pull request as ready for review February 28, 2023 00:46
@nagilson nagilson marked this pull request as draft February 28, 2023 00:47
@nagilson nagilson marked this pull request as ready for review February 28, 2023 00:49
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Two things:

  1. Can you expand on the breaking change some more, please? Is it only that we now respect VSLANG and will emit UTF-8 when it is specified?
  2. Can you test this in the SDK when opted into MSBuild Server? I'm especially curious about the "do a build/start the server with no DOTNET_CLI_UI_LANGUAGE, then set it, then do another build" case.

src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/MSBuild.UnitTests/XMake_Tests.cs Outdated Show resolved Hide resolved
src/MSBuild.UnitTests/XMake_Tests.cs Show resolved Hide resolved
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
@nagilson
Copy link
Member Author

Can you expand on the breaking change some more, please? Is it only that we now respect VSLANG and will emit UTF-8 when it is specified?

When you use VSLANG or DOTNET_CLI_UI_LANGUAGE it will change the encoding to UTF-8. But that was only officially supported in the Windows 10 (November 2019 update). What happens if you set the environment variable in a version of windows 10 below that is undefined to me. I should change this to only run if Environment.OSVersion's last string is less than the 2019 Nov build number: 18363. I think we would tell people then to set the code page themselves if they are out of date.

If we didn't do that (which is what the SDK is currently doing): probably it will work for some versions, but maybe with some encoding bugs from Windows. Maybe it will fail/reject the encoding in very early versions. Outside of VS at the very least, if you set the environment variable without the encoding it would give you a bunch of ??? or garbled missing-no's.

For VS it is more ... hrmmm. For non-Windows it should not matter, encoding is not an issue. For Win 8 and below setting the environment variables will probably result in garbage outputs until the user manually edits the code page. Thinking about it more, maybe we should try to set the language specific encoding for these languages. But each language has its own quirks with characters output by MSBuild that probably won't work well, and emojis (like the live logger) will be completely broken.

In addition, I need to get to your point in 2) later. But from the testing I did yesterday the chcp change is preserved upon MsBuild exit. If MSBuild is killed and doesn't get to restore the code page, for programs on the same console I believe they will be impacted by the encoding change if they don't manage their own encoding. This could cause various breakages, exemplified by one posted by the customer linked above. In addition, even for modern windows 10 versions, I would expect there to be legacy / custom consoles that for whatever reason don't work well with the UTF-8 encoding.

@nagilson
Copy link
Member Author

nagilson commented Mar 7, 2023

This is waiting for dotnet/sdk#30963 to be merged. Once that's merged, I will ping again for review.

nagilson and others added 6 commits March 8, 2023 15:38
…f8 dont get it by default unless opted in and introduce encoding restorer to prevent msbuild from changing the encoding.
Co-Authored-By: Forgind <12969783+Forgind@users.noreply.github.com>
@nagilson
Copy link
Member Author

nagilson commented Mar 9, 2023

For 1) Now, we only set utf-8 if its officially supported or opted in. And we also restore the encoding. So the breaking change is that if VS doesn't set the encoding for win 8 or below then the characters could become garbled for non-en scenarios. Let's discuss this.

For 2) It doesn't work and that may relate to the VS issue in 1. This is because xmake is skipped in these cases per @Forgind. I've pinged you offline @rainersigwald.

@rainersigwald
Copy link
Member

@rokonec what do you think the best way to change the stdout codepage in the msbuild client is, based primarily on an environment variable? Should it be done in the client init code, or maybe we should add a new ServerChangeCodepage packet type?

@nagilson
Copy link
Member Author

nagilson commented Mar 9, 2023

@rainersigwald This is good to consider, though we also must consider the VS Case not calling into XMake. What about BuildManager's BeginBuild methods?

cc @Forgind

@rainersigwald
Copy link
Member

we also must consider the VS Case not calling into XMake. What about BuildManager's BeginBuild methods?

If you're using the API, the log events should always be standard 16-bit UCS-2 .NET strings and shouldn't care about codepage one way or another.

Does the "output window" in VS have problems with encodings in these cases?

@nagilson
Copy link
Member Author

Does the "output window" in VS have problems with encodings in these cases?

Nope! Tested it myself at least and it didn't.... there were issues but they all came from calling into dotnet first, so seems like it was an SDK thing.

So @rokonec sorry for the delay, please let us know your opinion on the above as it still doesn't work in msbuild server.

@rokonec what do you think the best way to change the stdout codepage in the msbuild client is, based primarily on an environment variable? Should it be done in the client init code, or maybe we should add a new ServerChangeCodepage packet type?

…lready and also fix a test to use en because bild machines are apparently in french sometimes???
@rokonec
Copy link
Contributor

rokonec commented Mar 14, 2023

I would recommend to move whole logic around SetConsoleUI() from Execute to Main before the if (server) , about here


so it will become on both, classic and server path.

That shall cause to properly set CurrentThread.UICulture which is then passed to server in build command, and all should work "fine" since.

However, if it does not work "fine" for server scenarios, please focus on another aspects of this PR and I volunteer to fixing it for server in another consequent PR.

@nagilson
Copy link
Member Author

The encoding needs to be added to logging as well before this is merged.

@nagilson nagilson marked this pull request as draft April 11, 2023 16:51
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 to go

… before a debugger is attached so that build errors wont have a bad encoding
@nagilson
Copy link
Member Author

nagilson commented May 4, 2023

I was able to get it working with logs!
image

A new trade-off I made: I put the code to occur BEFORE debugging so it is literally the first thing to run. If I didn't do that and you got build errors in a different language, they would be junk. Maybe that's acceptable if we want to be obstinate on the principle that NOTHING should run before the debug attachment code.

What it looks like if we move it back and you get a build error building msbuild:
image

What it looks like now:
image

@nagilson nagilson marked this pull request as ready for review May 4, 2023 22:29
@nagilson
Copy link
Member Author

nagilson commented May 4, 2023

When working on this I happened to discover a deadlock (I think) that is unrelated to my changes.

You have to set set MSBUILDDEBUGONSTART=2 and then do artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe MSBuild.Dev.slnf /m  /flp:v=diag

This happens even on main without my changes, then MSBuild will hang forever in a deadlock waiting for the _syncLock. If you press ctrl-c it will say Attempting to cancel the build but no matter how many times you press it or wait youll need to task kill it. The thread window shows all the threads are waiting.

src/Build/Logging/FileLogger.cs Show resolved Hide resolved
src/Build/Logging/FileLogger.cs Outdated Show resolved Hide resolved
src/Framework/EncodingUtilities.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
@rainersigwald rainersigwald added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label May 9, 2023
@nagilson
Copy link
Member Author

nagilson commented May 9, 2023

🥳

src/MSBuild/AutomaticEncodingRestorer.cs Show resolved Hide resolved
src/Framework/EncodingUtilities.cs Outdated Show resolved Hide resolved
src/Framework/EncodingUtilities.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSBuild should respect upstream tool language requests
7 participants