-
Notifications
You must be signed in to change notification settings - Fork 8
build(cmake): Lower minimum Clang version for Velox compatibility: #72
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
Changes from 12 commits
ce7241c
1784467
292e6de
5070e26
fa39bd8
59f3679
f6df622
af310c9
3d288e1
21680f6
75fb463
ed0ea67
7f5e228
9ef42c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,21 +43,11 @@ jobs: | |
| run: "npm install -g @go-task/cli" | ||
|
|
||
| - if: "'macos-14' == matrix.os" | ||
| name: "Install macOS 14 deps: coreutils (for md5sum) and Apple Clang 16 (for C++20)" | ||
| name: "Install macOS 14 deps: coreutils (for md5sum)" | ||
| run: |- | ||
| brew install coreutils | ||
| brew install llvm@16 | ||
|
|
||
|
Comment on lines
45
to
49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainEnsure md5sum is on PATH after installing coreutils on macOS Homebrew’s coreutils installs md5sum under gnubin (not on PATH by default). If any step calls md5sum, it will fail unless you add gnubin to PATH or use gmd5sum explicitly. Apply this diff to export gnubin to PATH robustly: - run: |-
- brew install coreutils
+ run: |-
+ brew install coreutils
+ # Ensure md5sum is available on PATH (coreutils installs it under gnubin).
+ echo "$(brew --prefix coreutils)/libexec/gnubin" >> "$GITHUB_PATH"To verify whether md5sum (or gmd5sum) is actually used in the repo: 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Searching for md5sum usage..."
rg -n -C2 -g '!**/node_modules/**' -P '\b(g?md5sum)\b' || echo "No md5sum usage found."Length of output: 200 No md5sum usage detected – coreutils install step may be unnecessary I searched the repo for any invocations of
🤖 Prompt for AI Agents |
||
| - name: "Run unit tests and examples" | ||
| env: >- | ||
| ${{ | ||
| 'macos-14' == matrix.os | ||
| && fromJson('{ | ||
| "CC": "/opt/homebrew/opt/llvm@16/bin/clang", | ||
| "CXX": "/opt/homebrew/opt/llvm@16/bin/clang++" | ||
| }') | ||
| || fromJson('{}') | ||
| }} | ||
| # Currently unit tests rely on cassert and fail to compile in release mode. | ||
|
Bill-hbrhbr marked this conversation as resolved.
|
||
| run: |- | ||
| task test:run-debug | ||
|
|
||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # Used for CMake toolchain setup. Sets `VAR_NAME` to `BINARY_PATH` after verifying the toolchain | ||
| # binary exists. Stops configuration if the required binary is missing. | ||
| # | ||
| # @param {string} VAR_NAME Variable name to set. | ||
| # @param {string} BINARY_PATH Path to the CMake toolchain binary. | ||
| function(set_toolchain_binary_var VAR_NAME BINARY_PATH) | ||
| if(NOT EXISTS "${BINARY_PATH}") | ||
| message(FATAL_ERROR "Required CMake toolchain binary not found: ${BINARY_PATH}") | ||
| endif() | ||
| set(${VAR_NAME} "${BINARY_PATH}" PARENT_SCOPE) | ||
| endfunction() | ||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
||
|
|
||
| message(STATUS "Setting up LLVM v15 toolchain...") | ||
|
|
||
| execute_process( | ||
| COMMAND | ||
| "brew" "--prefix" "llvm@15" | ||
| RESULT_VARIABLE BREW_RESULT | ||
| OUTPUT_VARIABLE LLVM_TOOLCHAIN_PREFIX | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
||
| if(NOT 0 EQUAL BREW_RESULT) | ||
| message( | ||
| FATAL_ERROR | ||
| "Failed to locate LLVM v15 using Homebrew. Please ensure llvm@15 is installed: 'brew" | ||
| " install llvm@15'" | ||
| ) | ||
| endif() | ||
|
|
||
| set_toolchain_binary_var(CMAKE_C_COMPILER "${LLVM_TOOLCHAIN_PREFIX}/bin/clang") | ||
| set_toolchain_binary_var(CMAKE_CXX_COMPILER "${LLVM_TOOLCHAIN_PREFIX}/bin/clang++") | ||
| set_toolchain_binary_var(CMAKE_AR "${LLVM_TOOLCHAIN_PREFIX}/bin/llvm-ar") | ||
| set_toolchain_binary_var(CMAKE_RANLIB "${LLVM_TOOLCHAIN_PREFIX}/bin/llvm-ranlib") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # This file contains utility functions for setting up toolchains and validating toolchain versions | ||
| # to ensure compatibility with the C++20 features required by the project. | ||
|
|
||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
||
| # Sets up the appropriate toolchain file based on the host system. | ||
| function(setup_toolchains) | ||
| # For macOS versions below 15, use the LLVM 15 Clang toolchain. | ||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
||
| if("Darwin" STREQUAL "${CMAKE_HOST_SYSTEM_NAME}") | ||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
||
| execute_process( | ||
| COMMAND | ||
| "sw_vers" "--productVersion" | ||
| OUTPUT_VARIABLE MACOS_VERSION | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
| if("${MACOS_VERSION}" VERSION_LESS "14") | ||
| set(CMAKE_TOOLCHAIN_FILE | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Toolchains/llvm-clang-15-toolchain.cmake" | ||
| CACHE FILEPATH | ||
| "Toolchain file" | ||
| ) | ||
| endif() | ||
| endif() | ||
| endfunction() | ||
|
|
||
| # Checks if the compiler ID and version meet the minimum requirements to support C++20 features | ||
| # required by the project: | ||
| # - AppleClang: version 15+ | ||
| # - Clang: version 15+ | ||
| # - GNU: version 11+ | ||
|
Bill-hbrhbr marked this conversation as resolved.
Outdated
|
||
| function(validate_compiler_versions) | ||
| if("AppleClang" STREQUAL "${CMAKE_CXX_COMPILER_ID}") | ||
| set(CXX_COMPILER_MIN_VERSION "15") | ||
| elseif("Clang" STREQUAL "${CMAKE_CXX_COMPILER_ID}") | ||
| set(CXX_COMPILER_MIN_VERSION "15") | ||
| elseif("GNU" STREQUAL "${CMAKE_CXX_COMPILER_ID}") | ||
| set(CXX_COMPILER_MIN_VERSION "11") | ||
| else() | ||
| message( | ||
| FATAL_ERROR | ||
| "Unsupported compiler: ${CMAKE_CXX_COMPILER_ID}. Please use AppleClang, Clang, or GNU." | ||
| ) | ||
| endif() | ||
|
Bill-hbrhbr marked this conversation as resolved.
|
||
| if("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS "${CXX_COMPILER_MIN_VERSION}") | ||
| message( | ||
| FATAL_ERROR | ||
| "${CMAKE_CXX_COMPILER_ID} version ${CMAKE_CXX_COMPILER_VERSION} is too low. Must be at" | ||
| " least ${CXX_COMPILER_MIN_VERSION}." | ||
| ) | ||
|
Bill-hbrhbr marked this conversation as resolved.
|
||
| endif() | ||
| endfunction() | ||
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.