[libcxx-commits] [PATCH] D120546: [libcxx] [test] Fix dsl.sh.py to work on Windows.

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:174-177
     actualOut = re.search("# command output:\n(.+)\n$", out, flags=re.DOTALL)
     actualOut = actualOut.group(1) if actualOut else ""
+    if platform.system() == 'Windows':
+      actualOut = actualOut.replace('\r\n', '\n')
----------------
mstorsjo wrote:
> Quuxplusone wrote:
> > I'm mildly confused how the `re.search` on line 174 could ever succeed, if `out` always contains `\r\n` in place of `\n`. Patterns treat `\n` as magic, maybe?
> > If we were trying to deal with `\r\n` in generated files, we'd do, like,
> > ```
> > with open(out_path, 'w', newline='\n') as f:
> >                          ^^^^^^^^^^^^
> > ```
> > I'd like (you) to explore whether we can similarly push the `\r\n` handling all the way down into the library layer here. Should we delegate the `\r\n` handling down into `_executeScriptInternal`? Should we delegate it down into whatever `_executeScriptInternal` calls? Etc. Maybe it ultimately ends up that all we need to do is change `subprocess.call(~~~)` into `subprocess.call(~~~, translateNewlines=True)` or something; that would be awesome.
> Yes, that's a bit strange - it'd clearly be better to move the newline fixup before that at least.
> 
> I tried moving the fix all the way into llvm/utils/lit/lit/TestRunner.py, into `_executeShCmd`, by adding `universal_newlines=True` to the call to `subprocess.Popen()`. That did indeed also solve the issue for this test, but that did instead break a handful of other tests - e.g. https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/ELF/diff2.s, which runs a process which outputs binary data to stdout (where python fails to interpret the non-ascii chars as text).
> 
> So while I guess we should clean up such tests to not do that, I'm not sure if there's any other semi-legitimate case we might be breaking. But we can at least hoist the filtering up to within `_executeScriptInternal`, directly after the call to `lit.TestRunner.executeScriptInternal`.
I posted D120623 as an alternative way of fixing this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120546