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

[release/9.0.2xx] Use the static factory for creating a logger #45009

Draft
wants to merge 2 commits into
base: release/9.0.2xx
Choose a base branch
from

Conversation

MichalPavlik
Copy link
Member

@MichalPavlik MichalPavlik commented Nov 21, 2024

Fixes dotnet/msbuild#10998

Summary

dotnet run directly creates an instance of TerminalLogger, bypassing all checks. This can lead to situations where ANSI escape sequences are emitted to the terminal, even if the terminal does not support them. These escape sequences are also emitted when the standard output is redirected, which can break a CI build that relies on the command's output.

Changes Made

Added a static factory that can return instance of Terminal or Console logger based on current environment. The usage of TerminalLogger in this scenario can be explicitly disabled by using of already existing env. variable, which wasn't possible before.

This SDK update uses this factory to pass the appropriate logger to MSBuild.

Customer Impact

Some customers reported unexpected behavior of dotnet run command and CI builds failures.

Regression?

Yes.

Testing

Manual testing with locally updated SDK.

Notes

This PR depends on dotnet/msbuild#11016. Until the MSBuild PR is merged and inserted, this fix will not work.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-CLI untriaged Request triage from a team member labels Nov 21, 2024
@MichalPavlik MichalPavlik changed the title [9.0.2xx] Use the static factory for creating a logger [release/9.0.2xx] Use the static factory for creating a logger Nov 21, 2024
@JanKrivanek
Copy link
Member

This should not be merged until dotnet/msbuild#11016 is flown in

@MichalPavlik
Copy link
Member Author

This should not be merged until dotnet/msbuild#11016 is flown in

I think some test for dotnet run will fail anyway without the patched MSBuild insertion :)

@MichalPavlik
Copy link
Member Author

As discussed offline, we have time for 9.0.200, so I'll convert this PR to draft and extend the MSBuild part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI DO NOT MERGE untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants