build(cmake): Require compiler versions that support the C++20 features necessary for compiling ystdlib-cpp; Add toolchain file for LLVM Clang 16 on macOS 13 & 14.
#745
Conversation
WalkthroughThis pull request modifies the macOS build workflow and updates the CMake configuration. In the CI workflow, the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CMake as CMake Process
participant Setup as setup_toolchains()
participant Validate as validate_compiler_versions()
Dev->>CMake: Run CMake configuration
CMake->>Setup: Call setup_toolchains()
Setup-->>CMake: Set toolchain variables (conditional on macOS version)
CMake->>Validate: Call validate_compiler_versions()
Validate-->>CMake: Return validation outcome (error if unsupported)
CMake-->>Dev: Build system configured
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
C++20 features on macOS before version 15.
![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 (5)
components/core/cmake/Toolchains/llvm-clang-toolchain.cmake (2)
10-13: Verify that toolchain executables existThe script sets toolchain paths but doesn't verify that the executables actually exist at those paths, which could lead to cryptic build errors later.
+# Verify executables exist +foreach(TOOL_PATH "${CMAKE_C_COMPILER}" "${CMAKE_CXX_COMPILER}" "${CMAKE_AR}" "${CMAKE_RANLIB}") + if(NOT EXISTS "${TOOL_PATH}") + message(FATAL_ERROR "Required LLVM toolchain executable not found: ${TOOL_PATH}") + endif() +endforeach()
1-13: Consider adding version and platform information to the file nameThe current file name is generic and doesn't specify the LLVM version or platform. Since this is specifically for LLVM v16 on macOS, consider renaming the file to better reflect its purpose.
Consider renaming the file to something like
macos-llvm16-clang-toolchain.cmaketo clearly indicate its purpose and scope.components/core/CMakeLists.txt (3)
3-3: Use a relative path for including utils.cmakeThe include statement uses
${CMAKE_SOURCE_DIR}which makes assumptions about the project structure. For better modularity, consider using a relative path from this file to utils.cmake.-include("${CMAKE_SOURCE_DIR}/cmake/Toolchains/utils.cmake") +include("cmake/Toolchains/utils.cmake")
5-6: Add documentation about the setup_toolchains functionThe function
setup_toolchains()is called without any explanation of what it does or what parameters it accepts. Adding a brief comment would improve code readability.# Toolchain setup must come before the project() call -setup_toolchains() +# Set up appropriate toolchains based on platform detection (macOS uses LLVM v16) +setup_toolchains()
10-10: Add documentation about the validate_compiler_versions functionThe function
validate_compiler_versions()is called without any explanation of what it does or what parameters it accepts. Adding a brief comment would improve code readability.-validate_compiler_versions() +# Verify that compiler versions meet minimum requirements (C++20 support) +validate_compiler_versions()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/core/CMakeLists.txt(1 hunks)components/core/cmake/Toolchains/llvm-clang-toolchain.cmake(1 hunks)components/core/cmake/Toolchains/utils.cmake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/cmake/Toolchains/utils.cmake
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
3-10:❓ Verification inconclusive
Verify toolchain utilities are properly implemented
The PR introduces two new functions (
setup_toolchains()andvalidate_compiler_versions()) that aren't visible in this review. Ensure they properly handle all platforms and error cases.
🏁 Script executed:
#!/bin/bash # Check the implementation of the utils.cmake file to ensure proper error handling # First, check if the utils.cmake file exists if [ -f "${PWD}/components/core/cmake/Toolchains/utils.cmake" ]; then echo "Found utils.cmake file. Checking implementation..." # Check if setup_toolchains function exists and handles platforms grep -A 20 "function(setup_toolchains" "${PWD}/components/core/cmake/Toolchains/utils.cmake" # Check if validate_compiler_versions function exists and checks for C++20 grep -A 20 "function(validate_compiler_versions" "${PWD}/components/core/cmake/Toolchains/utils.cmake" # Check if there's platform detection for macOS grep -i "darwin\|apple\|macos" "${PWD}/components/core/cmake/Toolchains/utils.cmake" else echo "ERROR: utils.cmake file not found at expected location" fiLength of output: 2236
Toolchain utilities implementation appears sound, but please verify non-Darwin support
- The
setup_toolchains()function is present and correctly detects Darwin systems by invokingsw_versto obtain the macOS version. It conditionally sets the toolchain file when the macOS version is below "15".- The
validate_compiler_versions()function correctly sets minimum required versions for AppleClang (16), Clang (16), and GNU (11), and it properly issues a fatal error for unsupported compilers.- Note that the current implementation in
setup_toolchains()explicitly handles Darwin systems only. Please confirm that non-Darwin platforms are either intentionally unconfigured at this stage or that their toolchain setup is managed elsewhere.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai void the rest of your existing comments. Too verbose for this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/core/CMakeLists.txt (1)
2-7: Toolchain Setup and ConfigurationLines 2–7 now include the explicit inclusion of
cmake/Toolchains/utils.cmakeand the call tosetup_toolchains(). This ordering is critical as all toolchain settings must be established before theproject()command is invoked. Please verify that thesetup_toolchains()function is correctly defined in the included file and that its implementation properly configures the LLVM v16 toolchain for macOS. Also, ensure that any toolchain-related functions or variables follow the project’s naming conventions (for example, using aCLP_prefix) as noted in earlier reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/core/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: filter-relevant-changes
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
10-11: Compiler Version ValidationThe addition of the
validate_compiler_versions()call immediately after the project declaration is a good measure to ensure that the correct compiler is being used. This function should verify that the compiler meets the minimum requirements (i.e. LLVM Clang v16) for supporting C++20 features on macOS 13/14. Please ensure that its implementation inutils.cmakeaccurately detects incompatible compiler versions and provides clear error messages.
|
@Bill-hbrhbr I'll resolve my existing comments as requested. ✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Sorry, since we're hardcoding the version to 16, can we rename this file to llvm-16-clang-toolchain.cmake?
There was a problem hiding this comment.
renamed to llvm-clang-16-toolchain.cmake
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Can you change this line to:
A recent compiler that supports C++20
The rationale being that we can avoid having to keep that doc up-to-date with the precise versions required, but if the user has too low a compiler version, the validation in this PR will inform them.
For the PR title, how about:
build(cmake): Require compiler versions that support the C++20 features necessary for compiling `ystdlib-cpp`; Add toolchain file for LLVM Clang 16 on macOS 13 & 14.
C++20 features on macOS 13/14 via LLVM Clang v16.ystdlib-cpp; Add toolchain file for LLVM Clang 16 on macOS 13 & 14.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/src/dev-guide/components-core/index.md (1)
9-11: Enhance Compiler Requirement Clarity
The updated text now clearly specifies that a recent compiler supporting full C++20 features is needed. Explicitly listing bothstd::spanandstd::source_locationhelps users understand the minimum feature set required by the project. This aligns with the changes in your CMake configuration and ensures consistency in the documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/src/dev-guide/components-core/index.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
Description
To meet the
c++20feature requirements ofystdlib-cpp, we must use Clang/AppleClang version 16 or later.On macOS 13 and 14, a good solution is to install LLVM Clang v16 compilers. However, by default CMake only picks up the new toolchain's
llvm-ranlib, while still using/usr/bin/arfor AR, which creates a mismatch in tool versions. For instance, after runningbrew install llvm@16on macOS 14:It’s essential that both AR and RANLIB originate from the same LLVM distribution, otherwise linker errors will arise.
While one could technically use
/usr/bin/artogether with/usr/bin/ranlib, since we're using LLVM'sclangandclang++, it's best to ensure consistency by usingllvm-arandllvm-ranlibfrom the same distribution.To ensure consistency within the toolchain, we add an OS platform detection + loading the appropriate toolchain setting file before the
project(CLP ...)call. We also add a compiler check after the toolchains have been loaded.All the new CMake utilities are inside
./cmake/Toolchains.Checklist
breaking change.
Validation performed
Summary by CodeRabbit