[libcxx] [test] Prepend to PATH instead of overriding it

[mstorsjo]

Summary

On Windows, the PATH env variable is used for locating dynamically
linked librarys, akin to LD_LIBRARY_PATH on Linux.

The tests that run with a dynamically linked libc++ used "--env
PATH=%{lib}" in the test config. This had the unfortunate side effect
of making other tools available in PATH during the runtime of the tests;
in particular, it caused the "executor-has-no-bash" flag to be set for
all those Windows test configs (with the clang-cl static config being
the only one lacking it).

Thus, this increases the number of tests actually included in the
clang-cl dll and all mingw test configs by 9 tests.

The clang-cl static test configuration has been executing those tests
since the "--env PATH=%{lib}" was removed from that test config in
e78223e79efc886ef6f0ea5413deab3737d6d63b. (For mingw we haven't had a
need to split the test config between shared and static, which means
that the mingw static test config previously ran with --env PATH
needlessly.)

This increases the test coverage for patches like D146398 which
can't be executed in the executor-has-no-bash configs.

[mstorsjo]

Ping

[ldionne]

I'm not sure I follow.

This had the unfortunate side effect of making other tools available in PATH during the runtime of the tests

Shouldn't it be the other way around, i.e. shouldn't it make *only* the things in %{lib} available, which is less than if you prepend %{lib} to the rest of the PATH? This leads me to wonder whether we instead might want to acknowledge that this is only used to pass a dyld-library-path-kind-of-variable instead of trying to make it general in purpose. After all, you do use os.pathsep in there so it's pretty clear that this can only be used to set some sort of $PATH-like variable.

[mstorsjo]

In D148324#4300520, @ldionne wrote:

I'm not sure I follow.

This had the unfortunate side effect of making other tools available in PATH during the runtime of the tests

Shouldn't it be the other way around, i.e. shouldn't it make *only* the things in %{lib} available, which is less than if you prepend %{lib} to the rest of the PATH?

Sorry, yes, that sentence lacked a negation. I'll try to rephrase it in a simpler manner.

This leads me to wonder whether we instead might want to acknowledge that this is only used to pass a dyld-library-path-kind-of-variable instead of trying to make it general in purpose. After all, you do use os.pathsep in there so it's pretty clear that this can only be used to set some sort of $PATH-like variable.

So you mean we'd add an option like --library-path to run.py, which then sets it in the platform specific way (i.e. either setting or prepending to an environment variable)? I guess that'd work too (then we should use that in essentially all test configs).

There's one aspect with that setup which is slightly less flexible though; it works great when %{executor} is run.py which executes things locally (so we can ask Python all about the executing system), but when the executor is set to ssh.py, that script doesn't really know whether we should be setting e.g. LD_LIBRARY_PATH or DYLD_LIBRARY_PATH - while the test config knows more about the intended setup (where it currently spells out the expected env variable name).

Remote testing in general is mostly useful if using a static libc++ though, so that combination probably isn't something that must work flawlessly (since it probably can't, in general), but we should at least make an effort to not fall over flat if faced with those arguments.

Even with this version of the patch, I now realized that this patch breaks one of my cross-testing setups; I'll amend it to handle it.

[mstorsjo]

Updated to make ssh.py handle the new option in a best-effort way (not flat out refusing to run when the option is set).

Also use the new option in libcxxabi and libunwind test configs.

[ldionne]

In D148324#4301227, @mstorsjo wrote:

Sorry, yes, that sentence lacked a negation. I'll try to rephrase it in a simpler manner.

Ah, now I think this all makes sense.

This leads me to wonder whether we instead might want to acknowledge that this is only used to pass a dyld-library-path-kind-of-variable instead of trying to make it general in purpose. After all, you do use os.pathsep in there so it's pretty clear that this can only be used to set some sort of $PATH-like variable.

So you mean we'd add an option like --library-path to run.py, which then sets it in the platform specific way (i.e. either setting or prepending to an environment variable)? I guess that'd work too (then we should use that in essentially all test configs).

Yes, that's what I meant.

There's one aspect with that setup which is slightly less flexible though; it works great when %{executor} is run.py which executes things locally (so we can ask Python all about the executing system), but when the executor is set to ssh.py, that script doesn't really know whether we should be setting e.g. LD_LIBRARY_PATH or DYLD_LIBRARY_PATH - while the test config knows more about the intended setup (where it currently spells out the expected env variable name).

Hmm, yeah, I guess that's an annoying limitation. I think your patch as-is is reasonable.

libcxx/test/configs/llvm-libc++-mingw.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using either a DLL or a static library, with MinGW/Clang on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', ''))
 config.substitutions.append(('%{compile_flags}',
     '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib++ -L %{lib} -lc++'
 ))
 config.substitutions.append(('%{exec}',
-    '%{executor} --execdir %T --env PATH=%{lib} -- '
+    '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
     '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
 ))
 config.substitutions.append(('%{exec}',
-    '%{executor} --execdir %T --env PATH=%{lib} -- '
+    '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in

 # This testing configuration handles running the test suite against LLVM's libc++
 # using a DLL, with Clang-cl on Windows. This variant sets _HAS_EXCEPTIONS = 0
 # which removes exception class definitions  from the vcruntime.
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
     '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0'
 ))
 config.substitutions.append(('%{link_flags}',
     '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
 ))
 config.substitutions.append(('%{exec}',
-    '%{executor} --execdir %T --env PATH=%{lib} -- '
+    '%{executor} --execdir %T --prepend_env PATH=%{lib} -- '
 ))
 
 import os, site
 site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
 import libcxx.test.params, libcxx.test.config
 libcxx.test.config.configure(
     libcxx.test.params.DEFAULT_PARAMETERS,
     libcxx.test.features.DEFAULT_FEATURES,
     config,
     lit_config
 )

libcxx/utils/run.py

 import subprocess
 
 
 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('--prepend_env', type=str, nargs='*', required=False, default=dict())
     parser.add_argument("command", nargs=argparse.ONE_OR_MORE)
     args = parser.parse_args()
     commandLine = args.command
 
     # HACK:
     # If an argument is a file that ends in `.tmp.exe`, assume it is the name
     # of an executable generated by a test file. We call these test-executables
     # below. This allows us to do custom processing like codesigning test-executables.
     # It's also possible for there to be no such executable, for example in the case
     # of a .sh.cpp test.
     isTestExe = lambda exe: exe.endswith('.tmp.exe') and os.path.exists(exe)
 
     # Do any necessary codesigning of test-executables found in the command line.
     if args.codesign_identity:
         for exe in filter(isTestExe, commandLine):
             subprocess.check_call(['xcrun', 'codesign', '-f', '-s', args.codesign_identity, exe], env={})
 
     # Extract environment variables into a dictionary
     env = {k : v  for (k, v) in map(lambda s: s.split('=', 1), args.env)}
+
+    # Set environment variables where we prepend the given value to the
+    # existing environment variable.
+    for (k, v) in map(lambda s: s.split('=', 1), args.prepend_env):
+        if k in os.environ:
+            v = v + os.pathsep + os.environ[k]
+        env[k] = v
+
     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())