Skip to content

Conversation

@GangWang01
Copy link
Member

As part of the fix dotnet/msbuild#12637

To fix dotnet build, it needs to apply the feature flag in dotnet too.

Here is the comparison before/after the fix.
image

Copilot AI review requested due to automatic review settings October 15, 2025 03:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for a feature flag CONSOLE_USE_DEFAULT_ENCODING that allows users to opt out of automatic UTF8 console encoding. This change is part of a fix to address issues with dotnet build and console encoding behavior.

  • Adds environment variable check before setting UTF8 encoding in the main dotnet CLI entry point
  • Adds the same environment variable check in the UI language override setup
  • Provides users control over console encoding behavior through the CONSOLE_USE_DEFAULT_ENCODING flag

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Cli/dotnet/Program.cs Wraps UTF8 console encoding setup with feature flag check
src/Cli/Microsoft.DotNet.Cli.Utils/UILanguageOverride.cs Wraps console encoding logic in Setup method with feature flag check


// Setting output encoding is not available on those platforms
if (UILanguageOverride.OperatingSystemSupportsUtf8())
if (Environment.GetEnvironmentVariable("CONSOLE_USE_DEFAULT_ENCODING") != "1")
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Consider extracting the environment variable name and value into named constants to avoid magic strings and improve maintainability. This would also ensure consistency across the codebase where this same check is performed.

Copilot uses AI. Check for mistakes.

if (
!CultureInfo.CurrentUICulture.TwoLetterISOLanguageName.Equals("en", StringComparison.InvariantCultureIgnoreCase) &&
if (Environment.GetEnvironmentVariable("CONSOLE_USE_DEFAULT_ENCODING") != "1")
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Consider extracting the environment variable name and value into named constants to avoid magic strings and improve maintainability. This duplicates the same magic strings used in Program.cs.

Copilot uses AI. Check for mistakes.
@KalleOlaviNiemitalo
Copy link
Contributor

I don't like the CONSOLE_USE_DEFAULT_ENCODING name because

  • shouldn't it start with DOTNET_
  • it doesn't actually reset the console to the default encoding; instead it preserves the current encoding, which may have been set by an earlier process

@GangWang01
Copy link
Member Author

@baronfel can you please take a look?


// Setting output encoding is not available on those platforms
if (UILanguageOverride.OperatingSystemSupportsUtf8())
if (Environment.GetEnvironmentVariable("CONSOLE_USE_DEFAULT_ENCODING") != "1")
Copy link
Member

Choose a reason for hiding this comment

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

Any env variables that are important to the dotnet CLI need a DOTNET_CLI_ prefix, and then we'd need to 'transform' them to the MSBuild-friendly version before we call out to MSBuild in the MSBuildforwardingApp.cs logic.


if (
!CultureInfo.CurrentUICulture.TwoLetterISOLanguageName.Equals("en", StringComparison.InvariantCultureIgnoreCase) &&
if (Environment.GetEnvironmentVariable("CONSOLE_USE_DEFAULT_ENCODING") != "1")
Copy link
Member

Choose a reason for hiding this comment

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

Similar to my comment on the MSBuild side - this feels wrong. If we know the locale that we're using, and we know that the locale doesn't support UTF8 output for some reason, then we shouldn't do the UTF8 encoding change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants