CMake: General Improvements & Documentation #13509
Conversation
HTRamsey
force-pushed
the
dev-cmake-fixes
branch
2 times, most recently
from
5e42970 to
2bba793
Compare
HTRamsey
changed the title
HTRamsey
force-pushed
the
dev-cmake-fixes
branch
2 times, most recently
from
7c30006 to
da3e1e0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR significantly improves the CMake build system for QGroundControl through comprehensive documentation, enhanced configuration organization, and better maintainability. The primary goal is to make the build system more accessible to developers and improve long-term maintenance.
Key changes include:
- Addition of structured documentation headers and comments throughout all CMake files
- Reorganization of configuration options with clearer grouping and descriptions
- Enhanced error handling and validation in build scripts
Reviewed Changes
Copilot reviewed 98 out of 98 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/setup/install-dependencies-debian.sh | Added libpipewire-0.3-dev dependency for audio support |
| CMakeLists.txt | Restructured main build file with comprehensive documentation sections |
| cmake/*.cmake | Enhanced helper functions, Git integration, and toolchain configuration |
| src/*/CMakeLists.txt | Added module headers documenting purpose and organization |
| test/*/CMakeLists.txt | Documented test modules and improved resource organization |
| custom-example/*.cmake | Better structured custom build configuration templates |
Comments suppressed due to low confidence (1)
cmake/install/CreateWinInstaller.cmake:1
- The error message should be more descriptive and indicate what the user needs to do to fix the issue. Consider adding guidance about which CMake variables need to be set.
# ============================================================================
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Extracts version information and metadata from Git repository | ||
| # ---------------------------------------------------------------------------- | ||
|
|
||
| find_package(Git REQUIRED) |
There was a problem hiding this comment.
Changed from find_package(Git) to find_package(Git REQUIRED) but this could break builds on systems without Git. The original optional behavior should be preserved with proper error handling for missing Git.
| find_package(Git REQUIRED) | |
| find_package(Git) |
Copilot uses AI. Check for mistakes.
| file(GLOB GPS_DRIVERS_SOURCES | ||
| CONFIGURE_DEPENDS | ||
| "${px4-gpsdrivers_SOURCE_DIR}/src/*.c" | ||
| "${px4-gpsdrivers_SOURCE_DIR}/src/*.cpp" | ||
| "${px4-gpsdrivers_SOURCE_DIR}/src/*.h" | ||
| ) |
There was a problem hiding this comment.
Using file(GLOB) with CONFIGURE_DEPENDS on external dependency sources could cause unnecessary reconfiguration. Since this is an external dependency, the file list is unlikely to change and should use an explicit list or the original approach.
Copilot uses AI. Check for mistakes.
| if(android_openssl_ADDED) | ||
| include(${android_openssl_SOURCE_DIR}/android_openssl.cmake) | ||
| add_android_openssl_libraries(${CMAKE_PROJECT_NAME}) | ||
| message(STATUS "QGC: Android OpenSSL libraries added") | ||
| else() | ||
| message(WARNING "QGC: Failed to add Android OpenSSL libraries") | ||
| endif() |
There was a problem hiding this comment.
The variable android_openssl_ADDED should be checked as android-openssl_ADDED (with hyphen) to match the CPMAddPackage NAME parameter on line 89.
Copilot uses AI. Check for mistakes.
HTRamsey
force-pushed
the
dev-cmake-fixes
branch
2 times, most recently
from
d6aa70d to
2ea0f71
Compare
HTRamsey
force-pushed
the
dev-cmake-fixes
branch
from
2ea0f71 to
a6e8f2d
Compare
1 participant
Improve CMake and fix issues
Generate Documentation