Skip to content

Console.Unix: fix ANSI colors over redirected standard output. #95654

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 21, 2023
Merged

Console.Unix: fix ANSI colors over redirected standard output. #95654

merged 5 commits into from
Dec 21, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Dec 5, 2023

This fixes a regression that was introduced in #94414.

ANSI colors always need to be sent to standard output.

The Color.RedirectedOutput_EnvVarSet_EmitsAnsiCodes test caught the regression.

@adamsitnik @dotnet/area-system-console ptal.

@ghost ghost added area-System.Console community-contribution Indicates that the PR has been added by a community member labels Dec 5, 2023
@ghost
Copy link

ghost commented Dec 5, 2023

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes a regression that was introduced in #94414.

ANSI colors always need to be sent to standard output.

The Color.RedirectedOutput_EnvVarSet_EmitsAnsiCodes test caught the regression.

@adamsitnik @dotnet/area-system-console ptal.

Author: tmds
Assignees: -
Labels:

area-System.Console
Milestone: -

tmds
tmds commented Dec 5, 2023
View reviewed changes
@tmds
Copy link
Member Author

tmds commented Dec 5, 2023

Beside the updated tests, the test suite is also failing for me on the WindowAndCursorProps.SetWindowSize_GetWindowSize_ReturnsExpected test on this assert:

runtime/src/libraries/System.Console/tests/WindowAndCursorProps.cs

Line 570 in 185a22a

AssertExtensions.Throws<ArgumentOutOfRangeException>("width", () => Console.SetWindowSize(short.MaxValue - Console.WindowLeft, Console.WindowHeight));

The terminal accepts the size instead of giving an error.

@adamsitnik adamsitnik self-assigned this Dec 6, 2023
adamsitnik
adamsitnik reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

The fix LGTM but I am not sure whether we should update the failing tests or restore old parameter names in the argument exceptions.

We can either split it into two PRs or wait for @stephentoub answer.

Thank you for your contribution @tmds !

@adamsitnik adamsitnik added this to the 9.0.0 milestone Dec 6, 2023
@MichalPetryka
Copy link
Contributor

MichalPetryka commented Dec 6, 2023 •
edited
Loading

I initially blamed #79460 for that, then checked the history and realized that it was #79460.

Did you intend to link the same thing twice? (somehow I have no reply box under that comment)

@adamsitnik
Copy link
Member

Did you intend to link the same thing twice? (somehow I have no reply box under that comment)

I did not, I meant #75824. Thanks for pointing this out and apologies for the confusion.

tmds
tmds commented Dec 7, 2023
View reviewed changes
src/libraries/System.Console/tests/WindowAndCursorProps.cs Outdated
@@ -559,7 +559,7 @@ public void SetWindowPosition_Unix_ThrowsPlatformNotSupportedException()
Assert.Throws<PlatformNotSupportedException>(() => Console.SetWindowPosition(50, 50));
}

[PlatformSpecific((TestPlatforms.Windows) | (TestPlatforms.AnyUnix & ~TestPlatforms.Browser & ~TestPlatforms.iOS & ~TestPlatforms.MacCatalyst & ~TestPlatforms.tvOS))]
[PlatformSpecific((TestPlatforms.Windows)]
Copy link
Member Author

@tmds tmds Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated this to be skipped on Unix.

When I run it, I get no error from the kernel when setting these sizes.
And even if there were an error, Console.Unix has no code to turn it into the ArgumentOutOfRangeException this test expects.

On Windows, does calling Console.SetWindowSize resizes the visual window?
If it does, I'm not seeing this with Gnome terminal.
I think TIOCSWINSZ may be meant to inform the kernel about the actual size, instead of requesting a different one.

Copy link
Member Author

@tmds tmds Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These comments are not related to the issue being fixed here.

Unless we know for sure Console.SetWindowSize works as expected on Unix/Linux, we can create a new issue to look into this further.

Copy link
Member Author

Choose a reason for hiding this comment

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

we can create a new issue to look into this further.

I've created #96208 for this.

@tmds tmds force-pushed the fix_redirected_ansi branch from 12b2b69 to 62b1a44 Compare December 7, 2023 11:28
@tmds
Copy link
Member Author

tmds commented Dec 11, 2023

This should be good to merge.

@tmds
Copy link
Member Author

tmds commented Dec 15, 2023

@adamsitnik is this good to merge? And can you share your thoughts on #95654 (comment)?

@tmds
Copy link
Member Author

tmds commented Dec 19, 2023

@adamsitnik or @danmoseley can you merge this?

stephentoub
stephentoub reviewed Dec 19, 2023
View reviewed changes
stephentoub
stephentoub approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the args question, lgtm

@tmds tmds mentioned this pull request Dec 20, 2023
Closed
adamsitnik
adamsitnik approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @tmds!

@adamsitnik adamsitnik merged commit ec2bfd7 into dotnet:main Dec 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@danmoseley danmoseley danmoseley left review comments

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees

@adamsitnik adamsitnik

Labels
area-System.Console community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tmds @MichalPetryka @adamsitnik @stephentoub @danmoseley