Skip to content

Dev/cmake#25

Merged
jgeudens merged 3 commits intomasterfrom
dev/cmake
Apr 5, 2026
Merged

Dev/cmake#25
jgeudens merged 3 commits intomasterfrom
dev/cmake

Conversation

@jgeudens
Copy link
Copy Markdown
Member

@jgeudens jgeudens commented Apr 5, 2026

Summary by CodeRabbit

  • Chores
    • Standardised compiler and linker options and tightened platform checks for more consistent builds.
    • Automated and logged build-type resolution with safer default behaviours.
    • Scoped include paths and per-target definitions to reduce global build leakage.
    • Made source discovery reconfigure-aware so changes auto-trigger rebuilds.
    • Updated build and analysis scripts to generate explicit Debug/Release build artefacts for tooling and CI.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 5, 2026

Walkthrough

Refactors CMake build configuration to remove global compiler definitions/flags, adopt target-scoped compile/link options, enforce/announce build-type defaults, update file globbing with CONFIGURE_DEPENDS, and move test and library include/definitions from global scope into per-target scope. Build scripts now pass explicit CMAKE_BUILD_TYPE values.

Changes

Cohort / File(s) Summary
Top-level CMake changes
CMakeLists.txt
Removed global -DDEBUG, removed global include directories, replaced direct CMAKE_CXX_FLAGS/linker mutations with add_compile_options()/add_link_options(), added build-type auto-selection/validation and status message, simplified compiler ID checks, updated gcov handling.
Source target updates
src/CMakeLists.txt
Switched to file(... CONFIGURE_DEPENDS ...) for sources/UI, added conditional private DEBUG definition when CMAKE_BUILD_TYPE is Debug, published target include directories for source and libraries, tightened platform checks (STREQUAL).
Tests and mocks
tests/CMakeLists.txt
Removed global GTEST_LANGUAGE_CXX11 and global include dirs; added per-target private include directories and private GTEST_LANGUAGE_CXX11 compile definition inside add_xtest_mock(...).
Third-party library globbing
libraries/muparser/CMakeLists.txt
Changed FILE(GLOB_RECURSE ...) to include CONFIGURE_DEPENDS so CMake re-runs configuration when muparser source files change.
Build and analysis scripts
scripts/build_linux.sh, scripts/build_windows.bat, scripts/run_clang_tidy.sh, scripts/run_clazy.sh
CMake invocations now pass explicit -DCMAKE_BUILD_TYPE: Release for build scripts, Debug for clang-tidy/clazy compile-command generation.
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Dev/cmake' is vague and uses non-descriptive terminology that doesn't convey meaningful information about the specific changes in the changeset. Replace the generic title with a specific summary of the main changes, such as 'Refactor CMake build configuration to use per-target options and explicit build types' or similar.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev/cmake

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/CMakeLists.txt`:
- Around line 26-28: The CMake check using CMAKE_BUILD_TYPE misses Debug mode on
multi-config generators; replace the CMAKE_BUILD_TYPE conditional with a
config-aware generator expression on the target (${SCOPESOURCE}) so the DEBUG
definition is added only for Debug configurations (e.g., use
target_compile_definitions(${SCOPESOURCE} PRIVATE $<$<CONFIG:Debug>:DEBUG>) or
the equivalent generator-expression form) to ensure DEBUG is defined when the
active configuration is Debug across single- and multi-config generators.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 351f20f9-9b4b-47c3-a6b8-92e6c9d85f35

📥 Commits

Reviewing files that changed from the base of the PR and between 969a5fb and 065eecf.

📒 Files selected for processing (1)
  • src/CMakeLists.txt

Comment on lines +26 to +28
if(CMAKE_BUILD_TYPE STREQUAL "Debug")
target_compile_definitions(${SCOPESOURCE} PRIVATE DEBUG)
endif()
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 | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify config-gating patterns for DEBUG in CMake files.
# Expected: avoid compile-definition gating via CMAKE_BUILD_TYPE;
# prefer generator expressions like $<$<CONFIG:Debug>:DEBUG>.

rg -n --type=cmake -C3 'CMAKE_BUILD_TYPE\s+STREQUAL\s+"Debug"|target_compile_definitions\s*\(.*DEBUG|\$<CONFIG:Debug>'

Repository: ModbusScope/NotOnlyModbusScope

Length of output: 1695


Use config-aware DEBUG definition instead of CMAKE_BUILD_TYPE check.

Line 26 silently misses DEBUG on multi-config generators (notably Windows/Visual Studio), even for Debug builds. CMAKE_BUILD_TYPE is not populated in multi-config generators; use a generator expression tied to the active configuration instead.

Suggested fix
-if(CMAKE_BUILD_TYPE STREQUAL "Debug")
-    target_compile_definitions(${SCOPESOURCE} PRIVATE DEBUG)
-endif()
+target_compile_definitions(${SCOPESOURCE} PRIVATE
+    $<$<CONFIG:Debug>:DEBUG>
+)
📝 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
if(CMAKE_BUILD_TYPE STREQUAL "Debug")
target_compile_definitions(${SCOPESOURCE} PRIVATE DEBUG)
endif()
target_compile_definitions(${SCOPESOURCE} PRIVATE
$<$<CONFIG:Debug>:DEBUG>
)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/CMakeLists.txt` around lines 26 - 28, The CMake check using
CMAKE_BUILD_TYPE misses Debug mode on multi-config generators; replace the
CMAKE_BUILD_TYPE conditional with a config-aware generator expression on the
target (${SCOPESOURCE}) so the DEBUG definition is added only for Debug
configurations (e.g., use target_compile_definitions(${SCOPESOURCE} PRIVATE
$<$<CONFIG:Debug>:DEBUG>) or the equivalent generator-expression form) to ensure
DEBUG is defined when the active configuration is Debug across single- and
multi-config generators.

@jgeudens jgeudens merged commit a6172ee into master Apr 5, 2026
10 checks passed
@jgeudens jgeudens deleted the dev/cmake branch April 5, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant