Page MenuHomePhabricator

[libc] Add a convenience CMake rule to add testsuites.
ClosedPublic

Authored by sivachandra on Jan 7 2020, 11:32 AM.

Details

Summary

This rule helps avoid repeated setting of check-libc's dependency on the
various testsuites.

Event Timeline

sivachandra created this revision.Jan 7 2020, 11:32 AM

This patch is fine but my build fails (unrelated to this patch)

make[3]: *** No rule to make target 'projects/libc/src/errno/CMakeFiles/__errno_location_objects.dir/errno_location.cpp.o', needed by 'projects/libc/src/errno/__errno_location_raw.o'.  Stop.

But this happened before applying this patch (and after too). On CMake 3.7 and 3.16. Does it build for you?

libc/test/src/errno/CMakeLists.txt
1

It seems like the rest of LLVM would call this LibcErrnoUnittests, but I don't care too much for the formatting just thought I would bring it up.

sivachandra marked an inline comment as done.Jan 7 2020, 2:23 PM

This patch is fine but my build fails (unrelated to this patch)

make[3]: *** No rule to make target 'projects/libc/src/errno/CMakeFiles/__errno_location_objects.dir/errno_location.cpp.o', needed by 'projects/libc/src/errno/__errno_location_raw.o'.  Stop.

But this happened before applying this patch (and after too). On CMake 3.7 and 3.16. Does it build for you?

I confess I never built with make. I have always tested with Ninja, and it still works. I use CMake 3.13.4.

I tried with make, and indeed it fails in exactly the same fashion. Will dig into this separately. That it works with Ninja seems to indicate a CMake problem.

libc/test/src/errno/CMakeLists.txt
1

I picked this naming style from libcxx. It has targets with names cxx_abi_headers, cxx_external_threads, etc., and test targets like libcxx_test_objects.

TBF, I do not like either of these naming styles. I prefer target names which reflect the source layout. For example, one should not need to list a suites like this at all. Each directory in the test subtree should get its own suite automatically. Its fully qualified name should be something like libc.test.src.errno.all. Individual targets, like the errno_test listed below, will automatically be part of the suite for the directory and have a fully qualified name like libc.test.src.errno.errno_test.

It is possible to implement such a system in CMake, and a project where it was implemented successfully is this: https://github.com/google/pytype

abrachet added inline comments.Jan 7 2020, 6:59 PM
libc/test/src/errno/CMakeLists.txt
1

What do you think about something like I wrote up in D72383? It's pretty awkward for OS/architecture specific tests and doesn't handle it as well as here.

For reference, it changes the names to create these targets:

abrachet@Alexs-MacBook-Pro ~/D/l/build> ninja help | grep "libc\."
libc.test.config.linux.x86_64.all: phony
libc.test.config.linux.x86_64.syscall_test: phony
libc.test.src.errno.all: phony
libc.test.src.errno.errno_test: phony
libc.test.src.string.all: phony
libc.test.src.string.strcat_test: phony
libc.test.src.string.strcpy_test: phony
libc.test.src.sys.mman.all: phony
libc.test.src.sys.mman.mmap_test: phony
sivachandra marked an inline comment as done.Jan 7 2020, 8:32 PM
sivachandra added inline comments.
libc/test/src/errno/CMakeLists.txt
1

I think it captures the overall idea. Few details can be chiseled finer may be.

About the awkwardness, it is probably subjective. But, I tend to go with conventions which are explicit and deducible. For example, to run mmap test, I just do:

$> ninja libc.test.src.sys.mman.mmap_test

Likewise, to run all linux x86_64 tests we have very explicit target and I don't need to refer to the CMake files to see what target we need to run/build:

$> ninja libc.test.config.linux.x86_64.all
abrachet accepted this revision.Jan 7 2020, 9:12 PM
This revision is now accepted and ready to land.Jan 7 2020, 9:12 PM
This revision was automatically updated to reflect the committed changes.
tschuett added a comment.EditedJan 8 2020, 1:29 AM

Unfortunately, this fails on OSX when ${LIBC_TARGET_OS} == "darwin" in libc/test/config/CMakeLists.txt. Windows might have a similar problem. I have no intent building libc. It fails with having libc in LLVM_ENABLE_PROJECTS.

Unfortunately, this fails on OSX when ${LIBC_TARGET_OS} == "darwin" in libc/test/config/CMakeLists.txt. Windows might have a similar problem. I have no intent building libc. It fails with having libc in LLVM_ENABLE_PROJECTS.

Darwin and Windows support is still to be done. We will need a darwin config directory and many other darwin pieces for this to work on darwin.