Skip to content

Conversation

@HTRamsey
Copy link
Collaborator

@HTRamsey HTRamsey commented Dec 9, 2025

Summary

Creates a single source of truth for build toolchain versions in .github/build-config.json, eliminating hardcoded versions across CI workflows, setup scripts, CMake, and Docker files.

New Files

  • .github/build-config.json - Centralized version configuration
  • .github/actions/build-config/action.yml - GitHub Action to read config
  • cmake/BuildConfig.cmake - CMake module to parse JSON config
  • tools/setup/read-config.sh - Bash helper (jq + Python fallback)
  • tools/setup/read-config.ps1 - PowerShell helper

Updated to Use Centralized Config

  • CMake: CustomOptions, FindGStreamer, Android platform, gstqml6gl
  • Docker: Dockerfile-build-ubuntu, Dockerfile-build-android
  • Deploy: appimagecraft.yml, Vagrantfile, .vagrantconfig.yml
  • Scripts: install-dependencies-, install-qt-

Key Design Decisions

  • No fallback defaults - Config is authoritative; scripts fail if unavailable
  • Dual-mode shell helpers - Source for all vars or --get <key> for single value
  • jq with Python fallback - Maximum portability
  • NDK dual format - Both r27c and 27.2.12829759 for different tools

Configuration Keys

{
  "qt_version": "6.10.1",
  "qt_minimum_version": "6.10.0",
  "qt_modules": "qtcharts qtlocation ...",
  "gstreamer_version": "1.24.13",
  "gstreamer_windows_version": "1.22.12",
  "ndk_version": "r27c",
  "ndk_full_version": "27.2.12829759",
  "java_version": "17",
  "android_platform": "35",
  "android_min_sdk": "28",
  "android_build_tools": "35.0.0",
  "android_cmdline_tools": "13114758",
  "xcode_version": "16.x",
  "xcode_ios_version": "latest-stable",
  "cmake_minimum_version": "3.25",
  "ccache_version": "4.11.3",
  "ccache_max_size": "2G",
  "clang_format_version": "17",
  "node_version": "22",
  "flatpak_gnome_version": "47"
}

Test Plan

  • Linux workflow builds successfully
  • Windows workflow builds successfully
  • macOS workflow builds successfully
  • Android Docker build works
  • tools/setup/read-config.sh returns correct values

Copilot AI review requested due to automatic review settings December 9, 2025 04:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a centralized build configuration system to eliminate hardcoded toolchain versions across QGroundControl's build infrastructure. By creating .github/build-config.json as a single source of truth, the changes reduce maintenance burden and version synchronization issues across CI workflows, Docker builds, setup scripts, and CMake configuration.

Key Changes:

  • Centralized version configuration in JSON format with parsers for CMake, Bash, PowerShell, and GitHub Actions
  • Updated all build scripts and Docker files to source versions from the central config
  • Removed hardcoded version strings from 15+ files across the codebase

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
.github/build-config.json Central configuration defining Qt 6.10.1, GStreamer 1.24.13, NDK r27c, and other toolchain versions
.github/actions/build-config/action.yml GitHub Action to parse config and expose values as workflow outputs
cmake/BuildConfig.cmake CMake module using regex to extract values from JSON config
cmake/CustomOptions.cmake Updated to use centralized Qt version variables
cmake/find-modules/FindGStreamer.cmake Updated default GStreamer version from config
cmake/platform/Android.cmake Enhanced NDK version validation with conversion from rXXy to XX.Y format
src/VideoManager/VideoReceiver/GStreamer/gstqml6gl/CMakeLists.txt macOS GStreamer version now from centralized config
tools/setup/read-config.sh Bash helper with jq/Python fallback for config parsing
tools/setup/read-config.ps1 PowerShell helper for Windows config parsing
tools/setup/install-qt-windows.ps1 Sources Qt version from config, removed hardcoded 6.10.1
tools/setup/install-qt-macos.sh Sources Qt version from config, removed hardcoded defaults
tools/setup/install-qt-debian.sh Sources Qt version from config for Linux builds
tools/setup/install-dependencies-windows.ps1 GStreamer Windows version from config
tools/setup/install-dependencies-osx.sh GStreamer version from config for macOS
deploy/docker/Dockerfile-build-ubuntu Dynamic Qt version extraction from config at build time
deploy/docker/Dockerfile-build-android All Android/Java/NDK versions sourced from config
deploy/vagrant/Vagrantfile Reads Qt version and modules from centralized config
deploy/vagrant/.vagrantconfig.yml Removed hardcoded Qt version paths
deploy/linux/appimagecraft.yml Qt path now uses environment variable from config


