[libc-commits] [PATCH] D72353: [libc] Add a convenience CMake rule to add testsuites.

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked an inline comment as done.
sivachandra added inline comments.


================
Comment at: libc/test/src/errno/CMakeLists.txt:1
-add_custom_target(libc_errno_unittests)
-add_dependencies(check_libc libc_errno_unittests)
+add_libc_testsuite(libc_errno_unittests)
 
----------------
abrachet wrote:
> sivachandra wrote:
> > abrachet wrote:
> > > 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.
> > 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 
> 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 at 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
> ```
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
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72353/new/

https://reviews.llvm.org/D72353