[libc++] Add the "lint" scripts to libcxx-generate-files.

[ldionne]

Summary

This way, there's a quick way to run them locally instead of (or before) needing to run all the tests.

[ldionne]

I would like to retain the notion that these are tests, they are not generating anything. How about this:

diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index 1375bf45e7c7..b45a39ce832b 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -138,10 +138,13 @@ if (LIBCXX_INCLUDE_TESTS)
     DEPENDS cxx ${LIBCXX_TEST_DEPS}
     COMMENT "Builds dependencies required to run the test suite.")
 
+  add_lit_target(check-cxx-lint "Lint the header files, CMakeLists.txt, and modulemap."
+                 "${LIBCXX_SOURCE_DIR}/test/libcxx/lint")
+
   add_lit_testsuite(check-cxx
     "Running libcxx tests"
     ${CMAKE_CURRENT_BINARY_DIR}
-    DEPENDS cxx-test-depends)
+    DEPENDS cxx-test-depends check-cxx-lint)
 endif()
 
 if (LIBCXX_GENERATE_COVERAGE)

This will have a couple of downsides, like that it runs all of Lit just for linting (which is slow and verbose). But both of these problems have solutions and we need to fix them anyways (I have a patch that caches Lit configuration locally, let me update that to see what people think).

[• Quuxplusone]

In D117357#3244957, @ldionne wrote:

I would like to retain the notion that these are tests, they are not generating anything. How about this:

IIUC, that doesn't solve @Mordante's problem (which also became my problem today, as I realized that I was introducing a new file and didn't want to be arsed about whether _ sorts before or after a — I just wanted to do a quick ninja some-target and have it tell me the answer).
Spinning up all of check-cxx, or even llvm-lit, not only is wasteful — it adds a whole separate step to the overhead of the commit-my-patch workflow. The workflow right now is "edit the files, then if you added any new files ninja libcxx-generate-files, then git commit the changes." I even remember me conservatively objecting to the introduction of libcxx-generate-files originally — surely people could just run libcxx/utils/*.py by hand! :) But it turns out to be useful. Besides, I personally could remember the path to ../libcxx/util<tabcomplete>/gen<tabcomplete>, so I didn't see a need for libcxx-generate-files originally. But I don't want to have to remember the path to ../libcxx/test/libcxx/lint/<tabcomplete>!

Alternatively, if you don't like adding the lint steps to libcxx-generate-files directly, how would you feel about adding a new cmake target that runs both generate*.py and lint*.py?

[Mordante]

I prefer to also have a cmake target. I agree it feels somewhat odd to have the lint in the generate step. Having the libcxx-lint target not part of libcxx-generate-files would avoid that. I expect to run libcxx-lint more often than libcxx-generate-files.

Once we settle for an approach I would like to see this commit update the developer documentation mentioning the new target.
Adding a CI job should be done in a separate commit, which I wouldn't mind to do.

[ldionne]

If I understand what you are both saying properly, it seems to me that in that case, libcxx-generate-files is not the appropriate name.

What we want is a third target like libcxx-precommit-whatever, which would generate the files and also run the linter, and potentially run clang-tidy in the future, etc. I think what's blocking me right now is that linting is not in any way generating something, so libcxx-generate-files just seems like the wrong name for it. I'm not opposed to the idea of having a quick way to run "quick linting/generating actions" necessary before sending a review -- that does sound useful.

[• Quuxplusone]

In D117357#3258377, @ldionne wrote:

If I understand what you are both saying properly, it seems to me that in that case, libcxx-generate-files is not the appropriate name.

What we want is a third target like libcxx-precommit-whatever, which would generate the files and also run the linter, and potentially run clang-tidy in the future, etc. I think what's blocking me right now is that linting is not in any way generating something, so libcxx-generate-files just seems like the wrong name for it. I'm not opposed to the idea of having a quick way to run "quick linting/generating actions" necessary before sending a review -- that does sound useful.

How about libcxx-tidy? except that then Clang will want a clang-tidy target. ;)
What if we keep the nice short tab-completable name libcxx-lint, and make libcxx-lint also fail if the generated files contain any uncommitted changes (for which of course it'll have to generate them as a side effect)?

[ldionne]

In D117357#3258419, @Quuxplusone wrote:

In D117357#3258377, @ldionne wrote:

If I understand what you are both saying properly, it seems to me that in that case, libcxx-generate-files is not the appropriate name.

What we want is a third target like libcxx-precommit-whatever, which would generate the files and also run the linter, and potentially run clang-tidy in the future, etc. I think what's blocking me right now is that linting is not in any way generating something, so libcxx-generate-files just seems like the wrong name for it. I'm not opposed to the idea of having a quick way to run "quick linting/generating actions" necessary before sending a review -- that does sound useful.

How about libcxx-tidy? except that then Clang will want a clang-tidy target. ;)
What if we keep the nice short tab-completable name libcxx-lint, and make libcxx-lint also fail if the generated files contain any uncommitted changes (for which of course it'll have to generate them as a side effect)?

I like this! It would basically replace the definition for our current check-generated-output CI job inside run-buildbot. It could then run the libcxx-lint target instead, and that would check generated files as well as other things. I think that makes the most sense.

[ldionne]

[Github PR transition cleanup]

Abandoning in favor of doing something more holistic with clang-tidy, lint, etc.. tests.

libcxx/utils/CMakeLists.txt

 add_custom_target(libcxx-generate-feature-test-macros
     COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/generate_feature_test_macro_components.py"
     COMMENT "Generate the <version> header and tests for feature test macros.")
 
 add_custom_target(libcxx-generate-private-header-tests
     COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/generate_private_header_tests.py"
     COMMENT "Generate tests for ensuring that detail headers are private.")
 
+add_custom_target(libcxx-lint
+    COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/test/libcxx/lint/lint_cmakelists.sh.py"
+    COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/test/libcxx/lint/lint_headers.sh.py"
+    COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/test/libcxx/lint/lint_modulemap.sh.py"
+    COMMENT "Lint the header files, CMakeLists.txt, and modulemap.")
+
 add_custom_target(libcxx-generate-files
     DEPENDS libcxx-generate-public-header-transitive-inclusion-tests
             libcxx-generate-public-header-tests
             libcxx-generate-feature-test-macros
             libcxx-generate-private-header-tests
+            libcxx-lint
     COMMENT "Create all the auto-generated files in libc++ and its tests.")