CMake changes for NixOS#486
Conversation
… the NixOS build can run after determining the version and removing the .git tree.
|
Warning Rate limit exceeded@christofmuc has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCentralizes version computation in Changes
Sequence DiagramsequenceDiagram
participant Runner as CI / Makefile
participant Calc as calc_version.cmake
participant GitVer as gitversion.cmake
participant CMake as CMake configure
participant Project as The-Orm/CMakeLists.txt
Runner->>Calc: run `cmake -P cmake/calc_version.cmake`
Calc->>GitVer: execute_process(`gitversion.cmake`)
GitVer-->>Calc: raw version output / exit code
Calc->>Calc: normalize, validate, fail on error or empty
Calc-->>Runner: echo final version
Runner->>Runner: export KNOBKRAFT_EXTERNAL_VERSION
Runner->>CMake: configure with `-DKNOBKRAFT_EXTERNAL_VERSION=...`
CMake->>Project: project(...) reads `${KNOBKRAFT_EXTERNAL_VERSION}`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
The-Orm/CMakeLists.txt (1)
14-16: Add missing include to source PROJECT_DEV_TAG or extract it locally from KNOBKRAFT_EXTERNAL_VERSION.Lines 14-16 reference
PROJECT_DEV_TAG, but this variable is defined only incmake/gitversion.cmake:18and is never included inThe-Orm/CMakeLists.txt. Add one of:
- Include the gitversion module at the top of The-Orm/CMakeLists.txt:
include(${CMAKE_CURRENT_LIST_DIR}/../cmake/gitversion.cmake)
- Or extract the dev tag locally from
KNOBKRAFT_EXTERNAL_VERSION:string(REGEX MATCH "(-dev)$" PROJECT_DEV_TAG "${KNOBKRAFT_EXTERNAL_VERSION}")Without this,
PROJECT_DEV_TAGwill be undefined when the conditional evaluates, causing theadd_definitions(-DPROJECT_DEV)to never execute as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/builds-ubuntu.yml(1 hunks).github/workflows/builds-ubuntu24.yml(1 hunks).github/workflows/builds-windows.yml(1 hunks)CMakeLists.txt(1 hunks)Makefile(1 hunks)The-Orm/CMakeLists.txt(1 hunks)cmake/calc_version.cmake(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: macOS KnobKraft Orm
Makefile
[error] 38-38: Makefile:38: *** commands commence before first target. Stop.
🪛 GitHub Actions: Ubuntu 22 KnobKraft Orm
CMakeLists.txt
[error] 46-46: CMake configure failed: Parse error. Function missing ending ")". End of file reached. (Process completed with exit code 1.)
The-Orm/CMakeLists.txt
[error] 46-46: CMake configure failed: Parse error. Function missing ending ")". End of file reached. (Process completed with exit code 1.)
🪛 GitHub Actions: Ubuntu 24 KnobKraft Orm
CMakeLists.txt
[error] 46-46: CMake parse error: Function missing ending ')' End of file reached.
[warning] 1-1: CMake Warning: Ignoring extra path from command line: ""2.51.2")"
The-Orm/CMakeLists.txt
[error] 46-46: CMake parse error: Function missing ending ')' End of file reached.
[warning] 1-1: CMake Warning: Ignoring extra path from command line: ""2.51.2")"
⏰ 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). (1)
- GitHub Check: build-windows
🔇 Additional comments (3)
.github/workflows/builds-ubuntu24.yml (1)
42-43: Add error handling for version computation.If
cmake/calc_version.cmakefails or produces unexpected output, the build will continue with an empty or invalid version. Consider adding validation:export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)" +if [ -z "$KNOBKRAFT_EXTERNAL_VERSION" ]; then + echo "Error: Failed to compute version" + exit 1 +fi +echo "Computed version: $KNOBKRAFT_EXTERNAL_VERSION" CC=gcc-14 CXX=g++-14 cmake -S . -B builds -DKNOBKRAFT_EXTERNAL_VERSION=$KNOBKRAFT_EXTERNAL_VERSION -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=off -DPYTHON_VERSION_TO_EMBED=3.12 -DSENTRY_CRASH_REPORTING=ON -DSENTRY_DSN=$SENTRY_DSN.github/workflows/builds-windows.yml (1)
51-52: Add error handling for version computation.Same as in the Ubuntu 24 workflow, add validation to ensure version computation succeeds:
export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)" +if [ -z "$KNOBKRAFT_EXTERNAL_VERSION" ]; then + echo "Error: Failed to compute version" + exit 1 +fi +echo "Computed version: $KNOBKRAFT_EXTERNAL_VERSION" cmake -S . -B Builds -G "Visual Studio 17 2022" -A x64 -DKNOBKRAFT_EXTERNAL_VERSION=$KNOBKRAFT_EXTERNAL_VERSION -DCRASH_REPORTING=ON -DSENTRY_DSN=$SENTRY_DSN -DSPARKLE_UPDATES=ON.github/workflows/builds-ubuntu.yml (1)
34-35: Add error handling for version computation.Consistent with the other workflows, add validation:
export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)" +if [ -z "$KNOBKRAFT_EXTERNAL_VERSION" ]; then + echo "Error: Failed to compute version" + exit 1 +fi +echo "Computed version: $KNOBKRAFT_EXTERNAL_VERSION" cmake -S . -B builds -DKNOBKRAFT_EXTERNAL_VERSION=$KNOBKRAFT_EXTERNAL_VERSION -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=off -DPYTHON_VERSION_TO_EMBED=3.12 -DSENTRY_CRASH_REPORTING=ON -DSENTRY_DSN=$SENTRY_DSN
| configure: | ||
| @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE)" | ||
| cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)" | ||
| export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)" | ||
| @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $KNOBKRAFT_EXTERNAL_VERSION" | ||
| cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$KNOBKRAFT_EXTERNAL_VERSION -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)" |
There was a problem hiding this comment.
Critical: Fix Makefile syntax error.
Line 37 is indented, making it a recipe command that appears before the target's prerequisites, which violates Make syntax. The pipeline error "commands commence before first target" confirms this.
Apply this diff to fix the syntax:
configure:
- export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)"
- @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $KNOBKRAFT_EXTERNAL_VERSION"
- cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$KNOBKRAFT_EXTERNAL_VERSION -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)"
+ $(eval KNOBKRAFT_EXTERNAL_VERSION := $(shell cmake -P cmake/calc_version.cmake))
+ @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $(KNOBKRAFT_EXTERNAL_VERSION)"
+ cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$(KNOBKRAFT_EXTERNAL_VERSION) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| configure: | |
| @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE)" | |
| cmake -S . -B $(BUILD_DIR) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)" | |
| export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)" | |
| @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $KNOBKRAFT_EXTERNAL_VERSION" | |
| cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$KNOBKRAFT_EXTERNAL_VERSION -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)" | |
| configure: | |
| $(eval KNOBKRAFT_EXTERNAL_VERSION := $(shell cmake -P cmake/calc_version.cmake)) | |
| @echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $(KNOBKRAFT_EXTERNAL_VERSION)" | |
| cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$(KNOBKRAFT_EXTERNAL_VERSION) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)" |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 36-36: Target "configure" should be declared PHONY.
(phonydeclared)
🪛 GitHub Actions: macOS KnobKraft Orm
[error] 38-38: Makefile:38: *** commands commence before first target. Stop.
🤖 Prompt for AI Agents
In Makefile around lines 36 to 39, the echo line is incorrectly indented as a
recipe before the target which causes "commands commence before first target";
fix it by removing the leading tab from the echo line so it is part of the
makefile target declaration (or place it after the configure: line as a recipe
with a leading tab), ensuring only recipe lines are tab-indented and no commands
appear before the target header.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the exact indentation characters in the Makefile around the configure target
# Using cat -A to show tabs as ^I and spaces as regular spaces
echo "=== Checking indentation in Makefile around configure target ==="
sed -n '36,39p' Makefile | cat -A
echo ""
echo "=== Checking with od to see exact bytes ==="
sed -n '37p' Makefile | od -c
echo ""
echo "=== Line 37 content ==="
sed -n '37p' Makefile
Length of output: 707
@christofmuc Good news and bad news!
✅ You fixed the indentation issue - all three lines now use tabs correctly, so the "commands commence before first target" error should be gone.
export approach:
In Makefiles, each recipe line runs in its own separate shell. So when line 37 runs export KNOBKRAFT_EXTERNAL_VERSION=..., that variable is only available in that shell instance. Lines 38 and 39 run in different shells and won't see the exported variable.
Two solutions:
- Use the bot's suggested approach (cleanest):
configure:
$(eval KNOBKRAFT_EXTERNAL_VERSION := $(shell cmake -P cmake/calc_version.cmake))
@echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $(KNOBKRAFT_EXTERNAL_VERSION)"
cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$(KNOBKRAFT_EXTERNAL_VERSION) -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)"- Join the commands with semicolons (keeps them in one shell):
configure:
export KNOBKRAFT_EXTERNAL_VERSION="$$(cmake -P cmake/calc_version.cmake)"; \
echo "Configuring build for type $(BUILD_TYPE) in directory $(BUILD_DIR), using Python from $(PYTHON_TO_USE). Version is $$KNOBKRAFT_EXTERNAL_VERSION"; \
cmake -S . -B $(BUILD_DIR) -DKNOBKRAFT_EXTERNAL_VERSION=$$KNOBKRAFT_EXTERNAL_VERSION -DCMAKE_BUILD_TYPE=$(BUILD_TYPE) -DPYTHON_EXECUTABLE=$(PYTHON_TO_USE) -DCODESIGN_CERTIFICATE_NAME="$(APPLE_DEVELOPER_IDENTITY)"Note the $$ (double dollar signs) for shell variables in option 2.
I'd recommend option 1 as it's cleaner and more Makefile-idiomatic.
|
|
||
| # Start project | ||
| project(KnobKraftOrm VERSION ${PROJECT_VERSION}) | ||
| project(KnobKraftOrm VERSION ${KNOBKRAFT_EXTERNAL_VERSION}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for KNOBKRAFT_EXTERNAL_VERSION.
If KNOBKRAFT_EXTERNAL_VERSION is not set or is empty (e.g., due to the calc_version.cmake failures noted in other files), the project() command will fail or produce an invalid version.
+if(NOT DEFINED KNOBKRAFT_EXTERNAL_VERSION OR KNOBKRAFT_EXTERNAL_VERSION STREQUAL "")
+ message(FATAL_ERROR "KNOBKRAFT_EXTERNAL_VERSION must be set. Please pass -DKNOBKRAFT_EXTERNAL_VERSION=<version> to CMake.")
+endif()
+
project(KnobKraftOrm VERSION ${KNOBKRAFT_EXTERNAL_VERSION})🤖 Prompt for AI Agents
In The-Orm/CMakeLists.txt around line 10, the project() call uses
KNOBKRAFT_EXTERNAL_VERSION without validating it; add a check immediately before
the project() line to verify the variable is defined and non-empty and either
set a safe default (e.g., "0.0.0") or call message(FATAL_ERROR ...) with a clear
error asking to run calc_version.cmake so the build fails fast with a helpful
message instead of producing an invalid project() invocation.
1 participant
Summary by CodeRabbit