Skip to content

CMake changes for NixOS #486

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.

Sign up for GitHub

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

Open
christofmuc wants to merge 5 commits into
base: master
Choose a base branch
from
Open

CMake changes for NixOS #486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
40ed7c3
Extract the version calculation from git from the main CMake file, so…
christofmuc Nov 1, 2025
da9d27f
Fix syntax errors
christofmuc Nov 1, 2025
305a0ae
Using the wrong editor for a Makefile can make you look stupid
christofmuc Nov 1, 2025
c294a1c
Fix version calculation script
christofmuc Nov 1, 2025
ca7d0ef
More parsing goodies?
christofmuc Nov 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/builds-ubuntu.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ jobs:
SENTRY_DSN: ${{ secrets.SENTRY_DSN }}
shell: bash
run: |
cmake -S . -B builds -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=off -DPYTHON_VERSION_TO_EMBED=3.12 -DSENTRY_CRASH_REPORTING=ON -DSENTRY_DSN=$SENTRY_DSN
export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)"
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

- name: CMake build
run: cmake --build builds --target package -- -j4
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/builds-ubuntu24.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ jobs:
SENTRY_DSN: ${{ secrets.SENTRY_DSN }}
shell: bash
run: |
CC=gcc-14 CXX=g++-14 cmake -S . -B builds -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=off -DPYTHON_VERSION_TO_EMBED=3.12 -DSENTRY_CRASH_REPORTING=ON -DSENTRY_DSN=$SENTRY_DSN
export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)"
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

- name: CMake build
run: |
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/builds-windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ jobs:
SENTRY_DSN: ${{ secrets.SENTRY_DSN }}
shell: bash
run: |
cmake -S . -B Builds -G "Visual Studio 17 2022" -A x64 -DCRASH_REPORTING=ON -DSENTRY_DSN=$SENTRY_DSN -DSPARKLE_UPDATES=ON
export KNOBKRAFT_EXTERNAL_VERSION="$(cmake -P cmake/calc_version.cmake)"
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

- name: Run Python tests first
run: |
Expand Down
6 changes: 6 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ option(ASAN "Use Address Sanitization for Debug version (Windows only for now)"
# Gin requests C++ 20.
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
if(UNIX AND NOT APPLE)
# Linux settings allowing NixOs to be built
set(CMAKE_AR "gcc-ar")
set(CMAKE_CXX_ARCHIVE_CREATE "<CMAKE_AR> qcs <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_CXX_ARCHIVE_FINISH true
ENDIF()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)

# To avoid dependency on WebKit. This also came with MacOS, but as webkit is heavyweight, it is probably a good idea to turn it off for all
Expand Down
5 changes: 3 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ all: configure build sign-dmg verify-signed
apple: notarize staple verify-notarization

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)"
Comment on lines 36 to +39

@coderabbitai coderabbitai Bot Nov 1, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.

@christofmuc christofmuc Nov 1, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I fix it?

@coderabbitai coderabbitai Bot Nov 1, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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.

⚠️ However, there's a functional problem with the 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:

  1. 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)"
  1. 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.


.PHONY: build
build $(KNOBKRAFT_DMG):
Expand Down
13 changes: 1 addition & 12 deletions The-Orm/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,8 @@

cmake_minimum_required(VERSION 3.14)

# Get the version from our sub cmakefile
execute_process(
COMMAND ${CMAKE_COMMAND} -P "${CMAKE_CURRENT_LIST_DIR}/gitversion.cmake"
ERROR_VARIABLE PROJECT_VERSION
ERROR_STRIP_TRAILING_WHITESPACE
)
# Cleanup output
string(REGEX REPLACE "^[[:space:]]+|[[:space:]]+$" "" PROJECT_VERSION "${PROJECT_VERSION}")
string(REGEX REPLACE "\n$" "" PROJECT_VERSION "${PROJECT_VERSION}")
message(STATUS "Project version is '${PROJECT_VERSION}'")

# Start project
project(KnobKraftOrm VERSION ${PROJECT_VERSION})
project(KnobKraftOrm VERSION ${KNOBKRAFT_EXTERNAL_VERSION})

@coderabbitai coderabbitai Bot Nov 1, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


# Now you can use PROJECT_DEV_TAG in your CMake scripts or in your code to handle the "-dev" part.
# For example, you could add a preprocessor definition that your code could use:
Expand Down
12 changes: 12 additions & 0 deletions cmake/calc_version.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Get the version from our sub cmakefile
execute_process(
COMMAND ${CMAKE_COMMAND} -P "${CMAKE_CURRENT_LIST_DIR}/gitversion.cmake"
ERROR_VARIABLE PROJECT_VERSION
ERROR_STRIP_TRAILING_WHITESPACE
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

# Cleanup output
string(REGEX REPLACE "^[[:space:]]+|[[:space:]]+$" "" PROJECT_VERSION "${PROJECT_VERSION}")
string(REGEX REPLACE "\n$" "" PROJECT_VERSION "${PROJECT_VERSION}")
message("${PROJECT_VERSION}")

0 The-Orm/gitversion.cmake → cmake/gitversion.cmake
View file
Open in desktop
File renamed without changes.
Loading