Page MenuHomePhabricator

[libc++] Use the internal Lit shell to run the tests
ClosedPublic

Authored by ldionne on Oct 15 2020, 1:47 PM.

Details

Summary

This makes the libc++ tests more portable -- almost all of them should
now work on Windows, except for some tests that assume a shell is
available on the target. We should probably provide a way to exclude
those anyway for the purpose of running tests on embedded targets.

Diff Detail

Event Timeline

ldionne created this revision.Oct 15 2020, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 1:47 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Oct 15 2020, 1:47 PM
arichardson added inline comments.Nov 9 2020, 11:32 AM
libcxx/test/libcxx/selftest/shell-escape.sh.cpp
18 ↗(On Diff #298456)

Is this change required because the internal shell doesn't support !?

It might be useful to add ! support to the integrated shell, as that would allow avoiding some process spawns for the not utility.

ldionne added inline comments.Nov 20 2020, 12:14 PM
libcxx/test/libcxx/selftest/shell-escape.sh.cpp
18 ↗(On Diff #298456)

Yes, that's correct. I implemented not as a shell builtin in https://reviews.llvm.org/D89493.

ldionne updated this revision to Diff 340156.Apr 23 2021, 1:46 PM

Rebase onto main

Awesome!

For some reason, when I've tried running this, in addition to run-error.sh.cpp and run-success.sh.cpp, using the internal shell also broke libcxx/test/libcxx/selftest/additional_compile_flags/substitutes-in-compil
e-flags.sh.cpp in the Windows CI configuration - I haven't figured out why yet.

libcxx/utils/libcxx/test/format.py
241

These changes shouldn't be necessary any longer, ditto below on line 252 and 283

ldionne updated this revision to Diff 340500.Apr 26 2021, 6:15 AM

Rebase onto main and remove unnecessary nots

ldionne updated this revision to Diff 340591.Apr 26 2021, 11:15 AM

Rebase onto main without stray change for adding not to the Lit shell (it has already been added)

Ok, so there's just one test still that fails on Windows in this configuration, libcxx/selftest/additional_compile_flags/substitutes-in-compile-flags.sh.cpp. I can try to have a look into what the issue is what that one.

Ok, so there's just one test still that fails on Windows in this configuration, libcxx/selftest/additional_compile_flags/substitutes-in-compile-flags.sh.cpp. I can try to have a look into what the issue is what that one.

Ok, so the reason here is that substitutes-in-compile-flags.sh.cpp greps for %t in various incarnations. If running with an internal shell, %t is expanded to a path with backslashes, and when that is passed to grep, those backslashes would have to be escaped. I don't think we'd be missing out on much by just adding // UNSUPPORTED: windows on that test, instead of trying to fiddle with backslash-escaping of paths for grep.

With that in place, the tests can be run without bash as executor. However there's a couple other tests that manually invoke the bash executable as part of the test too (which probably also would fail the same for embedded targets; the tests are libcxx/selftest/remote-substitutions.sh.cpp, std/input.output/iostream.objects/narrow.stream.objects/cin.sh.cpp and std/input.output/iostream.objects/wide.stream.objects/wcin.sh.cpp), and a couple dozen more that require generic unix-like tools like cp to exist. (The generic LLVM tests also require such unix tools like cp and grep, but also tries to locate a usable version of such tools automatically, cf https://github.com/llvm/llvm-project/blob/llvmorg-12.0.0/llvm/utils/lit/lit/llvm/config.py#L129-L156.)

That said, we're soon getting the CI images upgraded to always have bash available in PATH, but this patch probably is worthwhile anyway.

Thanks a lot for the analysis @mstorsjo . For the other tests that use bash explicitly, we'll need a way to fix those when running on embedded targets anyway, so punting to that moment sounds acceptable to me.

ldionne updated this revision to Diff 342033.Apr 30 2021, 2:14 PM

Add UNSUPPORTED on Windows

mstorsjo accepted this revision.Apr 30 2021, 11:22 PM

LGTM, thanks!

ldionne accepted this revision as: Restricted Project.May 3 2021, 11:44 AM
This revision is now accepted and ready to land.May 3 2021, 11:44 AM
This revision was automatically updated to reflect the committed changes.