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

Flush stdout before calling _setmode on windows #2453

Merged
merged 1 commit into from
May 27, 2024

Conversation

HampusAdolfsson
Copy link
Contributor

@HampusAdolfsson HampusAdolfsson commented May 23, 2024

This fixes a partial regression of #773, where line endings in subprocess output are printed as CR CR LF on windows. The problem can be seen with the following build file:

rule MYRULE
  command = cmd /C echo hello

build foo: MYRULE

Using cat from MinGW:

$ ninja.exe | cat -v
[1/1] cmd /C echo hello^M
hello^M^M

Note the double ^Ms.

I've bisected this to #2085, which enabled stdout buffering on windows. When _setmode is called, it seems that any buffered content also has the new mode applied to it (not the mode that was set when that content was written). Therefore, the buffer should be flushed before calling _setmode. This is described under the "Remarks" section here:

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode

If you write data to a file stream, explicitly flush the code by using fflush before you use _setmode to change the mode. If you do not flush the code, you might get unexpected behavior.

This fixes a regression of ninja-build#773 which was caused by enabling
stdout buffering in ninja-build#2085. Before calling _setmode, the stdout
buffer should be emptied to ensure that the new mode is not
applied to the buffered content. See the 'Remarks' section here:

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode
@jhasse jhasse added this to the 1.13.0 milestone May 23, 2024
@jhasse jhasse added the windows label May 23, 2024
@digit-google
Copy link
Contributor

The fix looks good to me. It'd be nice if we had a regression test for this as well. Sadly it looks like our misc/output_test.py does not run on Windows, so adding something here would not work :-/ Since we have no easy way to test that, this PR is probably OK.

@jhasse jhasse merged commit a58bf70 into ninja-build:master May 27, 2024
11 checks passed
@Mhunter15
Copy link

#2450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants