[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 ↗

(On Diff #298359)

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/ssh.py

 import tempfile
 
 
 def main():
     parser = argparse.ArgumentParser()
     parser.add_argument('--host', type=str, required=True)
     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)
     parser.add_argument('--env', type=str, nargs='*', required=False, default=dict())
     (args, remaining) = parser.parse_known_args(sys.argv[1:])
 
     if len(remaining) < 2:
         sys.stderr.write('Missing actual commands to run')
         return 1
 
     commandLine = remaining[1:] # Skip the '--'
 
     ssh = lambda command: ['ssh', '-oBatchMode=yes', args.host, command]
     scp = lambda src, dst: ['scp', '-q', '-oBatchMode=yes', src, '{}:{}'.format(args.host, dst)]
 
     # 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('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):
+        return (exe == args.test_executable or exe.endswith('.tmp.exe')) and os.path.exists(exe)
+
     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={})