[libcxx] [test] Pass some windows environment variables through to test processes

[mstorsjo]

Summary

Normally, the run.py wrapper script runs the child processes in a clean environment, with only the environment variables available that are passed via the --env parameter.

However, the COMSPEC and TEMP variables are kind of necessary when running some tests; COMSPEC is necessary for finding the interpreter when executing commands via std::system().

Before f1a96de1bc8db527b5eb820c36c17e275900ca2b, tests were executed via an intermediate shell which implicitly readded the COMSPEC variable.

The TEMP variable allows temp files to be placed in a sensible location; if unset, they're placed in the default temp fallback of C:\Windows instead.

[curdeius]

LGTM. But I believe that the long term solution is to run all the tests in the internal lit shell.

[mstorsjo]

In D97452#2587321, @curdeius wrote:

LGTM. But I believe that the long term solution is to run all the tests in the internal lit shell.

Yes, but I believe that aspect is orthogonal to this script. Currently the lit test driver runs the test script (which consists of two lines, one "clang++" to compile the test and one which invokes the run.py script with python to execute the built test executable) with bash, which maybe could/should be changed to use the lit internal shell. That still keeps the run.py wrapper script for all the purposes it serves right now (codesign, setting up other test specific env variables etc).

[ldionne]

In D97452#2587362, @mstorsjo wrote:

In D97452#2587321, @curdeius wrote:

LGTM. But I believe that the long term solution is to run all the tests in the internal lit shell.

Yes, but I believe that aspect is orthogonal to this script. Currently the lit test driver runs the test script (which consists of two lines, one "clang++" to compile the test and one which invokes the run.py script with python to execute the built test executable) with bash, which maybe could/should be changed to use the lit internal shell. That still keeps the run.py wrapper script for all the purposes it serves right now (codesign, setting up other test specific env variables etc).

Yes, that's correct.

I think it's pretty important to keep run.py and the other executors free of logic. Instead, we could string those environment variables into self.exec_env in config.py. They would then be passed to the executor using the --env argument, which was meant for that. WDYT?

[mstorsjo]

In D97452#2597696, @ldionne wrote:

In D97452#2587362, @mstorsjo wrote:

In D97452#2587321, @curdeius wrote:

LGTM. But I believe that the long term solution is to run all the tests in the internal lit shell.

Yes, but I believe that aspect is orthogonal to this script. Currently the lit test driver runs the test script (which consists of two lines, one "clang++" to compile the test and one which invokes the run.py script with python to execute the built test executable) with bash, which maybe could/should be changed to use the lit internal shell. That still keeps the run.py wrapper script for all the purposes it serves right now (codesign, setting up other test specific env variables etc).

Yes, that's correct.

I think it's pretty important to keep run.py and the other executors free of logic. Instead, we could string those environment variables into self.exec_env in config.py. They would then be passed to the executor using the --env argument, which was meant for that. WDYT?

Sure - maybe.

However, I see it as a bit of different concern about passing specific options requested from config.py, vs allowing some vars through transparently.

Consider cross testing (I do that, but that's further away in the upstreaming pipeline) - I don't want to pick up env vars blindly from the (unix) build env and pass them to the remote runner (windows), I just want the logic that creates a clean env to let some specifics through.

Those setups use a variant of the existing ssh.py runner (which don't clear the env, only add vars to it), while run.py (for local execution) is the one that entirely wipe the env before adding the env vars that are requested.

[ldionne]

In D97452#2597780, @mstorsjo wrote:

[...]

Consider cross testing (I do that, but that's further away in the upstreaming pipeline) - I don't want to pick up env vars blindly from the (unix) build env and pass them to the remote runner (windows), I just want the logic that creates a clean env to let some specifics through.

Those setups use a variant of the existing ssh.py runner (which don't clear the env, only add vars to it), while run.py (for local execution) is the one that entirely wipe the env before adding the env vars that are requested.

I'm convinced. Indeed, it doesn't make any sense to forward env vars from the build host to the run host.

libcxx/utils/run.py

 #!/usr/bin/env python
 #===----------------------------------------------------------------------===##
 #
 # 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
 #
 #===----------------------------------------------------------------------===##
 
 """run.py is a utility for running a program.
 
 It can perform code signing, forward arguments to the program, and return the
 program's error code.
 """
 
 import argparse
+import os
+import platform
 import subprocess
 import sys
 
 
 def main():
     parser = argparse.ArgumentParser()
     parser.add_argument('--execdir', type=str, required=True)
     parser.add_argument('--codesign_identity', type=str, required=False, default=None)
     parser.add_argument('--env', type=str, nargs='*', required=False, default=dict())
     parser.add_argument("command", nargs=argparse.ONE_OR_MORE)
     args = parser.parse_args()
     commandLine = args.command
 
     # Do any necessary codesigning.
     if args.codesign_identity:
         exe = commandLine[0]
         rc = subprocess.call(['xcrun', 'codesign', '-f', '-s', args.codesign_identity, exe], env={})
         if rc != 0:
             sys.stderr.write('Failed to codesign: ' + exe)
             return rc
 
     # Extract environment variables into a dictionary
     env = {k : v  for (k, v) in map(lambda s: s.split('=', 1), args.env)}
+    if platform.system() == 'Windows':
+        # Pass some extra variables through on Windows:
+        # COMSPEC is needed for running subprocesses via std::system().
+        if 'COMSPEC' in os.environ:
+            env['COMSPEC'] = os.environ.get('COMSPEC')
+        # TEMP is needed for placing temp files in a sensible directory.
+        if 'TEMP' in os.environ:
+            env['TEMP'] = os.environ.get('TEMP')
 
     # Run the command line with the given environment in the execution directory.
     return subprocess.call(commandLine, cwd=args.execdir, env=env, shell=False)
 
 
 if __name__ == '__main__':
     exit(main())