[PATCH] D120623: [lit] Read command stdout/stderr as text on Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm

This will probably have the side effect of reducing the double newlines I have seen in lit test output... That could be nice.



================
Comment at: llvm/utils/lit/lit/TestRunner.py:315
         encode = lit.util.to_bytes
         stdout = open(stdout.name, stdout.mode + 'b')
         opened_files.append((None, None, stdout, None))
----------------
The intra-pipeline file descriptors are all *binary*, which is key to making the test suite pass.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:787
+                                          close_fds = kUseCloseFDs,
+                                          universal_newlines = True))
             proc_not_counts.append(not_count)
----------------
docs suggest that `text=True` is the preferred spelling:
https://docs.python.org/3/library/subprocess.html#frequently-used-arguments

My reading of the docs is that only the files opened internally for the STDOUT / STDERR sentinel pipe values are affected by this setting, so it's safe. Anything being read by lit will be printed, and won't be important binary data.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120623/new/

https://reviews.llvm.org/D120623