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

Console.Unix: revert SetWindowSize implementation. #100272

Merged
merged 8 commits into from
Aug 14, 2024

Conversation

tmds
Copy link
Member

@tmds tmds commented Mar 26, 2024

SetWindowSize was implemented using using TIOCSWINSZ.
TIOCSWINSZ is meant to inform the kernel of the terminal size.
The window that shows the terminal doesn't change to match that size.

Fixes #96208.

@dotnet/area-system-console ptal.

SetWindowSize was implemented using using TIOCSWINSZ.
TIOCSWINSZ is meant to inform the kernel of the terminal size.
The window that shows the terminal doesn't change to match that size.
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 26, 2024
@tmds
Copy link
Member Author

tmds commented May 10, 2024

@stephentoub do you have additional feedback?

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.

@dotnet/area-system-console, can you please review, in particular for the concept of the change?

@tmds
Copy link
Member Author

tmds commented May 28, 2024

@dotnet/area-system-console is this good to merge?

@stephentoub stephentoub requested a review from adamsitnik June 15, 2024 01:39
@tmds
Copy link
Member Author

tmds commented Jul 8, 2024

@adamsitnik ptal.

@stephentoub stephentoub added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Jul 9, 2024
@stephentoub stephentoub added this to the 9.0.0 milestone Jul 9, 2024
@jeffhandley
Copy link
Member

@jozkee Please review this. We should get this into RC1.

@jeffhandley jeffhandley requested a review from jozkee August 6, 2024 20:47
<Target>P:System.Console.WindowWidth:[T:System.Runtime.Versioning.UnsupportedOSPlatformAttribute]</Target>
<Left>net8.0/mscorlib.dll</Left>
<Right>net9.0/mscorlib.dll</Right>
</Suppression>
Copy link
Member

Choose a reason for hiding this comment

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

How was this generated? I tried as described in #75824 (comment) but didn't get any changes related to Console.
cc @ericstj

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

dotnet.cmd src/libraries/apicompat --no-dependencies /p:ApiCompatGenerateSuppressionFile=true should work, make sure you've built libs before doing that.

I think the comment you linked to had a different property name. The one listed in the error message and shared here should be the right one: ApiCompatGenerateSuppressionFile
https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#apicompatgeneratesuppressionfile

Copy link
Member

Choose a reason for hiding this comment

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

Also these are all CP0014 - they are what I'd expect for the attribute changes to the reference assembly. If you're OK with those changes then these are fine (so long as build passes they are in sync).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking them out. Btw, this is what I get from running dotnet.cmd:

C:\git\runtime>dotnet.cmd src\libraries\apicompat --no-dependencies /p:ApiCompatGenerateSuppressionFile=true
C:\Program Files\dotnet
Could not execute because the specified command or file was not found.
Possible reasons for this include:
  * You misspelled a built-in dotnet command.
  * You intended to execute a .NET program, but dotnet-src\libraries\apicompat does not exist.
  * You intended to run a global tool, but a dotnet-prefixed executable with this name could not be found on the PATH.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Console breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. community-contribution Indicates that the PR has been added by a community member needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unix Console SetWindowSize implementation
5 participants