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

[Reid Kleckner via Phabricator via libcxx-commits]

rnk added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:787
+                                          close_fds = kUseCloseFDs,
+                                          universal_newlines = True))
             proc_not_counts.append(not_count)
----------------
mstorsjo wrote:
> rnk wrote:
> > 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.
> The new option spelling `text` seems to exist since Python 3.7, but our build requirement is Python 3.6, so I’d stick with the older name for now.
Can we pass `errors="replace"` to get something like the previous behavior? I think it's more helpful to show something like ?f?o?o when tests fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120623