[libc++] [test] Introduce the `--test-executable` option in ssh.py

[ldionne]

Summary

The new optional command line argument allows to specify the test executable.

Previously we considered a file a test executable if it ended with .tmp.exe, which was a hack.

[ldionne]

Is it not possible to satisfy this convention in the compiler-rt tests?

I am uneasy to make this change because it's not tied to anything visible in libc++ or libc++abi. IOW, from my perspective it would be 100% valid to come back one year from now and clean up the "never used" --test-executable argument, only to see it fail somewhere in compiler-rt.

Is compiler-rt using the libc++ testing format? If not, I believe it should do so officially before we try to support it in these scripts.

[broadwaylamb]

In D83429#2142066, @ldionne wrote:

Is it not possible to satisfy this convention in the compiler-rt tests?

That would require changing a lot of tests. I'm not sure the compiler-rt folks will approve this, given that the use case is quite narrow [for compiler-rt].

I am uneasy to make this change because it's not tied to anything visible in libc++ or libc++abi. IOW, from my perspective it would be 100% valid to come back one year from now and clean up the "never used" --test-executable argument, only to see it fail somewhere in compiler-rt.

Can we remove the implicit inference and just pass this argument in libc++ tests? :)

Is compiler-rt using the libc++ testing format? If not, I believe it should do so officially before we try to support it in these scripts.

It uses the ShTest format. I'm not trying to support the libc++ testing format in compiler-rt (although it would be great; the current configuration logic there is a total mess). Everything works today.

[ldionne]

Could you instead use ssh.py as-is and use another script to chmod +x the executables? I'd really like to avoid adding something ad-hoc like that.

Otherwise, we can productize the libc++ test format properly and make ssh.py more than an implementation detail of it, but that's a lot more work.

[arichardson]

If this flag was also added to run.py, it could also be used to implement wrapper commands (such as running the test under QEMU/GDB/Valgrind) in a much less hacky way. I used a different approach in D88899 and D88903, but if there was a way to identify the test executable, we could implement the wrapper command feature simply by passing --test-executable (instead of --wrapper-command) to ensure code signing signs the test and not the wrapper. The wrapper command is then simply the first argument after the --.
If the --test-executable (or --test-executables if we could have multiple executables?) is not set, it simply defaults to the current behaviour.

[aorlov]

I have created a review https://reviews.llvm.org/D89349 for llvm/utils/remote-exec.py as discussed here.

[ldionne]

Ok, I think this is reasonable, but we should be consistent and do it for run.py as well. And can you please update the various configs to use the right %{exec} substitution?

[broadwaylamb]

Support --test-executable in run.py, update config.py to always pass the new argument.

[ldionne]

libcxx/utils/libcxx/test/config.py

624

What would you say of passing some kind of regex or glob pattern to --test-executable instead? I find it weird that we're passing the path to something that might not exist here.

Instead, if we had an argument like --test-executable-pattern (or similar), the contract could be that "the executor treats any file that matches this pattern as being a test executable". And when there's no such file, it's okay.

This would also accommodate passing several test executables, although I don't think we have a use case for this at the moment.

[ldionne]

[Github PR transition cleanup]

Dropping since there's no update.

[ldionne]

Please open as a github PR is you still want to do this (I think it makes sense but we have to drain our review queue here).

libcxx/utils/libcxx/test/config.py

def configure_substitutions(self):
         sub.append(('%{flags}',         ' '.join(map(pipes.quote, flags))))
         sub.append(('%{compile_flags}', ' '.join(map(pipes.quote, compile_flags))))
         sub.append(('%{link_flags}',    ' '.join(map(pipes.quote, self.cxx.link_flags))))
         sub.append(('%{link_libcxxabi}', pipes.quote(self.cxx.link_libcxxabi_flag)))
         codesign_ident = self.get_lit_conf('llvm_codesign_identity', '')
         env_vars = ' '.join('%s=%s' % (k, pipes.quote(v)) for (k, v) in self.exec_env.items())
         exec_args = [
             '--execdir %T',
+            '--test-executable %t.exe',
             '--codesign_identity "{}"'.format(codesign_ident),
             '--env {}'.format(env_vars)
         ]
         sub.append(('%{exec}', '{} {} -- '.format(self.executor, ' '.join(exec_args))))
         if self.get_lit_conf('libcxx_gdb'):
             sub.append(('%{libcxx_gdb}', self.get_lit_conf('libcxx_gdb')))
 
     def configure_triple(self):

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 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('--test-executable', type=str, required=False, default=None,
+                        help='The name of an executable generated by a test \
+                              file. Specifying it 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.')
     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
 
+    def isTestExe(exe):
+        if not exe or not args.test_executable:
+            return False
+        try:
+            return os.path.samefile(exe, args.test_executable)
+        except FileNotFoundError:
+            return False
+
     # Do any necessary codesigning.
     if args.codesign_identity:
-        exe = commandLine[0]
+        for exe in filter(isTestExe, commandLine):
             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)}
 
     # Run the command line with the given environment in the execution directory.
     return subprocess.call(subprocess.list2cmdline(commandLine), cwd=args.execdir, env=env, shell=True)
 
 
 if __name__ == '__main__':
     exit(main())

libcxx/utils/ssh.py

 
 def main():
     parser = argparse.ArgumentParser()
     parser.add_argument('--host', type=str, required=True)
     parser.add_argument('--execdir', type=str, required=True)
     parser.add_argument('--extra-ssh-args', type=str, required=False)
     parser.add_argument('--extra-scp-args', type=str, required=False)
     parser.add_argument('--codesign_identity', type=str, required=False, default=None)
+    parser.add_argument('--test-executable', type=str, required=False, default=None,
+                        help='The name of an executable generated by a test \
+                              file. Specifying it allows us to do custom \
+                              processing like codesigning test-executables \
+                              and changing their path when running on \
+                              the remote host. It\'s also possible for there \
+                              to be no such executable, for example in \
+                              the case of a .sh.cpp test.')
     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
 
     # Create a temporary directory where the test will be run.
     # That is effectively the value of %T on the remote host.
     tmp = subprocess.check_output(ssh(args, 'mktemp -d /tmp/libcxx.XXXXXXXXXX'), universal_newlines=True).strip()
 
-    # 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
-    # and changing their path when running on the remote host. 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)
+    def isTestExe(exe):
+        if not exe or not args.test_executable:
+            return False
+        try:
+            return os.path.samefile(exe, args.test_executable)
+        except FileNotFoundError:
+            return False
+
     pathOnRemote = lambda file: posixpath.join(tmp, os.path.basename(file))
 
     try:
         # 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={})