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. #85262

Merged
merged 1 commit into from
Mar 18, 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 requested a review from a team as a code owner March 14, 2024 16:37
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 14, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 14, 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/85262.diff

6 Files Affected:

  • (modified) libcxx/CMakeLists.txt (-5)
  • (modified) libcxx/docs/ReleaseNotes/19.rst (+2)
  • (modified) libcxx/test/tools/CMakeLists.txt (+4-8)
  • (modified) libcxx/test/tools/clang_tidy_checks/CMakeLists.txt (+8-5)
  • (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 2b62a36ca8e5cd..9a45a9a32b681e 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -80,6 +80,8 @@ Deprecations and Removals
   libatomic is not available. If you are one such user, please reach out to the libc++ developers so we can collaborate
   on a path for supporting atomics properly on freestanding platforms.
 
+- 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.
 
 Upcoming Deprecations and Removals
 ----------------------------------
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..27824e64c82a65 100644
--- a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
+++ b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
@@ -9,6 +9,13 @@ 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})
+if(NOT Clang_FOUND)
+  message(STATUS "Clang-tidy tests are disabled since the "
+	             "Clang development package is unavailable.")
+  return()
+else()
+  message(STATUS "Clang-tidy tests are enabled.")
+endif()
 
 set(SOURCES
     abi_tag_on_virtual.cpp
@@ -22,11 +29,7 @@ set(SOURCES
     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.")
-  return()
-endif()
+list(APPEND LIBCXX_TEST_DEPS cxx-tidy)
 
 set(LLVM_DIR "${LLVM_DIR_SAVE}" CACHE PATH "The directory containing a CMake configuration file for LLVM." FORCE)
 set(Clang_DIR "${Clang_DIR_SAVE}" CACHE PATH "The directory containing a CMake configuration file for Clang." FORCE)
diff --git a/libcxx/utils/ci/buildkite-pipeline.yml b/libcxx/utils/ci/buildkite-pipeline.yml
index e42262620d5fb0..0761b40c784d42 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 2905745355b68e..10b0ed607bce79 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 mordante force-pushed the review/removes_clang_tidy_cmake_option branch from 26fdc97 to c76d769 Compare March 15, 2024 16:26
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

libcxx/docs/ReleaseNotes/19.rst Outdated Show resolved Hide resolved
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 c76d769 to 0bd45dd Compare March 18, 2024 13:32
@mordante mordante merged commit 4109b18 into llvm:main Mar 18, 2024
9 of 10 checks passed
@mordante mordante deleted the review/removes_clang_tidy_cmake_option branch March 18, 2024 13:32
@jplehr
Copy link
Contributor

jplehr commented Mar 18, 2024

This broke this bot: https://lab.llvm.org/buildbot/#/builders/165/builds/50674

Should the CMake invocation be changed on the bots?

mordante added a commit that referenced this pull request Mar 18, 2024
This reverts commit 4109b18.

It looks like the automatic detection has false positives. This broke
the following build #85262
@mordante
Copy link
Member Author

As discussed on Discord this seems like a bug in the new clang-tidy detection. I've reverted the patch and ping you when I have a new review ready for testing.

@mordante
Copy link
Member Author

#85794 is the reworked patch.

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
The clang-tidy selection in CMake was refactored in
llvm#81362. During review it was
suggested to remove this CMake option.
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
This reverts commit 4109b18.

It looks like the automatic detection has false positives. This broke
the following build llvm#85262
blueboxd pushed a commit to blueboxd/libcxx that referenced this pull request May 4, 2024
This reverts commit 4109b18ee5de1346c2b89a5c89b86bae5c8631d3.

It looks like the automatic detection has false positives. This broke
the following build llvm/llvm-project#85262

NOKEYCHECK=True
GitOrigin-RevId: f8042171552ca16e77a5e9367188e7f218474380
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.

None yet

4 participants