# Extract value from JSON (simple regex for flat JSON with string values)
macro(qgc_config_get_value VAR_NAME JSON_KEY)
string(REGEX MATCH "\"${JSON_KEY}\"[ \t\n]*:[ \t\n]*\"([^\"]*)\"" _match "${QGC_BUILD_CONFIG_CONTENT}")
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The regex pattern only matches string values in the JSON (\"([^\"]*)\") and will fail for numeric values without quotes. While the current build-config.json has all values as strings, this is fragile. Fields like java_version, android_platform, android_min_sdk, and node_version represent numbers and could legitimately be unquoted in JSON.

Consider either:

  1. Adding support for unquoted numeric values: \"${JSON_KEY}\"[ \t\n]*:[ \t\n]*\"?([^,\"}]*)\"?
  2. Documenting that all config values must be quoted strings
  3. Using CMake's file(READ ... CONFIGURE_DEPENDS) with a proper JSON parser if available in the minimum CMake version
Suggested change
string(REGEX MATCH "\"${JSON_KEY}\"[ \t\n]*:[ \t\n]*\"([^\"]*)\"" _match "${QGC_BUILD_CONFIG_CONTENT}")
string(REGEX MATCH "\"${JSON_KEY}\"[ \t\n]*:[ \t\n]*\"?([^\",}]*)\"?" _match "${QGC_BUILD_CONFIG_CONTENT}")

Copilot uses AI. Check for mistakes.
@HTRamsey HTRamsey force-pushed the dev-build-config branch 6 times, most recently from 897ac59 to 6cf9b5c Compare December 9, 2025 14:10
@HTRamsey
Copy link
Collaborator Author

HTRamsey commented Dec 9, 2025

@DonLakeFlyer One source of truth for config values with this

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.

Comment on lines 70 to 77
echo "Available keys:" >&2
echo " qt_version, qt_modules, qt_minimum_version," >&2
echo " gstreamer_version, gstreamer_windows_version," >&2
echo " ndk_version, ndk_full_version, java_version," >&2
echo " android_platform, android_min_sdk, android_build_tools, android_cmdline_tools," >&2
echo " xcode_version, xcode_ios_version, ccache_version, ccache_max_size," >&2
echo " clang_format_version, node_version, flatpak_gnome_version" >&2
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The --get help message doesn't list gstreamer_android_version as an available key (line 72), but it's defined in .github/build-config.json line 7 and used by CMake. Consider adding it to the help message for completeness, or document why it's omitted if it's not meant to be retrieved via this script.

Copilot uses AI. Check for mistakes.
Comment on lines 83 to 102
# Export all configuration variables (only if not already set)
export QT_VERSION="${QT_VERSION:-$(get_config_value qt_version "$CONFIG_FILE")}"
export QT_MODULES="${QT_MODULES:-$(get_config_value qt_modules "$CONFIG_FILE")}"
export GSTREAMER_VERSION="${GSTREAMER_VERSION:-$(get_config_value gstreamer_version "$CONFIG_FILE")}"
export GSTREAMER_WINDOWS_VERSION="${GSTREAMER_WINDOWS_VERSION:-$(get_config_value gstreamer_windows_version "$CONFIG_FILE")}"
export XCODE_VERSION="${XCODE_VERSION:-$(get_config_value xcode_version "$CONFIG_FILE")}"
export NDK_VERSION="${NDK_VERSION:-$(get_config_value ndk_version "$CONFIG_FILE")}"
export NDK_FULL_VERSION="${NDK_FULL_VERSION:-$(get_config_value ndk_full_version "$CONFIG_FILE")}"
export JAVA_VERSION="${JAVA_VERSION:-$(get_config_value java_version "$CONFIG_FILE")}"
export ANDROID_PLATFORM="${ANDROID_PLATFORM:-$(get_config_value android_platform "$CONFIG_FILE")}"
export ANDROID_MIN_SDK="${ANDROID_MIN_SDK:-$(get_config_value android_min_sdk "$CONFIG_FILE")}"
export CCACHE_VERSION="${CCACHE_VERSION:-$(get_config_value ccache_version "$CONFIG_FILE")}"
export CCACHE_MAX_SIZE="${CCACHE_MAX_SIZE:-$(get_config_value ccache_max_size "$CONFIG_FILE")}"
export CLANG_FORMAT_VERSION="${CLANG_FORMAT_VERSION:-$(get_config_value clang_format_version "$CONFIG_FILE")}"
export NODE_VERSION="${NODE_VERSION:-$(get_config_value node_version "$CONFIG_FILE")}"
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The bash script is missing exports for QT_MINIMUM_VERSION, ANDROID_BUILD_TOOLS, and ANDROID_CMDLINE_TOOLS that are present in the PowerShell version (read-config.ps1 lines 73, 81-82). For consistency between platforms, these variables should also be exported in the bash script.

Copilot uses AI. Check for mistakes.
Comment on lines 100 to 125
if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then
echo "Build Configuration (from $CONFIG_FILE):"
echo " QT_VERSION=$QT_VERSION"
echo " QT_MODULES=$QT_MODULES"
echo " GSTREAMER_VERSION=$GSTREAMER_VERSION"
echo " GSTREAMER_WINDOWS_VERSION=$GSTREAMER_WINDOWS_VERSION"
echo " XCODE_VERSION=$XCODE_VERSION"
echo " NDK_VERSION=$NDK_VERSION"
echo " NDK_FULL_VERSION=$NDK_FULL_VERSION"
echo " JAVA_VERSION=$JAVA_VERSION"
echo " ANDROID_PLATFORM=$ANDROID_PLATFORM"
echo " ANDROID_MIN_SDK=$ANDROID_MIN_SDK"
echo " CCACHE_VERSION=$CCACHE_VERSION"
echo " CCACHE_MAX_SIZE=$CCACHE_MAX_SIZE"
echo " CLANG_FORMAT_VERSION=$CLANG_FORMAT_VERSION"
echo " NODE_VERSION=$NODE_VERSION"
fi
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The print output is missing QT_MINIMUM_VERSION, ANDROID_BUILD_TOOLS, and ANDROID_CMDLINE_TOOLS that are shown in the PowerShell version (read-config.ps1 lines 111-122). The output should be consistent across platforms.

Copilot uses AI. Check for mistakes.
Comment on lines 4 to 70
outputs:
qt_version:
description: Qt version
value: ${{ steps.read.outputs.qt_version }}
qt_minimum_version:
description: Qt minimum supported version
value: ${{ steps.read.outputs.qt_minimum_version }}
qt_modules:
description: Qt modules (desktop)
value: ${{ steps.read.outputs.qt_modules }}
qt_modules_ios:
description: Qt modules (iOS, excludes qtserialport/qtscxml)
value: ${{ steps.read.outputs.qt_modules_ios }}
gstreamer_version:
description: GStreamer version (macOS/Linux)
value: ${{ steps.read.outputs.gstreamer_version }}
gstreamer_windows_version:
description: GStreamer version (Windows)
value: ${{ steps.read.outputs.gstreamer_windows_version }}
ndk_version:
description: Android NDK version (short form)
value: ${{ steps.read.outputs.ndk_version }}
ndk_full_version:
description: Android NDK version (full form for sdkmanager)
value: ${{ steps.read.outputs.ndk_full_version }}
java_version:
description: Java version
value: ${{ steps.read.outputs.java_version }}
android_platform:
description: Android platform/API level
value: ${{ steps.read.outputs.android_platform }}
android_min_sdk:
description: Android minimum SDK version
value: ${{ steps.read.outputs.android_min_sdk }}
android_build_tools:
description: Android build tools version
value: ${{ steps.read.outputs.android_build_tools }}
android_cmdline_tools:
description: Android command line tools version
value: ${{ steps.read.outputs.android_cmdline_tools }}
xcode_version:
description: Xcode version (macOS)
value: ${{ steps.read.outputs.xcode_version }}
xcode_ios_version:
description: Xcode version (iOS)
value: ${{ steps.read.outputs.xcode_ios_version }}
cmake_minimum_version:
description: CMake minimum version
value: ${{ steps.read.outputs.cmake_minimum_version }}
ccache_version:
description: ccache version
value: ${{ steps.read.outputs.ccache_version }}
ccache_max_size:
description: ccache max cache size
value: ${{ steps.read.outputs.ccache_max_size }}
clang_format_version:
description: clang-format version
value: ${{ steps.read.outputs.clang_format_version }}
node_version:
description: Node.js version
value: ${{ steps.read.outputs.node_version }}
flatpak_gnome_version:
description: GNOME version for Flatpak
value: ${{ steps.read.outputs.flatpak_gnome_version }}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The GitHub Action doesn't expose gstreamer_android_version as an output, but it's used by CMake (cmake/BuildConfig.cmake line 28 and cmake/find-modules/FindGStreamer.cmake line 20). Consider adding this as an output for consistency with other platform-specific GStreamer versions.

Copilot uses AI. Check for mistakes.
Create single source of truth for build toolchain versions in
.github/build-config.json, eliminating hardcoded versions across
CI workflows, setup scripts, CMake, and Docker files.

New files:
- .github/build-config.json - Centralized version configuration
- .github/actions/build-config/action.yml - GitHub Action to read config
- cmake/BuildConfig.cmake - CMake module to parse JSON config
- tools/setup/read-config.sh - Bash helper (jq + Python fallback)
- tools/setup/read-config.ps1 - PowerShell helper

Updated to use centralized config:
- CMake: CustomOptions, FindGStreamer, Android platform, gstqml6gl
- Docker: Dockerfile-build-ubuntu, Dockerfile-build-android
- Deploy: appimagecraft.yml, Vagrantfile, .vagrantconfig.yml
- Scripts: install-dependencies-*, install-qt-*

Key improvements:
- No hardcoded fallback defaults in action.yml (config is authoritative)
- Added ndk_full_version for simplified Docker builds
- Simplified Dockerfile NDK version handling
- Version-agnostic comments throughout
@HTRamsey HTRamsey merged commit 4077f49 into mavlink:master Dec 9, 2025
13 checks passed
@HTRamsey HTRamsey deleted the dev-build-config branch December 9, 2025 21:28
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