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

Preserve console encoding when using --utf8output switch without swapping console streams #17761

Merged
merged 22 commits into from
Oct 23, 2024

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Sep 17, 2024

Previously the Console.Out stream was remembered and finally restored in case of using --utf8output switch in order to restore the original console encoding.

With this PR, just the original encoding is preserved and then restored, without swapping the whole Console.Out stream. This cooperates better with redirected console.

Tests added to ensure both fsc and fsi restore encoding correctly.

@majocha majocha requested a review from a team as a code owner September 17, 2024 17:27
Copy link
Contributor

github-actions bot commented Sep 17, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@T-Gro
Copy link
Member

T-Gro commented Sep 17, 2024

IIRC the HostedCompiler was relying on stdout redirection, should only be used for some of the legacy test suites (FsharpQA I think). But FSharpQA still runs here, so that keeps working after your change.

@KevinRansom KevinRansom added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Sep 18, 2024
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

@majocha , @T-Gro --- I'm afraid the savedOut is necessary.

The scenario is this:

  • The host application is using a non-UTF8 Encoding, E.g UTF-7 or UTF-32
  • The host executes the compilation and passes in the '--utf8output' flag
  • This flag changes the Encoding of the Console.Out to UTF which is not what the host app was set to.
  • The purpose of the savedOut is to ensure that these changes caused by our compiler do not adversely impact the hosts.

if tcConfigB.utf8output then

@KevinRansom
Copy link
Member

It also looks as if unused binding analysis didn't find the unused:
let setOutputStreams execute =

I wonder why.

@edgarfgp
Copy link
Contributor

It also looks as if unused binding analysis didn't find the unused: let setOutputStreams execute =

I wonder why.

Might be related to #13849

@majocha
Copy link
Contributor Author

majocha commented Sep 18, 2024

The scenario is this:

  • The host application is using a non-UTF8 Encoding, E.g UTF-7 or UTF-32
  • The host executes the compilation and passes in the '--utf8output' flag
  • This flag changes the Encoding of the Console.Out to UTF which is not what the host app was set to.
  • The purpose of the savedOut is to ensure that these changes caused by our compiler do not adversely impact the hosts.

Thanks @KevinRansom, this is important because it seems in the course of recent changes the test for this got lost.

Also, I'm wondering, maybe just restoring the encoding (if the Out is still the same during dispose) would be less intrusive.

@majocha
Copy link
Contributor Author

majocha commented Sep 18, 2024

I'm frankly confused about this but I suspect that setting OutputEncoding affects the underlying system level stuff and has no effect when console is redirected?

@psfinaki
Copy link
Member

Anything interesting from the git blame? (I can also take a look later in the day)

@KevinRansom
Copy link
Member

@majocha ---
the redirection is irrelevant, since there is a scenario where someone writes a .net app that does work and calls into the compiler directly. For example fantomas, or fsc.exe for that matter, although neither of those apps mess with Encoding.

@majocha
Copy link
Contributor Author

majocha commented Sep 18, 2024

I'll write a test to see what the situation is right now. I understand that from the consumer's pov it's important that fsc writes using the correct encoding according to the given switch and that console encoding is restored afterwards.

@KevinRansom
Copy link
Member

It works, but yes a test case in the repo would be useful to ensure that it doesn't get inadvertently ripped out in the future.

@majocha
Copy link
Contributor Author

majocha commented Sep 20, 2024

I added some tests for this.

@psfinaki
Copy link
Member

Thanks a lot! This week we have some internal stuff going on so our capacities are lower, hopefully we'll get to all the PRs early next week. You're doing amazing job (and inspiring me to do some related job).

@majocha
Copy link
Contributor Author

majocha commented Sep 20, 2024

No worries, I can imagine this week was quite demanding :)

@majocha
Copy link
Contributor Author

majocha commented Sep 20, 2024

I noticed there was code already that restored the Console Encoding in fsi and fsc. I reshufflled it a bit and it seems it works without swapping the whole console streams.

To clarify, my intention is to avoid mutating the Console.Out and Console.Error by fsc , because in my parallel testing experiment there are multithreaded TextWriters installed there so we can capture output from many tests at once.

@T-Gro T-Gro enabled auto-merge (squash) September 30, 2024 10:26
@majocha majocha requested a review from KevinRansom September 30, 2024 13:45
@psfinaki
Copy link
Member

@KevinRansom please rereview.

@majocha majocha changed the title Remove unnecessary Console.SetOut Preserve console encoding when using --utf8output switch without swapping console streams Oct 19, 2024
auto-merge was automatically disabled October 19, 2024 13:53

Head branch was pushed to by a user without write access

@majocha
Copy link
Contributor Author

majocha commented Oct 19, 2024

  • The host application is using a non-UTF8 Encoding, E.g UTF-7 or UTF-32

UTF-32 does not seem to be supported on my machine at all, but I tried UTF7 with
Console.OutputEncoding <- Text.Encoding.GetEncoding(65000) and it gives a runtime exception in dotnet now:
image

This made me think, what if the user somehow does have UTF7 set? Will we be able to restore from UTF8 to UTF7 without issues?

I made a small program to test it

open System
open System.Runtime.InteropServices

[<DllImport("kernel32.dll", SetLastError = true)>]
extern bool SetConsoleOutputCP(uint32 wCodePageID)

[<EntryPoint>]
let main argv =
    if SetConsoleOutputCP(65000u) then
        printfn $"Original output encoding is {Console.OutputEncoding.EncodingName}"
        let encoding = Console.OutputEncoding
        Console.OutputEncoding <- Text.Encoding.UTF8
        printfn $"set to {Console.OutputEncoding}"
        Console.OutputEncoding <- encoding
        printfn $"set back to {Console.OutputEncoding.EncodingName}"
    else
        printfn "Failed to set console output code page."
    0

and thankfully it does work. It seems any code page saved from the console can be restored, even if it cannot be created in dotnet.

Maybe this could be added to the tests as Windows only.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Just need to look at a few things

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

okay, this is still good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants