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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.


================
Comment at: libcxx/test/libcxx/selftest/dsl/dsl.sh.py:252-256
+        # The original spelling here, "for_sure_this_is_not_an_existing_locale",
+        # is accidentally detected as existing by the Windows UCRT.
+        # https://developercommunity.visualstudio.com/t/setlocale-succeeds-for-bogus-locale-names-in-older/1652241
+        # This is fixed in UCRT/Windows 10.0.18362.0.
+        self.assertFalse(dsl.hasAnyLocale(self.config, ['forsurethisisnotanexistinglocale']))
----------------
Quuxplusone wrote:
> Please remove the comment, and just mention it (including that URL) in the commit message.
Ok, will do.


================
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')
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120546