Page MenuHomePhabricator

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

Authored by broadwaylamb on Jul 8 2020, 2:49 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

broadwaylamb created this revision.Jul 8 2020, 2:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2020, 2:49 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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 added a comment.EditedJul 9 2020, 10:13 AM

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.

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.

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 added a subscriber: aorlov.Oct 13 2020, 4:39 PM

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

ldionne requested changes to this revision.Oct 13 2020, 5:28 PM

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?

This revision now requires changes to proceed.Oct 13 2020, 5:28 PM
broadwaylamb edited the summary of this revision. (Show Details)

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

ldionne requested changes to this revision.Oct 15 2020, 8:48 AM
ldionne added inline comments.
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.

This revision now requires changes to proceed.Oct 15 2020, 8:48 AM