Skip to content

Restore Encoding for the .NET SDK upon Exit#30963

Merged
nagilson merged 4 commits intodotnet:release/8.0.1xx-preview2from
nagilson:nagilson-encoding-raii
Mar 8, 2023
Merged

Restore Encoding for the .NET SDK upon Exit#30963
nagilson merged 4 commits intodotnet:release/8.0.1xx-preview2from
nagilson:nagilson-encoding-raii

Conversation

@nagilson
Copy link
Member

@nagilson nagilson commented Mar 2, 2023

Fixes #30170

  • The encoding before the SDK is under execution is saved and then restored upon program exit. This will make it so it doesn't affect the behavior of other programs on the console. This behavior was observed before the encoding fixes for other language support, but now that the encoding is actually being applied correctly, it would occur more often.

  • In addition, we have limited the build number to have the encoding UTF 8 fix only on windows builds that officially support UTF 8. Previous builds of Windows 10 2019 that were earlier, even in 2018, did have the support, but the behavior is technically unsupported and thus 'undefined.'

  • Added environment variable to turn on the and force the encoding even if your OS may not support it officially, or if you don't have read registry access and so we can't tell what build of windows you're on. This is 'breaking' in the sense that previously the encoding would work for those without registry access... maybe we don't want to do that? cc @baronfel for a decision.

  • This is a breaking change in that those who relied on the encoding to be changed by the SDK will no longer be able to rely on this.

@ghost
Copy link

ghost commented Mar 2, 2023

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@baronfel
Copy link
Member

baronfel commented Mar 2, 2023

Added environment variable to turn on the and force the encoding even if your OS may not support it officially, or if you don't have read registry access and so we can't tell what build of windows you're on. This is 'breaking' in the sense that previously the encoding would work for those without registry access... maybe we don't want to do that? cc @baronfel for a decision.

This behavior is fine to me - we just need to make sure that the environment variable is documented.

@nagilson nagilson marked this pull request as ready for review March 2, 2023 21:57
@nagilson nagilson requested a review from a team March 2, 2023 22:25
@nagilson
Copy link
Member Author

nagilson commented Mar 7, 2023

@joeloff Thanks for taking a look, I have responded to all feedback.

@marcpopMSFT marcpopMSFT requested a review from joeloff March 8, 2023 21:35
@nagilson
Copy link
Member Author

nagilson commented Mar 8, 2023

Made a breaking change doc as well. Thanks!

@marcpopMSFT
Copy link
Member

This should have targeted main. Preview 2 is closed and there is no automatic port to main from the preview branches. @nagilson

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