[libc++][CMake] Adds option to disable clang-tidy.

[mordante]

There have been some reports that always enabling clang-tidy (added in #90077) has some false positives in the detection. It would be good to fix these. This adds an escape hatch to disable the tests, which avoids blocking users with this issue.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

There have been some reports that always enabling clang-tidy (added in #90077) has some false positives in the detection. It would be good to fix these. This adds an escape hatch to disable the tests, which avoids blocking users with this issue.

---

Full diff: https://github.com/llvm/llvm-project/pull/95654.diff

2 Files Affected:

• (modified) libcxx/CMakeLists.txt (+4)
• (modified) libcxx/test/tools/CMakeLists.txt (+4)

diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 4b927017f8c2a..9a35ff30114cd 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -60,6 +60,10 @@ option(LIBCXX_ENABLE_FILESYSTEM
    available on the platform. This includes things like most parts of <filesystem> and
    others like <fstream>" ON)
 option(LIBCXX_INCLUDE_TESTS "Build the libc++ tests." ${LLVM_INCLUDE_TESTS})
+option(LIBCXX_DISABLE_CLANG_TIDY_TESTS
+  "Disables test using the clang-tidy plugin. This option should only be used
+   when autodetection fails, please file a bug report so the autodetection can
+   be improved." OFF)
 set(LIBCXX_SUPPORTED_HARDENING_MODES none fast extensive debug)
 set(LIBCXX_HARDENING_MODE "none" CACHE STRING
   "Specify the default hardening mode to use. This mode will be used inside the
diff --git a/libcxx/test/tools/CMakeLists.txt b/libcxx/test/tools/CMakeLists.txt
index 6d99c53ad46d9..801e45521edb8 100644
--- a/libcxx/test/tools/CMakeLists.txt
+++ b/libcxx/test/tools/CMakeLists.txt
@@ -5,4 +5,8 @@ if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
   message(STATUS "Clang-tidy tests are disabled due to non-clang based compiler.")
   return()
 endif()
+if(LIBCXX_DISABLE_CLANG_TIDY_TESTS)
+  message(STATUS "Clang-tidy tests are disabled by the configuration option LIBCXX_DISABLE_CLANG_TIDY_TESTS.")
+  return()
+endif()
 add_subdirectory(clang_tidy_checks)

[EricWF]

Thanks!

Minor nit: I think LIBCXX_ENABLE_CLANG_TIDY_TESTS would be more idiomatic, but in either case, the option is really just meant as a last resort for when builds break.

[EricWF]

On second thought, This setups allows for clang-tidy tests to be silently turned off if the auto-configuration fails, right?
I don't like when tests can turn themselves off silently and automagically, and I really like the clang-tidy tests.

If this is the case, should this option have three values? ON, FORCE_ON, and OFF? Where the default is ON , and is set to FORCE_ON in the CI?

[ldionne]

@mordante Where have there been some reports of the clang-tidy detection not working properly? I have a preference for simplifying the configuration around this to the maximum.

@EricWF To prevent these mis-configuration issues, I think we could instead enforce the set of Lit features that is defined inside our CI builders. This would catch a mis-configuration of this kind but also many other similar issues, and it wouldn't require adding an option for this in the CMake.

[mordante]

This option is mainly intended as an escape hatch when auto detection fails. I still feel we should fix those issues; however at the moment when somebody encounters the situation they are blocked from testing libc++.

[ldionne]

This option is mainly intended as an escape hatch when auto detection fails. I still feel we should fix those issues; however at the moment when somebody encounters the situation they are blocked from testing libc++.

IMO we should consider such failures as a priority and fix them -- we should aim for people not to disable these checks instead. That's the reason for my pushback on this patch. Also, since I don't know what the reports for clang-tidy not working were, I don't know what we're working around concretely. Do you still feel that this patch is worth pursuing?

[mordante]

I agree that we should abandon this.