Modernize CMake build system with modular structure and toolchain files#48
Modernize CMake build system with modular structure and toolchain files#48
Conversation
Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>
Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR modernizes the CMake build system by introducing a modular structure with separate files for compiler options, dependencies, platform checks, and definitions. It adds comprehensive cross-compilation toolchain files for Windows (MXE), macOS (osxcross), Linux ARM64/ARMv7, and musl-based static builds, along with CMakePresets.json for common build configurations.
Changes:
- Introduced modular CMake structure with
cmake/DCMA*.cmakefiles providing reusable functions for compiler options, dependency finding (with FetchContent fallbacks), platform detection, and feature definitions - Added four cross-compilation toolchain files and CMakePresets.json with debug, release, minimal, full, sanitizer, and cross-compile configurations
- Updated root CMakeLists.txt to use DCMA_-prefixed options with backward compatibility for legacy names, changed dependency finding from REQUIRED to QUIET with graceful fallback, and added a dependency configuration summary
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakePresets.json | Adds preset configurations for debug, release, minimal, full, sanitizers, static, and cross-compilation builds |
| cmake/DCMACompilerOptions.cmake | Provides target-based compiler flag functions with GCC/Clang support and sanitizer configurations |
| cmake/DCMADependencies.cmake | Implements dependency finding with FetchContent fallbacks for Eigen and NLopt |
| cmake/DCMAPlatformChecks.cmake | Handles ARM detection, stdlib feature checks, CGAL compatibility, and Windows configuration |
| cmake/DCMADefines.cmake | Manages feature compile definitions and version extraction |
| cmake/toolchains/mxe-x86_64-w64-mingw32.cmake | Windows cross-compilation via MXE toolchain |
| cmake/toolchains/macos-x86_64.cmake | macOS cross-compilation via osxcross |
| cmake/toolchains/linux-musl.cmake | musl libc static build configuration |
| cmake/toolchains/linux-aarch64.cmake | ARM64/ARMv7 cross-compilation setup |
| CMakeLists.txt | Adds DCMA_-prefixed options, backward compatibility mapping, graceful dependency fallback, and configuration summary |
cmake/DCMACompilerOptions.cmake
Outdated
| -pie | ||
| -fsanitize-memory-track-origins | ||
| ) | ||
| target_link_options(${target} PRIVATE -fsanitize=memory) |
There was a problem hiding this comment.
The -pie flag in line 110 is a linker flag, not a compiler flag. It should be passed via target_link_options instead of target_compile_options. The -fPIE flag is correct for compile options, but -pie should be moved to the link options in line 113 along with -fsanitize=memory.
| -pie | |
| -fsanitize-memory-track-origins | |
| ) | |
| target_link_options(${target} PRIVATE -fsanitize=memory) | |
| -fsanitize-memory-track-origins | |
| ) | |
| target_link_options(${target} PRIVATE | |
| -fsanitize=memory | |
| -pie | |
| ) |
| set(CMAKE_EXE_LINKER_FLAGS "-static") | ||
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build shared libraries" FORCE) |
There was a problem hiding this comment.
The linker flag -static is being set globally in CMAKE_EXE_LINKER_FLAGS on line 48, but this may not propagate correctly to all targets. Consider using add_link_options(-static) instead or documenting that users may need to set additional link options for libraries. Additionally, the CACHE FORCE approach on line 49 prevents users from overriding this setting.
| set(CMAKE_EXE_LINKER_FLAGS "-static") | |
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build shared libraries" FORCE) | |
| # Prefer static linking for executables built with this static MXE toolchain; | |
| # use add_link_options so that the flag propagates correctly to targets. | |
| add_link_options(-static) | |
| # Default to building static libraries, but allow users to override | |
| # BUILD_SHARED_LIBS from the command line or presets. | |
| if(NOT DEFINED BUILD_SHARED_LIBS) | |
| set(BUILD_SHARED_LIBS OFF CACHE BOOL "Build shared libraries") | |
| endif() |
cmake/toolchains/linux-musl.cmake
Outdated
| # Flags for musl compatibility | ||
| set(CMAKE_C_FLAGS_INIT "-static") | ||
| set(CMAKE_CXX_FLAGS_INIT "-static") | ||
| set(CMAKE_EXE_LINKER_FLAGS_INIT "-static") |
There was a problem hiding this comment.
The static linking flags are set via CMAKE_*_FLAGS_INIT variables (lines 67-69), which are initialization flags. While this is a valid approach, these flags may not apply to all link scenarios (e.g., shared libraries if BUILD_SHARED_LIBS is later changed). Consider using add_link_options(-static) or document that this toolchain is strictly for static executables only.
| set(CMAKE_EXE_LINKER_FLAGS_INIT "-static") | |
| add_link_options(-static) |
| function(dcma_run_all_platform_checks) | ||
| dcma_detect_arm_architecture() | ||
| dcma_check_cppstdlib_features() | ||
| dcma_check_system_features() |
There was a problem hiding this comment.
The function dcma_check_cgal_compatibility() is defined but not called in dcma_run_all_platform_checks() on line 126. If this function is intended to be part of the platform checks workflow, it should be added to the function. If it's intended to be called separately, consider documenting this or renaming the umbrella function to clarify its scope.
| dcma_check_system_features() | |
| dcma_check_system_features() | |
| dcma_check_cgal_compatibility() |
CMakePresets.json
Outdated
| "version": 6, | ||
| "cmakeMinimumRequired": { | ||
| "major": 3, | ||
| "minor": 21, |
There was a problem hiding this comment.
The CMakePresets.json file specifies cmakeMinimumRequired as 3.21.0, but the root CMakeLists.txt file requires CMake 3.12.0. This creates an inconsistency where presets require a newer CMake version than the project itself. Consider updating CMakeLists.txt to require CMake 3.21 if presets are intended to be the primary configuration method, or lower the preset requirement to match the project minimum.
| "minor": 21, | |
| "minor": 12, |
CMakeLists.txt
Outdated
| if(DEFINED WITH_SFML AND NOT DEFINED DCMA_WITH_SFML_SET) | ||
| set(DCMA_WITH_SFML ${WITH_SFML}) | ||
| endif() | ||
| if(DEFINED WITH_SDL AND NOT DEFINED DCMA_WITH_SDL_SET) | ||
| set(DCMA_WITH_SDL ${WITH_SDL}) | ||
| endif() | ||
| if(DEFINED WITH_WT AND NOT DEFINED DCMA_WITH_WT_SET) | ||
| set(DCMA_WITH_WT ${WITH_WT}) | ||
| endif() | ||
| if(DEFINED WITH_GNU_GSL AND NOT DEFINED DCMA_WITH_GNU_GSL_SET) | ||
| set(DCMA_WITH_GNU_GSL ${WITH_GNU_GSL}) | ||
| endif() | ||
| if(DEFINED WITH_POSTGRES AND NOT DEFINED DCMA_WITH_POSTGRES_SET) | ||
| set(DCMA_WITH_POSTGRES ${WITH_POSTGRES}) | ||
| endif() | ||
| if(DEFINED WITH_JANSSON AND NOT DEFINED DCMA_WITH_JANSSON_SET) | ||
| set(DCMA_WITH_JANSSON ${WITH_JANSSON}) | ||
| endif() | ||
| if(DEFINED WITH_THRIFT AND NOT DEFINED DCMA_WITH_THRIFT_SET) | ||
| set(DCMA_WITH_THRIFT ${WITH_THRIFT}) | ||
| endif() | ||
| if(DEFINED WITH_LTO AND NOT DEFINED DCMA_WITH_LTO_SET) |
There was a problem hiding this comment.
The backward compatibility mechanism is incomplete. The code checks for variables like DCMA_MEMORY_CONSTRAINED_BUILD_SET, DCMA_WITH_IWYU_SET, etc., but these variables are never set anywhere in the code. This means that if a user provides both the legacy option (e.g., WITH_EIGEN) and the new option (e.g., DCMA_WITH_EIGEN), the legacy option will always override the new one, which is likely not the intended behavior. The condition should either remove the _SET check, or the code should set these sentinel variables when the DCMA_ prefixed options are explicitly provided.
| if(DEFINED WITH_SFML AND NOT DEFINED DCMA_WITH_SFML_SET) | |
| set(DCMA_WITH_SFML ${WITH_SFML}) | |
| endif() | |
| if(DEFINED WITH_SDL AND NOT DEFINED DCMA_WITH_SDL_SET) | |
| set(DCMA_WITH_SDL ${WITH_SDL}) | |
| endif() | |
| if(DEFINED WITH_WT AND NOT DEFINED DCMA_WITH_WT_SET) | |
| set(DCMA_WITH_WT ${WITH_WT}) | |
| endif() | |
| if(DEFINED WITH_GNU_GSL AND NOT DEFINED DCMA_WITH_GNU_GSL_SET) | |
| set(DCMA_WITH_GNU_GSL ${WITH_GNU_GSL}) | |
| endif() | |
| if(DEFINED WITH_POSTGRES AND NOT DEFINED DCMA_WITH_POSTGRES_SET) | |
| set(DCMA_WITH_POSTGRES ${WITH_POSTGRES}) | |
| endif() | |
| if(DEFINED WITH_JANSSON AND NOT DEFINED DCMA_WITH_JANSSON_SET) | |
| set(DCMA_WITH_JANSSON ${WITH_JANSSON}) | |
| endif() | |
| if(DEFINED WITH_THRIFT AND NOT DEFINED DCMA_WITH_THRIFT_SET) | |
| set(DCMA_WITH_THRIFT ${WITH_THRIFT}) | |
| endif() | |
| if(DEFINED WITH_LTO AND NOT DEFINED DCMA_WITH_LTO_SET) | |
| if(DEFINED WITH_SFML) | |
| set(DCMA_WITH_SFML ${WITH_SFML}) | |
| endif() | |
| if(DEFINED WITH_SDL) | |
| set(DCMA_WITH_SDL ${WITH_SDL}) | |
| endif() | |
| if(DEFINED WITH_WT) | |
| set(DCMA_WITH_WT ${WITH_WT}) | |
| endif() | |
| if(DEFINED WITH_GNU_GSL) | |
| set(DCMA_WITH_GNU_GSL ${WITH_GNU_GSL}) | |
| endif() | |
| if(DEFINED WITH_POSTGRES) | |
| set(DCMA_WITH_POSTGRES ${WITH_POSTGRES}) | |
| endif() | |
| if(DEFINED WITH_JANSSON) | |
| set(DCMA_WITH_JANSSON ${WITH_JANSSON}) | |
| endif() | |
| if(DEFINED WITH_THRIFT) | |
| set(DCMA_WITH_THRIFT ${WITH_THRIFT}) | |
| endif() | |
| if(DEFINED WITH_LTO) |
CMakeLists.txt
Outdated
| option(WITH_LTO "Use link-time optimization when available." OFF) | ||
| option(DCMA_WITH_LTO "Use link-time optimization when available." OFF) | ||
|
|
||
| option(BUILD_SHARED_LIBS "Build shared-object/dynamicly-loaded binaries." ON) |
There was a problem hiding this comment.
The word "dynamicly" is misspelled and should be "dynamically".
| option(BUILD_SHARED_LIBS "Build shared-object/dynamicly-loaded binaries." ON) | |
| option(BUILD_SHARED_LIBS "Build shared-object/dynamically-loaded binaries." ON) |
cmake/DCMACompilerOptions.cmake
Outdated
| target_link_libraries(${target} PRIVATE gcov) | ||
| endif() | ||
|
|
||
| target_compile_definitions(${target} PRIVATE -U_FORTIFY_SOURCE) |
There was a problem hiding this comment.
The -U_FORTIFY_SOURCE flag is being passed to target_compile_definitions but this is a compiler flag, not a definition. It should be passed to target_compile_options instead. The correct approach would be to use target_compile_options(${target} PRIVATE -U_FORTIFY_SOURCE) or target_compile_definitions(${target} PRIVATE _FORTIFY_SOURCE=0) depending on the intent.
| target_compile_definitions(${target} PRIVATE -U_FORTIFY_SOURCE) | |
| target_compile_options(${target} PRIVATE -U_FORTIFY_SOURCE) |
|
@copilot apply changes based on the comments in this thread |
…d version consistency Co-authored-by: hdclark <934858+hdclark@users.noreply.github.com>
Applied all 8 review comments in commit ae95c39:
|
-piefrom compile options to link options (MSan)add_link_options(-static)in MXE toolchainadd_link_options(-static)in musl toolchain_SETchecks-U_FORTIFY_SOURCEto use compile_options instead of compile_definitionsSummary
This PR modernizes the DICOMautomaton CMake build system by:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.