-
Notifications
You must be signed in to change notification settings - Fork 10
build(clang): support clang 20 #239
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
base: unstable
Are you sure you want to change the base?
Conversation
WalkthroughThe versions of the external dependencies fmt and spdlog were updated in their respective CMake configuration files. The URLs, archive names, and SHA256 hashes were modified to reflect the new versions, with no changes to build arguments or logic. Changes
Estimated code review effort1 (~2 minutes) Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
cmake/spdlog.cmake (1)
16-38
: Mismatch between compile definition and CMake option will break the buildYou unconditionally add the pre-processor macro
SPDLOG_FMT_EXTERNAL
(line 16) but still configure spdlog with-DSPDLOG_FMT_EXTERNAL=OFF
(line 37).
With spdlog ≥ 1.12 this combination is illegal: headers will expect an external fmt, whereas the library is compiled with the bundled one, causing ODR / link errors.- -DSPDLOG_FMT_EXTERNAL=OFF + # Build spdlog against the already-built fmt target + -DSPDLOG_FMT_EXTERNAL=ON(or drop the
add_definitions()
entirely and leave the option OFF).Please adjust one side or the other before the Clang-20 roll-out hits CI.
cmake/fmt.cmake (1)
30-38
: Unknown CMake optionSHARED_LIBS
; use the canonical one
-DSHARED_LIBS=FALSE
(line 37) is not a recognised toggle in fmt’s CMakeLists; the standard variable isBUILD_SHARED_LIBS
. The current flag is silently ignored, so you might accidentally ship shared libs depending on the user’s default.- -DSHARED_LIBS=FALSE + -DBUILD_SHARED_LIBS=OFF
🧹 Nitpick comments (2)
cmake/spdlog.cmake (1)
22-26
: Hard-coded version strings appear three times – factor them out
1.15.3
is repeated in URL, hash-comment and file name. A singleset(SPDLOG_VERSION 1.15.3)
near the top would reduce future churn and copy-paste mistakes.cmake/fmt.cmake (1)
17-24
: Consider centralising the fmt version string
11.2.0
occurs in URL, hash comment and file name; aset(FMT_VERSION 11.2.0)
would DRY this up exactly as suggested for spdlog.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmake/fmt.cmake
(1 hunks)cmake/spdlog.cmake
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (1)
cmake/spdlog.cmake (1)
22-24
: SHA256 checksum verifiedThe SHA256 for
spdlog
v1.15.3 matches the value incmake/spdlog.cmake
(lines 22–24). No changes are needed—this is safe to merge.
support clang 20
Summary by CodeRabbit