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

Remove the requirement to link with clangTidy #72140

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented Mar 3, 2024

Summary

None

Purpose of change

A requirement to link against clangTidy was added as part of the transition to LLVM 17 (#71429), but it breaks the build against LLVM 17 for me locally.

Describe the solution

After some discussion below, it seems like @BrettDong and I have found an alternative solution that works for both of us. I've updated this PR accordingly.

This has to do with a difference between the default linking behaviour on Linux and macOS.

Describe alternatives you've considered

Testing

This is the test.

Additional context

@github-actions github-actions bot added Code: Build Issues regarding different builds and build environments Code: Tooling Tooling that is not part of the main game but is part of the repo. astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Mar 3, 2024
@jbytheway
Copy link
Contributor Author

Seems to be working fine (CI run).

So I will mark this as a draft to avoid it getting merged accidentally while I wait to hear from @BrettDong on what inspired the change.

@jbytheway jbytheway marked this pull request as draft March 3, 2024 14:59
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 3, 2024
@BrettDong
Copy link
Member

I got a huge number of "undefined references" linking error when building the plugin locally on my macOS system. The compiler used is Apple Clang from Xcode, and the LLVM library is installed from Homebrew package manager.

@jbytheway
Copy link
Contributor Author

Had you previously built successfully on the same system against LLVM 16?

@BrettDong
Copy link
Member

Building with LLVM 16 was successful but linking error occurs with LLVM 17.

@BrettDong
Copy link
Member

A portable way to create our plugin target in CMake is probably to use llvm_add_library() function
https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/cmake/modules/AddLLVM.cmake#L372-L401

@jbytheway
Copy link
Contributor Author

I also did some poking around, I wonder if we might be able to work around the issue using -undefined dynamic_lookup when linking the plugin.

This is based on some other GitHub issues. See, for example, this comment and this PR.

If this is the issue, though, I'm not sure it explains why LLVM 16 worked but 17 doesn't.

@BrettDong
Copy link
Member

I can successfully build and load the plugin in clang-tidy 17 on macOS with this patch

diff --git a/tools/clang-tidy-plugin/CMakeLists.txt b/tools/clang-tidy-plugin/CMakeLists.txt
index 46f3021206..4d5f5e35fb 100644
--- a/tools/clang-tidy-plugin/CMakeLists.txt
+++ b/tools/clang-tidy-plugin/CMakeLists.txt
@@ -55,7 +55,9 @@ if (CATA_CLANG_TIDY_EXECUTABLE)
 else ()
     set(CataAnalyzerName CataAnalyzerPlugin)
     add_library(${CataAnalyzerName} MODULE ${CataAnalyzerSrc})
-    target_link_libraries(${CataAnalyzerName} PRIVATE clangTidy)
+    if ("${CMAKE_SYSTEM_NAME}" MATCHES "Darwin")
+        target_link_options(${CataAnalyzerName} PRIVATE -undefined dynamic_lookup)
+    endif ()
 endif ()
 
 target_include_directories(${CataAnalyzerName} SYSTEM PRIVATE

@jbytheway
Copy link
Contributor Author

Awesome. I'll update this PR accordingly.

This was added as part of the transition to LLVM 17, but it breaks the
build against LLVM 17 for me locally.

The fix was needed for the macOS build, but we've now found an
alternative solution to that.

See CleverRaven#72140 for details.
@jbytheway jbytheway force-pushed the dont-link-against-clangTidy branch from 881e3cf to fbad490 Compare March 5, 2024 12:08
@jbytheway jbytheway marked this pull request as ready for review March 5, 2024 12:09
@jbytheway jbytheway changed the title [WIP] Remove the requirement to link with clangTidy Remove the requirement to link with clangTidy Mar 5, 2024
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 5, 2024
@dseguin dseguin merged commit f9ebfa1 into CleverRaven:master Mar 6, 2024
19 of 29 checks passed
@jbytheway jbytheway deleted the dont-link-against-clangTidy branch March 10, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Build Issues regarding different builds and build environments Code: Tooling Tooling that is not part of the main game but is part of the repo. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants