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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:787
+                                          close_fds = kUseCloseFDs,
+                                          universal_newlines = True))
             proc_not_counts.append(not_count)
----------------
rnk wrote:
> 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.
Oh, yes, that seems to work nicely!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120623