Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][CMake] Removes LIBCXX_ENABLE_CLANG_TIDY. #85794

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

mordante
Copy link
Member

The clang-tidy selection in CMake was refactored in #81362. During review it was suggested to remove this CMake option.

@mordante mordante force-pushed the review/removes_clang_tidy_cmake_option branch from bf7a4df to 6592086 Compare March 20, 2024 14:51
@mordante
Copy link
Member Author

@jplehr Can you test whether this patch works correctly on the clang-hip-vega build bots?

@mordante
Copy link
Member Author

@jplehr friendly ping.

1 similar comment
@mordante
Copy link
Member Author

mordante commented Apr 7, 2024

@jplehr friendly ping.

The clang-tidy selection in CMake was refactored in
llvm#81362. During review it was
suggested to remove this CMake option.
@mordante mordante force-pushed the review/removes_clang_tidy_cmake_option branch from 6592086 to 2474f6c Compare April 12, 2024 17:51
@mordante mordante marked this pull request as ready for review April 12, 2024 17:51
@mordante mordante requested a review from a team as a code owner April 12, 2024 17:51
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 12, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 12, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

The clang-tidy selection in CMake was refactored in #81362. During review it was suggested to remove this CMake option.


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

6 Files Affected:

  • (modified) libcxx/CMakeLists.txt (-5)
  • (modified) libcxx/docs/ReleaseNotes/19.rst (+3)
  • (modified) libcxx/test/tools/CMakeLists.txt (+4-8)
  • (modified) libcxx/test/tools/clang_tidy_checks/CMakeLists.txt (+41-15)
  • (modified) libcxx/utils/ci/buildkite-pipeline.yml (-1)
  • (modified) libcxx/utils/ci/run-buildbot (-8)
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index e565c47c76687a..043d5a8295c1a6 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -123,7 +123,6 @@ option(LIBCXX_ENABLE_VENDOR_AVAILABILITY_ANNOTATIONS
    to provide compile-time errors when using features unavailable on some version of
    the shared library they shipped should turn this on and see `include/__availability`
    for more details." OFF)
-option(LIBCXX_ENABLE_CLANG_TIDY "Whether to compile and run clang-tidy checks" OFF)
 
 if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
   set(LIBCXX_DEFAULT_TEST_CONFIG "llvm-libc++-shared-gcc.cfg.in")
@@ -863,10 +862,6 @@ add_subdirectory(modules)
 
 set(LIBCXX_TEST_DEPS "cxx_experimental")
 
-if (LIBCXX_ENABLE_CLANG_TIDY)
-  list(APPEND LIBCXX_TEST_DEPS cxx-tidy)
-endif()
-
 list(APPEND LIBCXX_TEST_DEPS generate-cxx-modules)
 
 if (LIBCXX_INCLUDE_BENCHMARKS)
diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst
index 7bc0148c9ff0aa..390de789ddb643 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -132,3 +132,6 @@ Build System Changes
 
 - The ``LIBCXX_EXECUTOR`` and ``LIBCXXABI_EXECUTOR`` CMake variables have been removed. Please
   set ``LIBCXX_TEST_PARAMS`` to ``executor=<...>`` instead.
+
+- The Cmake variable ``LIBCXX_ENABLE_CLANG_TIDY`` has been removed. The build system has been changed
+  to automatically detect the presence of ``clang-tidy`` and the required ``Clang`` libraries.
diff --git a/libcxx/test/tools/CMakeLists.txt b/libcxx/test/tools/CMakeLists.txt
index e30ad6cdd8201f..6d99c53ad46d9f 100644
--- a/libcxx/test/tools/CMakeLists.txt
+++ b/libcxx/test/tools/CMakeLists.txt
@@ -1,12 +1,8 @@
 
 set(LIBCXX_TEST_TOOLS_PATH ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE)
 
-# TODO: Remove LIBCXX_ENABLE_CLANG_TIDY
-if(LIBCXX_ENABLE_CLANG_TIDY)
-  if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
-    message(STATUS "Clang-tidy can only be used when building libc++ with "
-                   "a clang compiler.")
-    return()
-  endif()
-  add_subdirectory(clang_tidy_checks)
+if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+  message(STATUS "Clang-tidy tests are disabled due to non-clang based compiler.")
+  return()
 endif()
+add_subdirectory(clang_tidy_checks)
diff --git a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
index 74905a0c3ed1c4..28eed614458336 100644
--- a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
+++ b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
@@ -9,22 +9,14 @@ set(Clang_DIR_SAVE ${Clang_DIR})
 # versions must match. Otherwise there likely will be ODR-violations. This had
 # led to crashes and incorrect output of the clang-tidy based checks.
 find_package(Clang ${CMAKE_CXX_COMPILER_VERSION})
-
-set(SOURCES
-    abi_tag_on_virtual.cpp
-    header_exportable_declarations.cpp
-    hide_from_abi.cpp
-    proper_version_checks.cpp
-    qualify_declval.cpp
-    robust_against_adl.cpp
-    uglify_attributes.cpp
-
-    libcpp_module.cpp
-   )
-
 if(NOT Clang_FOUND)
-  message(STATUS "Could not find a suitable version of the Clang development package;
-                  custom libc++ clang-tidy checks will not be available.")
+  message(STATUS "Clang-tidy tests are disabled since the "
+                 "Clang development package is unavailable.")
+  return()
+endif()
+if(NOT TARGET clangTidy)
+  message(STATUS "Clang-tidy tests are disabled since the "
+                 "Clang development package has no clangTidy target.")
   return()
 endif()
 
@@ -54,6 +46,38 @@ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
                         )
 endif()
 
+# In some cases even with the clangTidy target present the headers appear not to
+# be on the system. Run a short test to see whether the header is present.
+file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/test.cpp" "
+#if !__has_include(\"clang-tidy/ClangTidyCheck.h\")
+  # error No clang-tidy headers
+#endif
+int main(){}
+")
+try_compile(HAS_CLANG_TIDY_HEADERS
+  "${CMAKE_CURRENT_BINARY_DIR}"
+  "${CMAKE_CURRENT_BINARY_DIR}/test.cpp"
+   LINK_LIBRARIES clangTidy)
+
+if(NOT HAS_CLANG_TIDY_HEADERS)
+  message(STATUS "Clang-tidy tests are disabled since the "
+                 "clang-tidy headers are not present.")
+  return()
+endif()
+message(STATUS "Clang-tidy tests are enabled.")
+
+set(SOURCES
+    abi_tag_on_virtual.cpp
+    header_exportable_declarations.cpp
+    hide_from_abi.cpp
+    proper_version_checks.cpp
+    qualify_declval.cpp
+    robust_against_adl.cpp
+    uglify_attributes.cpp
+
+    libcpp_module.cpp
+   )
+
 add_library(cxx-tidy MODULE ${SOURCES})
 target_link_libraries(cxx-tidy clangTidy)
 
@@ -64,3 +88,5 @@ set_target_properties(cxx-tidy PROPERTIES
 
 set_target_properties(cxx-tidy PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
 set(CMAKE_SHARED_MODULE_SUFFIX_CXX .plugin) # Use a portable suffix to simplify how we can find it from Lit
+
+list(APPEND LIBCXX_TEST_DEPS cxx-tidy)
diff --git a/libcxx/utils/ci/buildkite-pipeline.yml b/libcxx/utils/ci/buildkite-pipeline.yml
index c43e414418729a..4bacdec8f8d6bc 100644
--- a/libcxx/utils/ci/buildkite-pipeline.yml
+++ b/libcxx/utils/ci/buildkite-pipeline.yml
@@ -43,7 +43,6 @@ definitions:
 
 environment_definitions:
   _common_env: &common_env
-      ENABLE_CLANG_TIDY: "On"
       LLVM_SYMBOLIZER_PATH: "/usr/bin/llvm-symbolizer-${LLVM_HEAD_VERSION}"
       CLANG_CRASH_DIAGNOSTICS_DIR: "crash_diagnostics"
       CC: clang-${LLVM_HEAD_VERSION}
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index a6f3eb174308b4..cc72f4639b1a9b 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -44,9 +44,6 @@ CMAKE               The CMake binary to use. This variable is optional.
 CLANG_FORMAT        The clang-format binary to use when generating the format
                     ignore list.
 
-ENABLE_CLANG_TIDY   Whether to compile and run clang-tidy checks. This variable
-                    is optional.
-
 EOF
 }
 
@@ -111,10 +108,6 @@ function clean() {
     rm -rf "${BUILD_DIR}"
 }
 
-if [ -z "${ENABLE_CLANG_TIDY}" ]; then
-    ENABLE_CLANG_TIDY=Off
-fi
-
 function generate-cmake-base() {
     echo "--- Generating CMake"
     ${CMAKE} \
@@ -126,7 +119,6 @@ function generate-cmake-base() {
           -DLIBCXX_ENABLE_WERROR=YES \
           -DLIBCXXABI_ENABLE_WERROR=YES \
           -DLIBUNWIND_ENABLE_WERROR=YES \
-          -DLIBCXX_ENABLE_CLANG_TIDY=${ENABLE_CLANG_TIDY} \
           -DLLVM_LIT_ARGS="-sv --xunit-xml-output test-results.xml --timeout=1500 --time-tests" \
           "${@}"
 }

@mordante
Copy link
Member Author

This was approved and reverted to breakage. This breakage should be addressed, however the original reporter of the breakage has not replied to this PR. Merging it since it blocks other work.

@mordante mordante merged commit 6775285 into llvm:main Apr 13, 2024
55 checks passed
@mordante mordante deleted the review/removes_clang_tidy_cmake_option branch April 13, 2024 10:32
bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
The clang-tidy selection in CMake was refactored in
llvm#81362. During review it was
suggested to remove this CMake option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants