[libcxx-commits] [PATCH] D116958: [libc++] Alphabetize CMakeLists.txt and module.modulemap; enforce in CI.

Mon Jan 10 12:36:48 PST 2022

Quuxplusone planned changes to this revision.
Quuxplusone added a comment.

I'm gonna land the NFC changes to CMakeLists.txt and module.modulemap, just to reduce merge-conflict potential; and then keep this open to explore Louis's test approach.

================
Comment at: libcxx/utils/CMakeLists.txt:31-32
             libcxx-generate-private-header-tests
+            libcxx-lint-cmakelists
+            libcxx-lint-modulemap
     COMMENT "Create all the auto-generated files in libc++ and its tests.")
----------------
ldionne wrote:
> Note that the fact that we'd be running tests as part of a target that generates files is what got me to suggest the alternative approach with a shell test.
The logic of what I did is that I think of this target not as "generating files" per se, but rather "restoring repo invariants (such as 'every detail header has a submodule test') which will plausibly be broken on a regular basis if we're not careful." Anyone who adds a new file will //have// to run `ninja libcxx-generate-files` (or run the equivalent Python scripts manually) to restore the repo invariants. They won't necessarily think or care to run all of the llvm-lit tests in addition.

However, making this a .sh.py test might produce more understandable output on failure. "This test failed" is easier to understand than "some random build step seems to have returned non-zero." So that's what I like about your alternative approach. (It looks like `test/libcxx/selftest/dsl/dsl.sh.py` is the only `.sh.py` test in the repo right now, but that's at least a //non-zero// amount of precedent for adding more.)

The other good thing about your approach is that it moves these scripts into our existing place for "dump random code to test random things" — the test suite — as opposed to bloating `libcxx/utils/` and potentially making it harder to navigate.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116958