[compiler-rt] Better Windows support for running tests in external shell

[broadwaylamb]

Summary

These changes are necessary to support remote running compiler-rt tests that were compiled on Windows.

Most of the code here has been copy-pasted from other lit configs.

[compnerd]

Can you add an explanation for why the conversion to ascii was removed? I don't immediately see why that is needed, not that ascii is a great choice.

[broadwaylamb]

In D83485#2141930, @compnerd wrote:

Can you add an explanation for why the conversion to ascii was removed? I don't immediately see why that is needed, not that ascii is a great choice.

We set the universal_newlines argument to True in Popen instead. This is supported in both Python 2.7 and 3, is easier (no need to do the str(dir.decode('ascii')) dance) and less error prone.

Also, this is necessary because if the config is executed on Windows, and execute_external is True, we take the branch if sys.platform in ['win32'] and execute_external, and if we use Python 3, then the dir variable is a byte-like object, not str, but the replace method on byte-like objects requires its arguments to also be byte-like objects, which is incompatible with Python 2 etc etc.

It is a lot simpler to just work with strings in the first place, which is achieved by setting universal_newlines to True. As far as I understand, this way wasn't taken because of the need to support Python <2.7, but this is not the case now.

[compnerd]

Ah, thanks for the explanation. I'm accepting the change, but would like that explanation also reflected in the commit message. I think that it is very helpful and explains why the change is being made.

[broadwaylamb]

@compnerd thank your for responding so fast!

compiler-rt/test/builtins/Unit/lit.cfg.py

 # -*- Python -*-
 
 import os
 import platform
 
 import lit.formats
 
+# Choose between lit's internal shell pipeline runner and a real shell.  If
+# LIT_USE_INTERNAL_SHELL is in the environment, we use that as an override.
+use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")
+if use_lit_shell:
+    # 0 is external, "" is default, and everything else is internal.
+    execute_external = (use_lit_shell == "0")
+else:
+    # Otherwise we default to internal on Windows and external elsewhere, as
+    # bash on Windows is usually very slow.
+    execute_external = (not sys.platform in ['win32'])
+
 def get_required_attr(config, attr_name):
   attr_value = getattr(config, attr_name, None)
   if attr_value == None:
     lit_config.fatal(
       "No attribute %r in test configuration! You may need to run "
       "tests from your build directory or add this attribute "
       "to lit.site.cfg.py " % attr_name)
   return attr_value
base_lib = os.path.join(config.compiler_rt_libdir, "clang_rt.builtins%s.lib "
                           % config.target_suffix)
   config.substitutions.append( ("%librt ", base_lib) )
 elif config.host_os  == 'Darwin':
   base_lib = os.path.join(config.compiler_rt_libdir, "libclang_rt.osx.a ")
   config.substitutions.append( ("%librt ", base_lib + ' -lSystem ') )
 else:
   base_lib = os.path.join(config.compiler_rt_libdir, "libclang_rt.builtins%s.a"
                           % config.target_suffix)
+  if sys.platform in ['win32'] and execute_external:
+    # Don't pass dosish path separator to msys bash.exe.
+    base_lib = base_lib.replace('\\', '/')
   config.substitutions.append( ("%librt ", base_lib + ' -lc -lm ') )
 
 builtins_source_dir = os.path.join(
   get_required_attr(config, "compiler_rt_src_root"), "lib", "builtins")
+if sys.platform in ['win32'] and execute_external:
+  # Don't pass dosish path separator to msys bash.exe.
+  builtins_source_dir = builtins_source_dir.replace('\\', '/')
 builtins_lit_source_dir = get_required_attr(config, "builtins_lit_source_dir")
 
 extra_link_flags = ["-nodefaultlibs"]
 
 target_cflags = [get_required_attr(config, "target_cflags")]
 target_cflags += ['-fno-builtin', '-I', builtins_source_dir]
 target_cflags += extra_link_flags
 target_cxxflags = config.cxx_mode_flags + target_cflags

compiler-rt/test/crt/lit.cfg.py

else:
     # bash on Windows is usually very slow.
     execute_external = (not sys.platform in ['win32'])
 
 def get_library_path(file):
     cmd = subprocess.Popen([config.clang.strip(),
                             config.target_cflags.strip(),
                             '-print-file-name=%s' % file],
                            stdout=subprocess.PIPE,
-                           env=config.environment)
+                           env=config.environment,
+                           universal_newlines=True)
     if not cmd.stdout:
       lit_config.fatal("Couldn't find the library path for '%s'" % file)
     dir = cmd.stdout.read().strip()
     if sys.platform in ['win32'] and execute_external:
         # Don't pass dosish path separator to msys bash.exe.
         dir = dir.replace('\\', '/')
-    # Ensure the result is an ascii string, across Python2.5+ - Python3.
-    return str(dir.decode('ascii'))
+    return dir
 
 
 def get_libgcc_file_name():
     cmd = subprocess.Popen([config.clang.strip(),
                             config.target_cflags.strip(),
                             '-print-libgcc-file-name'],
                            stdout=subprocess.PIPE,
-                           env=config.environment)
+                           env=config.environment,
+                           universal_newlines=True)
     if not cmd.stdout:
       lit_config.fatal("Couldn't find the library path for '%s'" % file)
     dir = cmd.stdout.read().strip()
     if sys.platform in ['win32'] and execute_external:
         # Don't pass dosish path separator to msys bash.exe.
         dir = dir.replace('\\', '/')
-    # Ensure the result is an ascii string, across Python2.5+ - Python3.
-    return str(dir.decode('ascii'))
+    return dir
 
 
 def build_invocation(compile_flags):
     return ' ' + ' '.join([config.clang] + compile_flags) + ' '
 
 
 # Setup substitutions.
 config.substitutions.append(
     ('%clang ', build_invocation([config.target_cflags])))
 config.substitutions.append(
     ('%clangxx ',
      build_invocation(config.cxx_mode_flags + [config.target_cflags])))
 
 base_lib = os.path.join(
     config.compiler_rt_libdir, "clang_rt.%%s%s.o" % config.target_suffix)
+
+if sys.platform in ['win32'] and execute_external:
+    # Don't pass dosish path separator to msys bash.exe.
+    base_lib = base_lib.replace('\\', '/')
+
 config.substitutions.append(('%crtbegin', base_lib % "crtbegin"))
 config.substitutions.append(('%crtend', base_lib % "crtend"))
 
 config.substitutions.append(
     ('%crt1', get_library_path('crt1.o')))
 config.substitutions.append(
     ('%crti', get_library_path('crti.o')))
 config.substitutions.append(