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

Set sync or flush_on_newline for standard I/O on Windows. #9207

Merged
merged 2 commits into from
May 8, 2020

Conversation

kubo
Copy link
Contributor

@kubo kubo commented Apr 30, 2020

This PR sets sync or flush_on_newline for standard I/O on Windows as on Unix.
This uses GetConsoleMode, which succeeds only when the handle is to the console.

I tested it by the code in #9149 (comment). Without this PR, the benchmark results are displayed on process exit. With this PR, they are displayed as on Unix.

I want to hear opinions. Is it better to move win32 and unix code in IO::FileDescriptor.from_stdio to crystal/system/win32/file_descriptor.cr and crystal/system/unix/file_descriptor.cr respectively?

@oprypin
Copy link
Member

oprypin commented Apr 30, 2020

Yes, it is better to move the code as you described.

@kubo kubo force-pushed the standard-handles-on-win32 branch from 151a184 to 2e1a236 Compare May 1, 2020 08:20
@RX14 RX14 added the platform:windows Windows support based on the MSVC toolchain / Win32 API label May 1, 2020
@RX14 RX14 added this to the 0.35.0 milestone May 1, 2020
If starndard I/O is a handle to the console, set `sync` to true.
Otherwise, set `flush_on_newline` to true as described in STDOUT
and STDERR docs.
@kubo kubo force-pushed the standard-handles-on-win32 branch from 2e1a236 to e43943a Compare May 1, 2020 14:56
@kubo
Copy link
Contributor Author

kubo commented May 1, 2020

Add a comment as requested by @straight-shoota here.
Others were not changed.

@oprypin
Copy link
Member

oprypin commented May 1, 2020

This should be merged as separate commits btw.

@kubo
Copy link
Contributor Author

kubo commented May 1, 2020

This should be merged as separate commits btw.

Could you explain more specific? I cannot understand your request.

@oprypin
Copy link
Member

oprypin commented May 1, 2020

Sorry for confusion :(
This is not a request for you.
I'm saying that, because you kept this PR as 2 separate and tidy commits, the person who will be merging this should not automatically squash it into one commit.

@RX14
Copy link
Contributor

RX14 commented May 2, 2020

Sorry, we can't do that any more, @waj decided everything had to be squashed.

@waj
Copy link
Member

waj commented May 2, 2020

Yeah, how lovely...
I just re-enabled merges. Enjoy!

@asterite
Copy link
Member

asterite commented May 2, 2020

Please let's not say X decided something. Many of us agree that squashing is fine because you can always go to the PR to see the commit history.

@oprypin do you think it would be fine to squash this anyway? In my experience whenever I ended up using blame and in a single commit, I navigated to the PR anyway to understand the overall context.

@oprypin
Copy link
Member

oprypin commented May 2, 2020

I would be fine with always squashing, and announcing that that is the case, then people won't have any reason to force-push into the PR branch. Otherwise we're getting the worst of both worlds.

@RX14
Copy link
Contributor

RX14 commented May 2, 2020

I've had an idea for a solution which would make everyone happy for a few years, but unfortunately it requires writing code and starting a new project is hard to justify...

I apologise for being ratty this morning, I think I might need to take a break from Crystal...

@asterite
Copy link
Member

asterite commented May 2, 2020

I think that's another benefit of squashing: PR authors don't need to care about keeping a clean commit history. That should speed up contributions.

@bcardiff bcardiff merged commit 3706389 into crystal-lang:master May 8, 2020
@kubo kubo deleted the standard-handles-on-win32 branch May 8, 2020 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants