refactor(cmake): clean up toolchain and CMakeLists separation#379
Merged
16bit-ykiko merged 9 commits intomainfrom Mar 31, 2026
Merged
refactor(cmake): clean up toolchain and CMakeLists separation#37916bit-ykiko merged 9 commits intomainfrom
16bit-ykiko merged 9 commits intomainfrom
Conversation
- toolchain.cmake: only clang/lld specific setup (compiler paths, LLVM tools, -fuse-ld=lld) - Promote -ffunction-sections/-fdata-sections and platform linker flags (gc-sections, dead_strip, static-libstdc++) to global scope so FetchContent deps also benefit from dead code elimination - Promote _LIBCPP_DISABLE_AVAILABILITY to global scope - Simplify clice_options to only project-specific settings: -fno-rtti, -fno-exceptions, warnings, compile definitions, Win32 system libs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded configurable CMake build options, globalized linker/compile INIT flags and compiler-launcher support (ccache/sccache), CI cache restore/reporting, conditional benchmark and release packaging targets (strip + archive), an archive helper, a clang-resource copy target, and replaced explicit core source list with a recursive glob. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Runner
participant Cache as Actions Cache
participant Launcher as sccache/ccache
participant CMake as CMake Configure/Build
participant Archiver as cmake/archive.cmake
Runner->>Cache: restore cache (OS/build-type key)
alt cache hit
Cache-->>Runner: cached launcher/artifacts
Runner->>Launcher: make launcher available
else cache miss
Runner-->>Runner: proceed without launcher
end
Runner->>CMake: configure (apply *_INIT linker flags, CLICE options, set launcher)
CMake->>Launcher: invoke compiler via launcher (if present)
alt CLICE_RELEASE enabled
CMake->>Archiver: run strip, collect symbols, create package archive
end
Runner->>Cache: save cache (post-build)
Runner->>Runner: show cache stats (sccache/ccache --show-stats)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/toolchain.cmake (1)
27-36: Consider addingCMAKE_MODULE_LINKER_FLAGSfor consistency.The linker flags are appended to
CMAKE_EXE_LINKER_FLAGSandCMAKE_SHARED_LINKER_FLAGS, butCMAKE_MODULE_LINKER_FLAGSis omitted. This is inconsistent with the toolchain file (which sets all three*_INITvariables) and the LTO handling below (lines 47-49 which also sets all three). If any MODULE library targets exist or are added in the future, they won't benefit from dead code elimination.Proposed fix
if(WIN32) string(APPEND CMAKE_EXE_LINKER_FLAGS " -Wl,/OPT:REF") string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,/OPT:REF") + string(APPEND CMAKE_MODULE_LINKER_FLAGS " -Wl,/OPT:REF") elseif(APPLE) string(APPEND CMAKE_EXE_LINKER_FLAGS " -Wl,-dead_strip") string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,-dead_strip") + string(APPEND CMAKE_MODULE_LINKER_FLAGS " -Wl,-dead_strip") else() string(APPEND CMAKE_EXE_LINKER_FLAGS " -static-libstdc++ -static-libgcc -Wl,--gc-sections") string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,--gc-sections") + string(APPEND CMAKE_MODULE_LINKER_FLAGS " -Wl,--gc-sections") endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/toolchain.cmake` around lines 27 - 36, Add the missing CMAKE_MODULE_LINKER_FLAGS_INIT setting to mirror the existing CMAKE_EXE_LINKER_FLAGS_INIT and CMAKE_SHARED_LINKER_FLAGS_INIT entries: in the WIN32 branch set CMAKE_MODULE_LINKER_FLAGS_INIT to "-fuse-ld=lld-link" and in the non-WIN32 branch set CMAKE_MODULE_LINKER_FLAGS_INIT to "-fuse-ld=lld" so MODULE targets receive the same linker as EXE/SHARED (consistent with the LTO handling that sets all three *_INIT variables); update the same symbolic variables CMAKE_EXE_LINKER_FLAGS_INIT, CMAKE_SHARED_LINKER_FLAGS_INIT, and CMAKE_MODULE_LINKER_FLAGS_INIT accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 27-36: The WIN32 branch is applying MSVC-specific linker flags
(-Wl,/OPT:REF) which will break MinGW/GCC; change the condition to detect
MSVC/clang-cl rather than generic WIN32 (e.g. use MSVC or CMAKE_CXX_COMPILER_ID
== "MSVC" or "Clang" with MSVC frontend) before appending the MSVC linker flags
to CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS, and keep the existing
else branch for other Unix-like/linker toolchains; update the conditional around
the string(APPEND ... "-Wl,/OPT:REF") and corresponding shared linker flag so
only MSVC/clang-cl builds receive those flags.
---
Nitpick comments:
In `@cmake/toolchain.cmake`:
- Around line 27-36: Add the missing CMAKE_MODULE_LINKER_FLAGS_INIT setting to
mirror the existing CMAKE_EXE_LINKER_FLAGS_INIT and
CMAKE_SHARED_LINKER_FLAGS_INIT entries: in the WIN32 branch set
CMAKE_MODULE_LINKER_FLAGS_INIT to "-fuse-ld=lld-link" and in the non-WIN32
branch set CMAKE_MODULE_LINKER_FLAGS_INIT to "-fuse-ld=lld" so MODULE targets
receive the same linker as EXE/SHARED (consistent with the LTO handling that
sets all three *_INIT variables); update the same symbolic variables
CMAKE_EXE_LINKER_FLAGS_INIT, CMAKE_SHARED_LINKER_FLAGS_INIT, and
CMAKE_MODULE_LINKER_FLAGS_INIT accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 433b819a-d0f6-406c-8be0-45dcaed8a8fa
📒 Files selected for processing (2)
CMakeLists.txtcmake/toolchain.cmake
- Move compiler cache (ccache/sccache) detection into toolchain.cmake - Add platform-specific cache deps in pixi.toml (ccache for linux/mac, sccache for win) - Add compiler cache restore/stats to CI workflow - Declare missing options (CLICE_USE_LIBCXX, CLICE_OFFLINE_BUILD, etc.) - Fix duplicate include_resolver.cpp source - Replace configure-time file(COPY) with build-time custom target for clang resources - Gate scan_benchmark behind CLICE_ENABLE_BENCHMARK option - Add -fno-exceptions to clice_options Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CLICE_RELEASE option that enables LTO and produces two archives: - clice.tar.gz: stripped binary + clang resource dir + config - clice-symbol.tar.gz: debug symbols for crash analysis Platform-specific symbol handling: objcopy on Linux, dsymutil on macOS, PDB on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test-cmake.yml (1)
24-26: Consider a slower-rotating cache key.Lines 24-26 key the compiler cache by
github.sha, which gives every commit its own immutable cache entry. Sinceccache/sccachealready hash compiler inputs internally, a key based on toolchain inputs usually reuses storage better and avoids cache churn.♻️ Suggested key shape
- key: ${{ runner.os }}-${{ matrix.build_type }}-ccache-${{ github.sha }} + key: ${{ runner.os }}-${{ matrix.build_type }}-ccache-${{ hashFiles('pixi.toml', 'cmake/toolchain.cmake', 'CMakeLists.txt') }} restore-keys: | ${{ runner.os }}-${{ matrix.build_type }}-ccache-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-cmake.yml around lines 24 - 26, The cache key currently pins to every commit via the `${{ github.sha }}` entry (see the `key:` and `restore-keys:` lines), causing excessive cache churn; change the key to a slower-rotating identity that represents compiler/toolchain and build configuration (for example use `${{ runner.os }}-${{ matrix.build_type }}-ccache-<toolchain-version-or-hash>` or a hash of relevant files like CMakeLists/lockfiles) and keep the current pattern as a restore fallback so cache hits persist across commits while invalidating only when compiler or input files change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-cmake.yml:
- Line 23: The cache path expression currently falls back to the Linux path for
non-Windows runners; update the conditional that builds the path (the expression
using runner.os and the existing '~/ .cache/ccache' fallback) to add an explicit
macOS case so macOS runners use '~/Library/Caches/ccache' (while keeping the
Windows '~\AppData\Local\Mozilla\sccache' and Linux '~/.cache/ccache' branches)
— modify the conditional expression that references runner.os to include
runner.os == 'macOS' and return the macOS ccache directory.
---
Nitpick comments:
In @.github/workflows/test-cmake.yml:
- Around line 24-26: The cache key currently pins to every commit via the `${{
github.sha }}` entry (see the `key:` and `restore-keys:` lines), causing
excessive cache churn; change the key to a slower-rotating identity that
represents compiler/toolchain and build configuration (for example use `${{
runner.os }}-${{ matrix.build_type }}-ccache-<toolchain-version-or-hash>` or a
hash of relevant files like CMakeLists/lockfiles) and keep the current pattern
as a restore fallback so cache hits persist across commits while invalidating
only when compiler or input files change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebc95b2e-1d6b-4b5a-9fd2-a2e8462dcbe0
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/test-cmake.ymlCMakeLists.txtcmake/toolchain.cmakepixi.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/archive.cmake (1)
1-6: Add input validation for required variables.If
OUTPUTorWORK_DIRare undefined, the script will either silently misbehave or produce a confusing error. Adding explicit checks improves debuggability.🛡️ Proposed input validation
+if(NOT DEFINED OUTPUT) + message(FATAL_ERROR "OUTPUT variable is required") +endif() + +if(NOT DEFINED WORK_DIR) + message(FATAL_ERROR "WORK_DIR variable is required") +endif() + if(OUTPUT MATCHES "\\.tar\\.gz$") execute_process( COMMAND ${CMAKE_COMMAND} -E tar czf "${OUTPUT}" .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/archive.cmake` around lines 1 - 6, Add explicit input validation for the required variables used by the tar step: check that OUTPUT and WORK_DIR are defined and non-empty before the if(OUTPUT MATCHES ...) block, and emit clear fatal errors if missing (e.g., using message(FATAL_ERROR) mentioning the variable name and expected value). Also validate that WORK_DIR exists or is a directory (using if(NOT IS_DIRECTORY ...) or similar) before running execute_process, and adjust the flow so the tar command only runs when all validations pass; reference the variables OUTPUT and WORK_DIR and the existing execute_process call to locate where to insert these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/archive.cmake`:
- Around line 1-15: The script ignores the ARCHIVE_DIR parameter and archives
"." from WORK_DIR; update the execute_process calls in archive.cmake (the blocks
using OUTPUT and WORK_DIR and the COMMAND invoking ${CMAKE_COMMAND} -E tar) to
use the ARCHIVE_DIR as the source directory instead of "." (i.e., pass the
ARCHIVE_DIR path as the last argument to the tar/zip commands), ensuring both
the gz and zip branches reference ARCHIVE_DIR; alternatively, if ARCHIVE_DIR is
intentionally unused, remove the ARCHIVE_DIR -D flag from callers — but do not
leave ARCHIVE_DIR unused in the script.
---
Nitpick comments:
In `@cmake/archive.cmake`:
- Around line 1-6: Add explicit input validation for the required variables used
by the tar step: check that OUTPUT and WORK_DIR are defined and non-empty before
the if(OUTPUT MATCHES ...) block, and emit clear fatal errors if missing (e.g.,
using message(FATAL_ERROR) mentioning the variable name and expected value).
Also validate that WORK_DIR exists or is a directory (using if(NOT IS_DIRECTORY
...) or similar) before running execute_process, and adjust the flow so the tar
command only runs when all validations pass; reference the variables OUTPUT and
WORK_DIR and the existing execute_process call to locate where to insert these
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b35063e-7275-433f-8c1a-99c9f4ddf0bf
📒 Files selected for processing (2)
CMakeLists.txtcmake/archive.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
- Move release packaging logic (strip/pack) to cmake/release.cmake - Replace manual source file list with GLOB_RECURSE for clice-core - Fix duplicate include_resolver.cpp entry - Use build-time copy for clang resource directory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
CMakeLists.txt (1)
33-41:⚠️ Potential issue | 🟠 MajorDon't send every Windows toolchain through the
/OPT:REFbranch.Because this condition is still
WIN32, MinGW/MSYS2 builds will inherit the/OPT:REFpath as soon as the optional clang toolchain is not used. CMake explicitly calls out that linker-option forwarding is compiler-driver specific, so this needs the same MSVC/clang-cl frontend gating used later in the file. Please mirror the same change in the debug/OPT:NOICFblock below as well. (cmake.org)Proposed fix
-if(WIN32) +if(MSVC OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND + CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC")) string(APPEND CMAKE_EXE_LINKER_FLAGS " -Wl,/OPT:REF") string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,/OPT:REF") elseif(APPLE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 33 - 41, Replace the broad WIN32 check that appends MSVC-specific flags (/OPT:REF) with the same frontend gate used later: only append "/OPT:REF" to CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS when the toolchain is an MSVC-style driver (e.g., if(MSVC) OR when the compiler frontend is clang-cl), not for all WIN32 builds (so MinGW/clang/gcc on Windows won't get MSVC flags); apply the identical change to the debug block that currently appends "/OPT:NOICF" so both places use the MSVC/clang-cl frontend condition instead of plain WIN32.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/release.cmake`:
- Around line 57-61: The clice-pack-symbol custom target incorrectly uses the
file-copy form of CMake's -E copy for CLICE_SYMBOL_NAME, which on macOS is a
.dSYM bundle directory; update the target to use the directory form by replacing
the -E copy invocation with -E copy_directory and target the source
"${CLICE_SYMBOL_DIR}/${CLICE_SYMBOL_NAME}" and destination
"${CLICE_SYMBOL_DIR}/pack/${CLICE_SYMBOL_NAME}" so the top-level bundle
structure is preserved; keep the rest of the target (DEPENDS clice-strip, rm -rf
pack, make_directory pack) unchanged.
- Around line 18-23: The Windows branch for the clice-strip target currently
only creates ${CLICE_SYMBOL_DIR} and never stages the linker PDB, causing
clice-pack-symbol to fail; modify the clice-strip target to also copy the
linker-generated PDB into ${CLICE_SYMBOL_DIR}/${CLICE_SYMBOL_NAME} by using the
generator expression $<TARGET_PDB_FILE:clice> (or ${CLICE_PDB} set to that
expression) and adding a COMMAND that copies $<TARGET_PDB_FILE:clice> to the
destination, keeping the target name clice-strip and preserving the existing
DEPENDS clice and COMMENT.
In `@CMakeLists.txt`:
- Around line 99-105: The MSVC/clang-cl branch sets
target_compile_options(clice_options ...) with the incorrect flag `/EHsc-` which
still enables exception handling; remove `/EHsc-` from the MSVC branch so the
behavior matches the non-MSVC branch that disables exceptions, and if clang-cl
requires different flags, add a separate conditional for CMAKE_CXX_COMPILER_ID
MATCHES "Clang" AND CMAKE_CXX_COMPILER_FRONTEND_VARIANT STREQUAL "MSVC" to set
clang-cl-specific options instead of bundling it with MSVC in the
target_compile_options(clice_options ...) call.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 33-41: Replace the broad WIN32 check that appends MSVC-specific
flags (/OPT:REF) with the same frontend gate used later: only append "/OPT:REF"
to CMAKE_EXE_LINKER_FLAGS and CMAKE_SHARED_LINKER_FLAGS when the toolchain is an
MSVC-style driver (e.g., if(MSVC) OR when the compiler frontend is clang-cl),
not for all WIN32 builds (so MinGW/clang/gcc on Windows won't get MSVC flags);
apply the identical change to the debug block that currently appends
"/OPT:NOICF" so both places use the MSVC/clang-cl frontend condition instead of
plain WIN32.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f454d25b-203b-4363-a7d9-eb9cbcdd310c
📒 Files selected for processing (2)
CMakeLists.txtcmake/release.cmake
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The sccache daemon keeps a lock on pixi env bin directory, causing EBUSY error during post-run cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix WIN32 linker flag check to use MSVC/clang-cl detection instead - Fix /EHsc- to /EHs-c- to properly disable exceptions on MSVC - Copy PDB file in Windows clice-strip target - Use copy_directory for macOS dSYM bundle in symbol packaging - Use env vars (CCACHE_DIR/SCCACHE_DIR) for CI compiler cache paths - Add zero-stats and stop-server steps for proper cache lifecycle Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move sccache --stop-server to after tests (not between build and test), since integration tests can re-launch the sccache daemon. Also add stop-server to benchmark workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Match eventide's pattern: stop any existing sccache server before resetting stats to ensure a clean starting state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
added a commit
that referenced
this pull request
Mar 31, 2026
## Summary - Restore comment explaining clang-cl manual ASan runtime linking workaround - Restore comment explaining OPT:NOICF for ASan ODR false positives on Windows These were accidentally removed in #379. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved internal build configuration for Debug builds on Windows to enhance development and testing infrastructure with better error detection and optimization settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve CMake build system: cleaner separation, compiler caching, and release packaging.
Toolchain & Build
toolchain.cmaketo only contain clang/lld-specific setup (compiler paths, linker selection, llvm tools), allowing other toolchains like GCC to work without it-ffunction-sections,--gc-sections,-static-libstdc++, etc.) toCMakeLists.txtso they apply regardless of toolchain-fno-exceptionsto project compile options; fix/EHs-c-for proper MSVC exception disablingWIN32for MSVC-specific linker flagsCLICE_USE_LIBCXX,CLICE_OFFLINE_BUILD,CLICE_ENABLE_BENCHMARK,CLICE_RELEASERelease Packaging (
cmake/release.cmake).debug/.dSYM/.pdb)$<TARGET_PDB_FILE:clice>; macOS: usecopy_directoryfor dSYM bundlecmake/archive.cmakehelper for cross-platform archive creation-DCLICE_RELEASE=ON(auto-enables LTO)Code Cleanup
GLOB_RECURSEfor clice-coreinclude_resolver.cppentryadd_custom_targetfor clang resource dir copy (instead of configure-timefile(COPY))scan_benchmarkbehindCLICE_ENABLE_BENCHMARKoptionCI
CCACHE_DIR/SCCACHE_DIR) andactions/cachefor persistenceCLICE_ENABLE_BENCHMARK=ONin benchmark workflowTest plan