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 (
|
C++20 features on macOS before version 15.
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. |
kirkrodrigues
left a comment
There was a problem hiding this comment.
A few more suggestions.
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>
kirkrodrigues
left a comment
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)
…es necessary for compiling `ystdlib-cpp`; Add toolchain file for LLVM Clang 16 on macOS 13 & 14. (y-scope#745) Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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