[libcxx] [test] Fix dsl.sh.py to work on Windows.

[mstorsjo]

Summary

Convert CRLF to LF in the output of tested commands. D120623 fixes the same issue differently.

[• Quuxplusone]

libcxx/test/libcxx/selftest/dsl/dsl.sh.py

252–253

Please remove the comment, and just mention it (including that URL) in the commit message.

libcxx/utils/libcxx/test/dsl.py

178–180

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.

[mstorsjo]

libcxx/test/libcxx/selftest/dsl/dsl.sh.py

252–253

Ok, will do.

libcxx/utils/libcxx/test/dsl.py

178–180

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.

[mstorsjo]

Removed the verbose bug workaround comment from the code, moved the CRLF filtering up to right after the call to lit.TestRunner.executeScriptInternal.

[mstorsjo]

libcxx/utils/libcxx/test/dsl.py

178–180

I posted D120623 as an alternative way of fixing this issue.

[• Quuxplusone]

My understanding is that the forsurethisisnotanexistinglocale fix is pretty much orthogonal to the rest of this patch, and is uncontroversial. Feel free to land that one-liner (with explanatory commit message as discussed above).

I think what you've got here now (replace('\r\n', '\n') in _executeScriptInternal) is better than what you originally proposed. D120623 also looks reasonable to me (but affects tests that libc++ doesn't own, so make sure you've pinged some people relevant to those tests). However, I think both of these are going to need more eyeballs than mine.

[mstorsjo]

Rebased, after committing acf20001a01119ce02d5f2b0282e5a7c966ca12c on its own. (No action needed here right now, awaiting verdict on D120623 first.)

[mstorsjo]

Superseded by D120623.

libcxx/test/libcxx/selftest/dsl/dsl.sh.py

 #===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===----------------------------------------------------------------------===##
 
-# XFAIL: LIBCXX-WINDOWS-FIXME
-
 # Note: We prepend arguments with 'x' to avoid thinking there are too few
 #       arguments in case an argument is an empty string.
 # RUN: %{python} %s x%S x%T x%{substitutions}
 
 import base64
 import copy
 import os
 import pickle
def test_doesnt_explode(self):
         # It's really hard to test that a system has a given locale, so at least
         # make sure we don't explode when we try to check it.
         try:
             dsl.hasAnyLocale(self.config, ['en_US.UTF-8'])
         except subprocess.CalledProcessError:
             self.fail("checking for hasLocale should not explode")
 
     def test_nonexistent_locale(self):
         self.assertFalse(dsl.hasAnyLocale(self.config, ['forsurethisisnotanexistinglocale']))
 
     def test_localization_program_doesnt_compile(self):
         compilerIndex = findIndex(self.config.substitutions, lambda x: x[0] == '%{cxx}')
         self.config.substitutions[compilerIndex] = ('%{cxx}', 'this-is-certainly-not-a-valid-compiler!!')
         self.assertRaises(dsl.ConfigurationCompilationError, lambda: dsl.hasAnyLocale(self.config, ['en_US.UTF-8']))
 
 
 class TestCompilerMacros(SetupConfigs):
     """

libcxx/utils/libcxx/test/dsl.py

litConfig = lit.LitConfig.LitConfig(
     params={})
   _, tmpBase = libcxx.test.format._getTempPaths(test)
   execDir = os.path.dirname(test.getExecPath())
   res = lit.TestRunner.executeScriptInternal(test, litConfig, tmpBase, parsedCommands, execDir)
   if isinstance(res, lit.Test.Result): # Handle failure to parse the Lit test
     res = ('', res.output, 127, None)
   (out, err, exitCode, timeoutInfo) = res
 
+  if platform.system() == 'Windows':
+    out = out.replace('\r\n', '\n')
+    err = err.replace('\r\n', '\n')
+
   # TODO: As a temporary workaround until https://reviews.llvm.org/D81892 lands, manually
   #       split any stderr output that is included in stdout. It shouldn't be there, but
   #       the Lit internal shell conflates stderr and stdout.
   conflatedErrorOutput = re.search("(# command stderr:.+$)", out, flags=re.DOTALL)
   if conflatedErrorOutput:
     conflatedErrorOutput = conflatedErrorOutput.group(0)
     out = out[:-len(conflatedErrorOutput)]
     err += conflatedErrorOutput
with _makeConfigTest(config) as test:
     _, err, exitCode, _ = _executeScriptInternal(test, ['%{build}'])
     if exitCode != 0:
       raise ConfigurationCompilationError("Failed to build program, stderr is:\n{}".format(err))
 
     out, err, exitCode, _ = _executeScriptInternal(test, ["%{{run}} {}".format(' '.join(args))])
     if exitCode != 0:
       raise ConfigurationRuntimeError("Failed to run program, stderr is:\n{}".format(err))
 
     actualOut = re.search("# command output:\n(.+)\n$", out, flags=re.DOTALL)
     actualOut = actualOut.group(1) if actualOut else ""
     return actualOut
 
 @_memoizeExpensiveOperation(lambda c, p, args=None: (c.substitutions, c.environment, p, args))
 def programSucceeds(config, program, args=None):
   """
   Compiles a program for the test target, run it on the test target and return
   whether it completed successfully.
 
   Note that execution of the program is done through the %{exec} substitution,