Page MenuHomePhabricator

[CMake] Disable libc++ filesystem tests in CrossWinToARMLinux cache file
Changes PlannedPublic

Authored by broadwaylamb on Jan 14 2020, 9:43 AM.

Details

Summary

filesystem tests are not yet supported when running tests on a remote board, because the testing infrastructure isn't quite ready for it yet. Supporting filesystem in the infrastructure is non-trivial, so I suggest disabling these tests for now.

Also, pass through LIT arguments to builtins and runtimes (they were not passed through before). I'm not sure about this change though, as we may want to differentiate between LIT arguments for llvm/clang tests from those for libunwind/libc++abi/libc++ tests. For example, we may want to specify a higher number of threads for the tests that are run remotely, as it significantly speeds up the test suite. Please let me know what you think.

Event Timeline

broadwaylamb created this revision.Jan 14 2020, 9:43 AM

Hi Sergej,

as far as I understood '--param enable_filesystem=False' argument is libc++ specific, but we pass those parameters to all the library tests.
I think it is optimal to pass this parameter directly in the builder configuration only for libcxx as '-DLLVM_LIT_ARGS="--param enable_filesystem=False"'.
Everything else looks good to me.

May be just keep a note in the comments that this lit parameter is required for the remote libcxx tests.

I think it is optimal to pass this parameter directly in the builder configuration only for libcxx as '-DLLVM_LIT_ARGS="--param enable_filesystem=False"'.

You mean, instead of ninja check-cxx just invoke LIT directly and pass this argument?

You mean, instead of ninja check-cxx just invoke LIT directly and pass this argument?

No, a little bit different.
Probably we need something like 'LIBCXX_LLVM_LIT_ARGS` and pass the LIT arguments into the libc++ tests through that kind of parameter (and most likely extend a common LIT_ARGS).
In that case we can pass '--param enable_filesystem=False' using the buildbot configuration files.
Or through this cmake cache file if this is a common case for the arm cross builds on Windows.

It's weird to mix Clang and libc++ test options because they test fundamentally different things. But I don't mind about this patch since it doesn't touch libc++ directly.

broadwaylamb planned changes to this revision.Thu, Jan 30, 2:32 AM

It's weird to mix Clang and libc++ test options because they test fundamentally different things. But I don't mind about this patch since it doesn't touch libc++ directly.

I understand that. This was supposed to be a temporary workaround.

I'm going to go with what @vvereschaka has proposed, although that will probably require some changes in libc++ CMake configuration.