From 895d0ed410415cc4c99ddb821bdabbf7430540ac Mon Sep 17 00:00:00 2001 From: Raphael Hunziker Date: Wed, 9 Sep 2026 15:47:03 +0200 Subject: [PATCH 01/10] Fix the Parameter Group Version Check so it can run on pull requests The check has failed on every pull request since it was added: - check-pg-versions.sh declares `local companion` in the top-level loop that builds the file list. Outside a function bash rejects `local`, and with `set -e` the script aborts right there with "local: can only be used in a function", before any file is checked. - The workflow then inlines the multi-line script output into the JavaScript source of the github-script step as a single-quoted string literal, which fails to parse ("SyntaxError: Invalid or unexpected token") and fails the job. Drop the `local`, and hand the output to the script through an environment variable instead of the source text. Verified locally against a change that adds a field to telemetryConfig_t: with the PG version bump the script reports "No PG version issues detected" and exits 0, without the bump it reports the struct, the unchanged version and the recommended increment and exits 1. --- .github/scripts/check-pg-versions.sh | 3 ++- .github/workflows/pg-version-check.yml | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/scripts/check-pg-versions.sh b/.github/scripts/check-pg-versions.sh index e07f7538fda..b0d73651de3 100755 --- a/.github/scripts/check-pg-versions.sh +++ b/.github/scripts/check-pg-versions.sh @@ -187,7 +187,8 @@ while IFS= read -r file; do fi # Determine companion file (.c <-> .h) - local companion="" + # (this loop runs at top level, so no "local" here: bash would abort the script) + companion="" if [[ "$file" == *.c ]]; then companion="${file%.c}.h" elif [[ "$file" == *.h ]]; then diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index d9d8c289930..89c71c82224 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -46,10 +46,14 @@ jobs: - name: Post comment if issues found if: steps.pg_check.outputs.exit_code == '1' uses: actions/github-script@v7 + env: + # Passed through the environment: inlining the multi-line script output + # into the JavaScript source breaks the string literal (SyntaxError). + PG_CHECK_OUTPUT: ${{ steps.pg_check.outputs.output }} with: script: | // Use the captured output from the previous step - const output = '${{ steps.pg_check.outputs.output }}'; + const output = process.env.PG_CHECK_OUTPUT || ''; let issuesContent = ''; try { From cef5f36012fcf9310627b8d949d626a7a1dc3981 Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Wed, 9 Sep 2026 18:02:59 +0200 Subject: [PATCH 02/10] Fix portable PG version validation --- .github/scripts/check-pg-versions.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/scripts/check-pg-versions.sh b/.github/scripts/check-pg-versions.sh index b0d73651de3..aedc895b77f 100755 --- a/.github/scripts/check-pg-versions.sh +++ b/.github/scripts/check-pg-versions.sh @@ -123,8 +123,8 @@ check_file_for_pg_changes() { echo " ⚠️ Struct definition modified in $struct_found_in" # Check if version was incremented in PG_REGISTER - local old_version=$(echo "$diff_output" | grep "^-.*PG_REGISTER.*$struct_type" | grep -oP ',\s*\K\d+(?=\s*\))' || echo "") - local new_version=$(echo "$diff_output" | grep "^+.*PG_REGISTER.*$struct_type" | grep -oP ',\s*\K\d+(?=\s*\))' || echo "") + local old_version=$(echo "$diff_output" | grep "^-.*PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") + local new_version=$(echo "$diff_output" | grep "^+.*PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") # Find line number of PG_REGISTER for error reporting local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) From 6f0895e4dbd56f4e43631fea0dd0b4b083d465cf Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Thu, 10 Sep 2026 17:32:01 +0200 Subject: [PATCH 03/10] Fix PG checker scope, conditional versions and workflow output handling Reuse the CI fixes from #11885 and cover scalar, array and conditional registrations with regression fixtures. --- .github/scripts/check-pg-versions.sh | 28 ++++++++++++++++++----- .github/scripts/test-check-pg-versions.py | 20 ++++++++++++++++ .github/workflows/pg-version-check.yml | 10 ++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 .github/scripts/test-check-pg-versions.py diff --git a/.github/scripts/check-pg-versions.sh b/.github/scripts/check-pg-versions.sh index aedc895b77f..ea37274fa12 100755 --- a/.github/scripts/check-pg-versions.sh +++ b/.github/scripts/check-pg-versions.sh @@ -85,7 +85,8 @@ check_file_for_pg_changes() { local struct_type="${BASH_REMATCH[1]}" local pg_name="${BASH_REMATCH[2]}" local pg_id="${BASH_REMATCH[3]}" - local version="${BASH_REMATCH[4]}" + # Arrays have an extra count argument; the version is always last. + local version=$(echo "$pg_line" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p') # Clean up whitespace struct_type=$(echo "$struct_type" | xargs) @@ -123,21 +124,36 @@ check_file_for_pg_changes() { echo " ⚠️ Struct definition modified in $struct_found_in" # Check if version was incremented in PG_REGISTER - local old_version=$(echo "$diff_output" | grep "^-.*PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") - local new_version=$(echo "$diff_output" | grep "^+.*PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") + local old_version=$(git show "$BASE_COMMIT:$file" 2>/dev/null | grep "PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") + local new_version=$(git show "$HEAD_COMMIT:$file" 2>/dev/null | grep "PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") # Find line number of PG_REGISTER for error reporting local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) if [ -n "$old_version" ] && [ -n "$new_version" ]; then - # PG_REGISTER was modified - check if version increased - if [ "$new_version" -le "$old_version" ]; then + # Conditional builds can register the same type several times. + # Compare every registration, including unchanged alternatives. + local old_versions=() new_versions=() + read -r -a old_versions <<< "$(echo "$old_version" | tr '\n' ' ')" + read -r -a new_versions <<< "$(echo "$new_version" | tr '\n' ' ')" + local versions_increased=true + local version_index + if [ "${#old_versions[@]}" -ne "${#new_versions[@]}" ]; then + versions_increased=false + else + for version_index in "${!old_versions[@]}"; do + if [ "${new_versions[$version_index]}" -le "${old_versions[$version_index]}" ]; then + versions_increased=false + fi + done + fi + if [ "$versions_increased" = false ]; then echo " ❌ Version NOT incremented ($old_version → $new_version)" cat >> $ISSUES_FILE << EOF ### \`$struct_type\` ($file:$line_num) - **Struct modified:** Field changes detected in $struct_found_in - **Version status:** ❌ Not incremented (version $version) -- **Recommendation:** Increment version from $old_version to $(($old_version + 1)) +- **Recommendation:** Verify that every conditional registration has its version incremented EOF else diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py new file mode 100644 index 00000000000..d8bff310340 --- /dev/null +++ b/.github/scripts/test-check-pg-versions.py @@ -0,0 +1,20 @@ +import subprocess,tempfile,pathlib,os +script=str(pathlib.Path(__file__).with_name('check-pg-versions.sh').resolve()) +cases=[('unchanged',False,False,[1],[1],0),('missing bump',True,False,[1],[1],1),('bumped',True,False,[1],[2],0),('array missing',True,True,[4],[4],1),('array bumped',True,True,[4],[5],0),('conditional bumped',True,True,[4,1],[5,2],0),('conditional partial',True,True,[4,1],[5,1],1),('conditional decreased',True,True,[4,1],[3,2],1)] +for label,changed,array,old,new,expected in cases: + with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + git('init'); p=pathlib.Path(d) + def reg(versions): + lines=[f'PG_REGISTER_{"ARRAY_" if array else ""}WITH_RESET_FN(config_t, {"3, " if array else ""}config, PG_CONFIG, {v});' for v in versions] + return '\n'.join(lines) if len(lines)==1 else '#ifdef LARGE\n'+lines[0]+'\n#else\n'+lines[1]+'\n#endif\n' + (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.c').write_text(reg(old)) + git('add','.'); git('commit','-m','base') + if changed: (p/'config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n') + (p/'config.c').write_text(reg(new)) + git('add','.'); git('commit','--allow-empty','-m','head') + r=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env={k:v for k,v in os.environ.items() if k not in ('GITHUB_BASE_REF','GITHUB_HEAD_REF')}) + print(label,'exit',r.returncode,'expected',expected) + assert r.returncode==expected,r.stdout+r.stderr + assert 'integer expression expected' not in r.stderr,r.stderr diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index 89c71c82224..36bd53f80f2 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -9,6 +9,9 @@ on: paths: - 'src/**/*.c' - 'src/**/*.h' + - '.github/scripts/check-pg-versions.sh' + - '.github/scripts/test-check-pg-versions.py' + - '.github/workflows/pg-version-check.yml' jobs: check-pg-versions: @@ -27,6 +30,9 @@ jobs: run: | git fetch origin ${{ github.base_ref }}:refs/remotes/origin/${{ github.base_ref }} + - name: Test PG version checker + run: python3 .github/scripts/test-check-pg-versions.py + - name: Run PG version check script id: pg_check run: | @@ -35,6 +41,10 @@ jobs: # The output is captured and encoded to be passed between steps. output=$(bash .github/scripts/check-pg-versions.sh 2>&1) exit_code=$? + if [ "$exit_code" -gt 1 ]; then + printf '%s\n' "$output" + exit "$exit_code" + fi echo "exit_code=${exit_code}" >> $GITHUB_OUTPUT echo "output<> $GITHUB_OUTPUT echo "$output" >> $GITHUB_OUTPUT From d42addb88f7c1d4efb32d8537af7f132c42f420e Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Thu, 10 Sep 2026 17:35:50 +0200 Subject: [PATCH 04/10] Ignore PG macro definitions and fail on incomplete checker runs --- .github/scripts/check-pg-versions.sh | 7 ++++++- .github/scripts/test-check-pg-versions.py | 4 ++-- .github/workflows/pg-version-check.yml | 4 ++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/scripts/check-pg-versions.sh b/.github/scripts/check-pg-versions.sh index ea37274fa12..71108218b3b 100755 --- a/.github/scripts/check-pg-versions.sh +++ b/.github/scripts/check-pg-versions.sh @@ -88,8 +88,13 @@ check_file_for_pg_changes() { # Arrays have an extra count argument; the version is always last. local version=$(echo "$pg_line" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p') + # Macro definitions and examples can contain PG_REGISTER too. + # A registration must have a literal numeric version. + [[ "$version" =~ ^[0-9]+$ ]] || continue + # Clean up whitespace - struct_type=$(echo "$struct_type" | xargs) + struct_type=$(echo "$struct_type" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') + [[ "$struct_type" =~ ^[a-zA-Z_][a-zA-Z0-9_]*$ ]] || continue version=$(echo "$version" | xargs) echo " 📋 Found: $struct_type (version $version)" diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py index d8bff310340..f34c911a80c 100644 --- a/.github/scripts/test-check-pg-versions.py +++ b/.github/scripts/test-check-pg-versions.py @@ -8,10 +8,10 @@ def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.em def reg(versions): lines=[f'PG_REGISTER_{"ARRAY_" if array else ""}WITH_RESET_FN(config_t, {"3, " if array else ""}config, PG_CONFIG, {v});' for v in versions] return '\n'.join(lines) if len(lines)==1 else '#ifdef LARGE\n'+lines[0]+'\n#else\n'+lines[1]+'\n#endif\n' - (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.h').write_text('#define PG_REGISTER_FAKE(type, name, id, version) \"not a registration\"\n'+'typedef struct config_s {\n int old;\n} config_t;\n') (p/'config.c').write_text(reg(old)) git('add','.'); git('commit','-m','base') - if changed: (p/'config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n') + if changed: (p/'config.h').write_text('#define PG_REGISTER_FAKE(type, name, id, version) \"not a registration\"\n'+'typedef struct config_s {\n int old;\n int added;\n} config_t;\n') (p/'config.c').write_text(reg(new)) git('add','.'); git('commit','--allow-empty','-m','head') r=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env={k:v for k,v in os.environ.items() if k not in ('GITHUB_BASE_REF','GITHUB_HEAD_REF')}) diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index 36bd53f80f2..98aef49b20b 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -41,9 +41,9 @@ jobs: # The output is captured and encoded to be passed between steps. output=$(bash .github/scripts/check-pg-versions.sh 2>&1) exit_code=$? - if [ "$exit_code" -gt 1 ]; then + if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^### ' <<< "$output"; }; then printf '%s\n' "$output" - exit "$exit_code" + exit 2 fi echo "exit_code=${exit_code}" >> $GITHUB_OUTPUT echo "output<> $GITHUB_OUTPUT From 5efd79495122adcee2c77ef87a316bb12889ae05 Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Fri, 11 Sep 2026 18:28:29 +0200 Subject: [PATCH 05/10] Check PG layouts across files and conditional build variants --- .github/scripts/check-pg-versions.py | 199 +++++++++++++++++ .github/scripts/check-pg-versions.sh | 248 +--------------------- .github/scripts/test-check-pg-versions.py | 46 ++++ .github/workflows/pg-version-check.yml | 1 + 4 files changed, 248 insertions(+), 246 deletions(-) create mode 100644 .github/scripts/check-pg-versions.py diff --git a/.github/scripts/check-pg-versions.py b/.github/scripts/check-pg-versions.py new file mode 100644 index 00000000000..b6964d05d6d --- /dev/null +++ b/.github/scripts/check-pg-versions.py @@ -0,0 +1,199 @@ +#!/usr/bin/env python3 +"""Compare changed PG structs against registrations across the repository. + +Preprocessor branches are compared symbolically. Complex #if expressions are +conservative independent conditions; this is not a C ABI or macro-expansion check. +""" +import ast +import functools +import itertools +import os +import re +import subprocess +import sys + + +def git(*args, allow_missing=False): + result = subprocess.run(['git', *args], capture_output=True, text=True) + if result.returncode and not (allow_missing and result.returncode == 1): + raise RuntimeError(result.stderr.strip() or 'git command failed') + return result.stdout + + +def clean(source): + return re.sub(r'/\*.*?\*/|//[^\n]*', lambda m: '\n' * m[0].count('\n'), source, flags=re.S) + + +def condition(text): + text = re.sub(r'\s+', '', text) + match = re.fullmatch(r'(!?)defined\(?([A-Za-z_]\w*)\)?', text) + return (('defined:' + match[2], not bool(match[1])) if match else (text, True)) + + +def annotated(source): + """Attach surrounding #if/#elif/#else predicates to each non-directive line.""" + stack = [] + result = [] + for line in source.splitlines(keepends=True): + match = re.match(r'\s*#\s*(if|ifdef|ifndef|elif|else|endif)\b(.*)', line) + if match: + directive, value = match.groups() + if directive in ('if', 'ifdef', 'ifndef'): + atom = condition(value) if directive == 'if' else ('defined:' + value.strip(), directive == 'ifdef') + stack.append(([atom], [atom])) + elif directive == 'elif': + previous, _ = stack[-1] + atom = condition(value) + stack[-1] = (previous + [atom], [(a, not b) for a, b in previous] + [atom]) + elif directive == 'else': + previous, _ = stack[-1] + stack[-1] = (previous, [(a, not b) for a, b in previous]) + else: + stack.pop() + result.append(('', ())) + elif re.match(r'\s*#', line): + result.append(('', ())) + else: + result.append((line, tuple(item for _, active in stack for item in active))) + return result + + +def structures(source): + source = clean(source) + lines = annotated(source) + result = {} + for match in re.finditer(r'\btypedef\s+struct(?:\s+[A-Za-z_]\w*)?\s*\{', source): + depth, end = 1, match.end() + while end < len(source) and depth: + depth += (source[end] == '{') - (source[end] == '}') + end += 1 + alias = re.match(r'\s*([A-Za-z_]\w*)\s*;', source[end:]) + if not alias: + continue + first = source.count('\n', 0, match.start()) + last = source.count('\n', 0, end) + 1 + result[alias[1]] = lines[first:last] + return result + + +def registrations(ref): + paths = git('grep', '-l', '-E', 'PG_REGISTER', ref, '--', '*.c', '*.h', allow_missing=True).splitlines() + result = {} + for entry in paths: + path = entry[len(ref) + 1:] + lines = annotated(clean(git('show', ref + ':' + path))) + text = ''.join(line if line.endswith('\n') else line + '\n' for line, _ in lines) + for match in re.finditer(r'\bPG_REGISTER\w*\s*\(([^;]+?)\)\s*;', text): + args = [arg.strip() for arg in match[1].split(',')] + if len(args) < 4 or not re.fullmatch(r'[A-Za-z_]\w*', args[0]) or not args[-1].isdigit(): + continue + line = text.count('\n', 0, match.start()) + result.setdefault(args[0], []).append((args[-2], int(args[-1]), lines[line][1], path)) + return result + + +@functools.lru_cache(maxsize=None) +def boolean_expression(expression): + names = [] + def replace_defined(match): + names.append('defined:' + (match[1] or match[2])) + return 'v' + str(len(names) - 1) + translated = re.sub(r'defined(?:\(([A-Za-z_]\w*)\)|([A-Za-z_]\w*))', replace_defined, expression) + if not names: + return None + translated = translated.replace('&&', ' and ').replace('||', ' or ').replace('!', ' not ').strip() + try: + tree = ast.parse(translated, mode='eval') + except SyntaxError: + return None + allowed = (ast.Expression, ast.BoolOp, ast.And, ast.Or, ast.UnaryOp, ast.Not, ast.Name, ast.Load) + if any(not isinstance(node, allowed) for node in ast.walk(tree)): + return None + if any(isinstance(node, ast.Name) and node.id not in {'v' + str(i) for i in range(len(names))} for node in ast.walk(tree)): + return None + return tree.body, names + + +def variables(expression): + parsed = boolean_expression(expression) + return parsed[1] if parsed else [expression] + + +def evaluate(expression, values): + if expression in ('0', '1'): + return bool(int(expression)) + parsed = boolean_expression(expression) + if not parsed: + return values[expression] + tree, names = parsed + def visit(node): + if isinstance(node, ast.Name): + return values[names[int(node.id[1:])]] + if isinstance(node, ast.UnaryOp): + return not visit(node.operand) + operands = [visit(value) for value in node.values] + return all(operands) if isinstance(node.op, ast.And) else any(operands) + return visit(tree) + + +def active(predicates, values): + return all(evaluate(atom, values) == expected for atom, expected in predicates) + + +def layout(lines, values): + return ''.join(re.sub(r'\s+', '', line) for line, predicates in lines if active(predicates, values)) + + +def check(base, head): + base = git('merge-base', base, head).strip() + changed = [path for path in git('diff', '--name-only', base + '..' + head).splitlines() if path.endswith(('.c', '.h'))] + if not changed: + print('No C/H files changed') + return 0 + old_paths = set(git('ls-tree', '-r', '--name-only', base).splitlines()) + new_paths = set(git('ls-tree', '-r', '--name-only', head).splitlines()) + old_structs, new_structs = {}, {} + for path in changed: + if path in old_paths: + old_structs.update(structures(git('show', base + ':' + path))) + if path in new_paths: + new_structs.update(structures(git('show', head + ':' + path))) + old_regs, new_regs = registrations(base), registrations(head) + issues = [] + for name in old_structs.keys() & new_structs.keys() & new_regs.keys(): + before, after = old_structs[name], new_structs[name] + if before == after: + continue + previous = old_regs.get(name, []) + current = new_regs[name] + if not previous: + continue # No persisted instance existed before this change. + predicates = [p for _, p in before + after] + [r[2] for r in previous + current] + atoms = sorted({variable for predicate in predicates for atom, _ in predicate for variable in variables(atom)} - {'0', '1'}) + if len(atoms) > 10: + issues.append(f'{name}: more than 10 conditional expressions; manually verify the PG versions') + continue + for flags in itertools.product((False, True), repeat=len(atoms)): + values = dict(zip(atoms, flags)) + old_layout, new_layout = layout(before, values), layout(after, values) + if old_layout == new_layout or not old_layout: + continue + old_versions = {r[0]: r[1] for r in previous if active(r[2], values)} + new_versions = {r[0]: r[1] for r in current if active(r[2], values)} + if any(pg not in new_versions or new_versions[pg] <= version for pg, version in old_versions.items()): + issues.append(f'{name}: changed layout without a version increase in {", ".join(sorted({r[3] for r in current}))}; conditions {values}') + break + for issue in issues: + print('PG version issue: ' + issue) + if not issues: + print('No PG version issues detected') + return int(bool(issues)) + + +if __name__ == '__main__': + try: + base = 'origin/' + os.environ['GITHUB_BASE_REF'] if os.environ.get('GITHUB_BASE_REF') and os.environ.get('GITHUB_HEAD_REF') else 'HEAD~1' + sys.exit(check(base, 'HEAD')) + except (RuntimeError, ValueError, IndexError, OSError) as error: + print('PG checker error: ' + str(error), file=sys.stderr) + sys.exit(2) diff --git a/.github/scripts/check-pg-versions.sh b/.github/scripts/check-pg-versions.sh index 71108218b3b..5455b25b705 100755 --- a/.github/scripts/check-pg-versions.sh +++ b/.github/scripts/check-pg-versions.sh @@ -1,248 +1,4 @@ #!/bin/bash -# -# Check if parameter group struct modifications include version increments -# This prevents settings corruption when struct layout changes without version bump -# -# Exit codes: -# 0 - No issues found -# 1 - Potential issues detected (will post comment) -# 2 - Script error - +# Exit 0: checked, 1: potential PG version issue, 2: checker error. set -euo pipefail - -# Output file for issues found -ISSUES_FILE=$(mktemp) -trap "rm -f $ISSUES_FILE" EXIT - -# Color output for local testing -if [ -t 1 ]; then - RED='\033[0;31m' - GREEN='\033[0;32m' - YELLOW='\033[1;33m' - NC='\033[0m' # No Color -else - RED='' - GREEN='' - YELLOW='' - NC='' -fi - -echo "🔍 Checking for Parameter Group version updates..." - -# Get base and head commits -BASE_REF=${GITHUB_BASE_REF:-} -HEAD_REF=${GITHUB_HEAD_REF:-} - -if [ -z "$BASE_REF" ] || [ -z "$HEAD_REF" ]; then - echo "⚠️ Warning: Not running in GitHub Actions PR context" - echo "Using git diff against HEAD~1 for local testing" - BASE_COMMIT="HEAD~1" - HEAD_COMMIT="HEAD" -else - BASE_COMMIT="origin/$BASE_REF" - HEAD_COMMIT="HEAD" -fi - -# Get list of changed files -CHANGED_FILES=$(git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -E '\.(c|h)$' || true) - -if [ -z "$CHANGED_FILES" ]; then - echo "✅ No C/H files changed" - exit 0 -fi - -echo "📁 Changed files:" -echo "$CHANGED_FILES" | sed 's/^/ /' - -# Function to extract PG info from a file -check_file_for_pg_changes() { - local file=$1 - local diff_output=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$file") - - # Check if file contains PG_REGISTER in current version - if ! git show $HEAD_COMMIT:"$file" 2>/dev/null | grep -q "PG_REGISTER"; then - return 0 - fi - - echo " 🔎 Checking $file (contains PG_REGISTER)" - - # Extract all PG_REGISTER lines from the diff (both old and new) - local pg_registers=$(echo "$diff_output" | grep -E "^[-+].*PG_REGISTER" || true) - - if [ -z "$pg_registers" ]; then - # PG_REGISTER exists but wasn't changed - # Still need to check if the struct changed - pg_registers=$(git show $HEAD_COMMIT:"$file" | grep "PG_REGISTER" || true) - fi - - # Process each PG registration - while IFS= read -r pg_line; do - [ -z "$pg_line" ] && continue - - # Extract struct name and version - # Pattern: PG_REGISTER.*\((\w+),\s*(\w+),\s*PG_\w+,\s*(\d+)\) - if [[ $pg_line =~ PG_REGISTER[^(]*\(([^,]+),([^,]+),([^,]+),([^)]+)\) ]]; then - local struct_type="${BASH_REMATCH[1]}" - local pg_name="${BASH_REMATCH[2]}" - local pg_id="${BASH_REMATCH[3]}" - # Arrays have an extra count argument; the version is always last. - local version=$(echo "$pg_line" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p') - - # Macro definitions and examples can contain PG_REGISTER too. - # A registration must have a literal numeric version. - [[ "$version" =~ ^[0-9]+$ ]] || continue - - # Clean up whitespace - struct_type=$(echo "$struct_type" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') - [[ "$struct_type" =~ ^[a-zA-Z_][a-zA-Z0-9_]*$ ]] || continue - version=$(echo "$version" | xargs) - - echo " 📋 Found: $struct_type (version $version)" - - # Check if this struct's typedef was modified in ANY changed file - local struct_pattern="typedef struct ${struct_type%_t}_s" - local struct_body_diff="" - local struct_found_in="" - - # Search all changed files for this struct definition - while IFS= read -r changed_file; do - [ -z "$changed_file" ] && continue - - local file_diff=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$changed_file") - local struct_in_file=$(echo "$file_diff" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p") - - if [ -n "$struct_in_file" ]; then - struct_body_diff="$struct_in_file" - struct_found_in="$changed_file" - echo " 🔍 Found struct definition in $changed_file" - break - fi - done <<< "$CHANGED_FILES" - - local struct_changes=$(echo "$struct_body_diff" | grep -E "^[-+]" \ - | grep -v -E "^[-+]\s*(typedef struct|}|//|\*)" \ - | sed -E 's://.*$::' \ - | sed -E 's:/\*.*\*/::' \ - | tr -d '[:space:]') - - if [ -n "$struct_changes" ]; then - echo " ⚠️ Struct definition modified in $struct_found_in" - - # Check if version was incremented in PG_REGISTER - local old_version=$(git show "$BASE_COMMIT:$file" 2>/dev/null | grep "PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") - local new_version=$(git show "$HEAD_COMMIT:$file" 2>/dev/null | grep "PG_REGISTER.*$struct_type" | sed -nE 's/.*,[[:space:]]*([0-9]+)[[:space:]]*\).*/\1/p' || echo "") - - # Find line number of PG_REGISTER for error reporting - local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) - - if [ -n "$old_version" ] && [ -n "$new_version" ]; then - # Conditional builds can register the same type several times. - # Compare every registration, including unchanged alternatives. - local old_versions=() new_versions=() - read -r -a old_versions <<< "$(echo "$old_version" | tr '\n' ' ')" - read -r -a new_versions <<< "$(echo "$new_version" | tr '\n' ' ')" - local versions_increased=true - local version_index - if [ "${#old_versions[@]}" -ne "${#new_versions[@]}" ]; then - versions_increased=false - else - for version_index in "${!old_versions[@]}"; do - if [ "${new_versions[$version_index]}" -le "${old_versions[$version_index]}" ]; then - versions_increased=false - fi - done - fi - if [ "$versions_increased" = false ]; then - echo " ❌ Version NOT incremented ($old_version → $new_version)" - cat >> $ISSUES_FILE << EOF -### \`$struct_type\` ($file:$line_num) -- **Struct modified:** Field changes detected in $struct_found_in -- **Version status:** ❌ Not incremented (version $version) -- **Recommendation:** Verify that every conditional registration has its version incremented - -EOF - else - echo " ✅ Version incremented ($old_version → $new_version)" - fi - elif [ -z "$old_version" ] && [ -z "$new_version" ]; then - # PG_REGISTER wasn't modified but struct was - THIS IS THE BUG! - echo " ❌ PG_REGISTER not modified, version still $version" - cat >> $ISSUES_FILE << EOF -### \`$struct_type\` ($file:$line_num) -- **Struct modified:** Field changes detected in $struct_found_in -- **Version status:** ❌ Not incremented (still version $version) -- **Recommendation:** Increment version to $(($version + 1)) in $file - -EOF - else - # One exists but not the other - unusual edge case - echo " ⚠️ Unusual version change pattern detected" - cat >> $ISSUES_FILE << EOF -### \`$struct_type\` ($file:$line_num) -- **Struct modified:** Field changes detected in $struct_found_in -- **Version status:** ⚠️ Unusual change pattern (old: ${old_version:-none}, new: ${new_version:-none}) -- **Current version:** $version -- **Recommendation:** Manually verify version increment - -EOF - fi - else - echo " ✅ Struct unchanged" - fi - fi - done <<< "$pg_registers" -} - -# Build list of files to check (changed files + companions with PG_REGISTER) -echo "🔍 Building file list including companions with PG_REGISTER..." -FILES_TO_CHECK="" -ALREADY_ADDED="" - -while IFS= read -r file; do - [ -z "$file" ] && continue - - # Add this file to check list - if ! echo "$ALREADY_ADDED" | grep -qw "$file"; then - FILES_TO_CHECK="$FILES_TO_CHECK$file"$'\n' - ALREADY_ADDED="$ALREADY_ADDED $file" - fi - - # Determine companion file (.c <-> .h) - # (this loop runs at top level, so no "local" here: bash would abort the script) - companion="" - if [[ "$file" == *.c ]]; then - companion="${file%.c}.h" - elif [[ "$file" == *.h ]]; then - companion="${file%.h}.c" - fi - - # If companion exists and contains PG_REGISTER, add it to check list - if [ -n "$companion" ]; then - if git show $HEAD_COMMIT:"$companion" 2>/dev/null | grep -q "PG_REGISTER"; then - if ! echo "$ALREADY_ADDED" | grep -qw "$companion"; then - echo " 📎 Adding $companion (companion of $file with PG_REGISTER)" - FILES_TO_CHECK="$FILES_TO_CHECK$companion"$'\n' - ALREADY_ADDED="$ALREADY_ADDED $companion" - fi - fi - fi -done <<< "$CHANGED_FILES" - -# Check each file (including companions) -while IFS= read -r file; do - [ -z "$file" ] && continue - check_file_for_pg_changes "$file" -done <<< "$FILES_TO_CHECK" - -# Check if any issues were found -if [ -s $ISSUES_FILE ]; then - echo "" - echo "${YELLOW}⚠️ Potential PG version issues detected${NC}" - echo "Output saved to: $ISSUES_FILE" - cat $ISSUES_FILE - exit 1 -else - echo "" - echo "${GREEN}✅ No PG version issues detected${NC}" - exit 0 -fi +exec python3 "$(dirname "$0")/check-pg-versions.py" diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py index f34c911a80c..888bd851d12 100644 --- a/.github/scripts/test-check-pg-versions.py +++ b/.github/scripts/test-check-pg-versions.py @@ -18,3 +18,49 @@ def reg(versions): print(label,'exit',r.returncode,'expected',expected) assert r.returncode==expected,r.stdout+r.stderr assert 'integer expression expected' not in r.stderr,r.stderr + +# Registrations need not share the structure header's basename, and a conditional +# field must only require a bump for the build variant in which it exists. +for label, header, conditional, versions, expected in [ + ('different basename missing', 'battery_config_structs.h', False, [4], 1), + ('different basename bumped', 'battery_config_structs.h', False, [5], 0), + ('conditional field affected bumped', 'battery_config_structs.h', True, [5, 1], 0), + ('conditional field unaffected bumped', 'battery_config_structs.h', True, [4, 2], 1), + ('conditional field neither bumped', 'battery_config_structs.h', True, [4, 1], 1), + ('compound condition affected bumped', 'battery_config_structs.h', 'compound', [5, 1], 0), + ('compound condition unaffected bumped', 'battery_config_structs.h', 'compound', [4, 2], 1), +]: + with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + p=pathlib.Path(d);git('init') + def registration(v): + rows=[f'PG_REGISTER_WITH_RESET_FN(config_t, config, PG_CONFIG, {x});' for x in v] + return rows[0] if len(rows)==1 else '#ifdef LARGE\n'+rows[0]+'\n#else\n'+rows[1]+'\n#endif\n' + (p/header).write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'battery.c').write_text(registration([4,1] if conditional else [4])) + git('add','.');git('commit','-m','base') + field='#ifdef LARGE\n int added;\n#endif\n' if conditional else ' int added;\n' + if conditional == 'compound': field = '#if defined(LARGE) && defined(EXTRA)\n int added;\n#endif\n' + (p/header).write_text('typedef struct config_s {\n int old;\n'+field+'} config_t;\n') + (p/'battery.c').write_text(registration(versions));git('add','.');git('commit','-m','head') + env={k:v for k,v in os.environ.items() if k not in ('GITHUB_BASE_REF','GITHUB_HEAD_REF')} + result=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env=env) + print(label,'exit',result.returncode,'expected',expected) + assert result.returncode==expected,result.stdout+result.stderr + + +# Advancing the base branch must not make changes outside the PR look like removals. +with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + p=pathlib.Path(d);git('init') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.c').write_text('PG_REGISTER_WITH_RESET_FN(config_t, config, PG_CONFIG, 1);\n') + git('add','.');git('commit','-m','common');common=git('rev-parse','HEAD') + (p/'readme.md').write_text('PR documentation only');git('add','.');git('commit','-m','PR');head=git('rev-parse','HEAD') + git('checkout','--detach',common) + (p/'config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n');git('add','.');git('commit','-m','base advancement') + git('update-ref','refs/remotes/origin/test-base','HEAD');git('checkout','--detach',head) + env=dict(os.environ,GITHUB_BASE_REF='test-base',GITHUB_HEAD_REF='feature') + result=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env=env) + print('advanced base uses merge-base','exit',result.returncode,'expected',0) + assert result.returncode==0,result.stdout+result.stderr diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index 98aef49b20b..c9caac7eaaf 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -10,6 +10,7 @@ on: - 'src/**/*.c' - 'src/**/*.h' - '.github/scripts/check-pg-versions.sh' + - '.github/scripts/check-pg-versions.py' - '.github/scripts/test-check-pg-versions.py' - '.github/workflows/pg-version-check.yml' From 6603b63102a92ac2dc0a9cd6fbff7b0ed0db2469 Mon Sep 17 00:00:00 2001 From: Raphael Hunziker Date: Sun, 13 Sep 2026 21:25:02 +0200 Subject: [PATCH 06/10] docs: describe the Python parameter-group checker The workflow README still described the shell implementation and its local-testing snippet did not mention python3 or the regression fixtures. --- .github/workflows/README.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 7e28f7cad01..5e59aacf056 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -40,14 +40,19 @@ fire. **Why:** Prevents settings corruption when struct layout changes without version bump **How it works:** -1. Scans changed .c/.h files for `PG_REGISTER` entries -2. Detects if associated struct typedefs were modified -3. Checks if the PG version parameter was incremented +1. Maps every `PG_REGISTER` in the repository's .c/.h files to its struct, so a registration in a + different file than the struct is still found +2. Detects if associated struct typedefs were modified, comparing against the PR's merge base + so later changes on the base branch are not attributed to the PR +3. Checks if the PG version parameter was incremented, per preprocessor condition: a struct + guarded by `#ifdef` is only compared under the conditions where it actually changes 4. Posts helpful comment if version not incremented **Reference:** See `docs/development/parameter_groups/` for PG system documentation -**Script:** `.github/scripts/check-pg-versions.sh` +**Script:** `.github/scripts/check-pg-versions.sh`, a thin wrapper around +`.github/scripts/check-pg-versions.py` (standard library only, python3 required). Its regression +fixtures live in `.github/scripts/test-check-pg-versions.py` and run in CI. **When to increment PG versions:** - ✅ Adding/removing fields from struct @@ -182,7 +187,10 @@ Scripts in `.github/scripts/` can be run locally: cd inav export GITHUB_BASE_REF=maintenance-9.x export GITHUB_HEAD_REF=feature-branch -bash .github/scripts/check-pg-versions.sh +bash .github/scripts/check-pg-versions.sh # needs python3 on PATH + +# run the checker's own regression fixtures +python3 .github/scripts/test-check-pg-versions.py ``` ## References From e2762ab638db53518284e8a8e5dcad1b64355654 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 14 Sep 2026 17:03:34 -0500 Subject: [PATCH 07/10] Fix PG checker output-format mismatch and non-UTF-8 file handling Two commits in this same PR chain drifted apart: d42addb88f added a workflow guard requiring '### ' in the checker's stdout to treat exit code 1 as a normal detection, but 5efd794951 rewrote the checker in Python, which prints "PG version issue: ..." with no '###' anywhere. As a result the workflow hard-fails (exit 2, raw stdout dump) instead of posting the intended PR comment on every genuine detection - the one case this tooling exists for. Verified by running the workflow's own guard logic against the checker's real "issue found" output before and after this fix. Same fix applied to the PR comment step's JS output filter, which keyed on the same stale '###' marker. Also decode git subprocess output with errors='replace' instead of the default strict UTF-8, since a single non-ASCII byte (e.g. a smart quote in a comment) anywhere in a touched .c/.h file would otherwise raise an uncaught UnicodeDecodeError and hard-fail the check with a message that doesn't name the offending file. Added a regression fixture that runs the workflow's actual guard logic (read from the workflow YAML, not duplicated) against a real detected issue, so the two can't silently diverge again. --- .github/scripts/check-pg-versions.py | 2 +- .github/scripts/test-check-pg-versions.py | 30 ++++++++++++++++++++++- .github/workflows/pg-version-check.yml | 4 +-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/scripts/check-pg-versions.py b/.github/scripts/check-pg-versions.py index b6964d05d6d..2122215491a 100644 --- a/.github/scripts/check-pg-versions.py +++ b/.github/scripts/check-pg-versions.py @@ -14,7 +14,7 @@ def git(*args, allow_missing=False): - result = subprocess.run(['git', *args], capture_output=True, text=True) + result = subprocess.run(['git', *args], capture_output=True, text=True, encoding='utf-8', errors='replace') if result.returncode and not (allow_missing and result.returncode == 1): raise RuntimeError(result.stderr.strip() or 'git command failed') return result.stdout diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py index 888bd851d12..548cfabdf4d 100644 --- a/.github/scripts/test-check-pg-versions.py +++ b/.github/scripts/test-check-pg-versions.py @@ -1,5 +1,6 @@ -import subprocess,tempfile,pathlib,os +import re,subprocess,tempfile,pathlib,os script=str(pathlib.Path(__file__).with_name('check-pg-versions.sh').resolve()) +workflow=pathlib.Path(__file__).parents[1]/'workflows'/'pg-version-check.yml' cases=[('unchanged',False,False,[1],[1],0),('missing bump',True,False,[1],[1],1),('bumped',True,False,[1],[2],0),('array missing',True,True,[4],[4],1),('array bumped',True,True,[4],[5],0),('conditional bumped',True,True,[4,1],[5,2],0),('conditional partial',True,True,[4,1],[5,1],1),('conditional decreased',True,True,[4,1],[3,2],1)] for label,changed,array,old,new,expected in cases: with tempfile.TemporaryDirectory() as d: @@ -64,3 +65,30 @@ def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.em result=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env=env) print('advanced base uses merge-base','exit',result.returncode,'expected',0) assert result.returncode==0,result.stdout+result.stderr + +# The workflow's own "Run PG version check script" step re-parses the checker's stdout +# with a bash guard and (on the next step) a JS filter, both keyed on a literal string. +# Run that guard for real, against the checker's real "issue found" output, so the two +# can't silently drift apart the way they did across two commits in this same PR chain +# (one added a '^### ' guard for the old bash script's Markdown headings, a later one +# rewrote the checker in Python with no '###' anywhere in its output). +run_block=re.search(r"- name: Run PG version check script\n(?:.*\n)*? run: \|\n((?:( {10}.*)?\n)+)",workflow.read_text()) +assert run_block,'could not find the "Run PG version check script" step in ' + str(workflow) +guard=run_block[1] +assert 'check-pg-versions.sh' in guard and 'exit_code' in guard,'unexpected step contents:\n' + guard +with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + p=pathlib.Path(d);git('init') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.c').write_text('PG_REGISTER_WITH_RESET_FN(config_t, config, PG_CONFIG, 1);\n') + git('add','.');git('commit','-m','base');base=git('rev-parse','HEAD') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n') + git('add','.');git('commit','--allow-empty','-m','head, missing version bump') + git('update-ref','refs/remotes/origin/test-base',base) + wrapper='#!/bin/bash\nset -e\ncd ' + d + '\n' + guard.replace('.github/scripts/check-pg-versions.sh', script) + outputs=str(p/'github_output') + env=dict(os.environ,GITHUB_BASE_REF='test-base',GITHUB_HEAD_REF='feature',GITHUB_OUTPUT=outputs) + result=subprocess.run(['bash','-c',wrapper],capture_output=True,text=True,env=env) + print('workflow guard accepts a real detected issue','exit',result.returncode,'expected',0) + assert result.returncode==0,'the workflow step would hard-fail instead of posting a comment:\n'+result.stdout+result.stderr + assert 'exit_code=1' in pathlib.Path(outputs).read_text(),'workflow step did not record the issue for the comment step' diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index c9caac7eaaf..660ab5d7053 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -42,7 +42,7 @@ jobs: # The output is captured and encoded to be passed between steps. output=$(bash .github/scripts/check-pg-versions.sh 2>&1) exit_code=$? - if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^### ' <<< "$output"; }; then + if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^PG version issue:' <<< "$output"; }; then printf '%s\n' "$output" exit 2 fi @@ -74,7 +74,7 @@ jobs: let issues = []; for (const line of lines) { - if (line.includes('###')) { + if (line.includes('PG version issue:')) { capturing = true; } if (capturing) { From 172174158b38dfe4aa2adee058c4f6cabc3580b1 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 14 Sep 2026 17:20:45 -0500 Subject: [PATCH 08/10] Run the PG version check on release/9.1 PRs too The check previously only triggered on PRs targeting maintenance-9.x and maintenance-10.x, so bugfixes landing directly on release/9.1 that touch a PG struct got no automated version-bump check at all. Add release/9.1 to the trigger's branch list and update the workflow README's documented scope to match. --- .github/workflows/README.md | 2 +- .github/workflows/pg-version-check.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 5e59aacf056..fbd80adeda8 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -35,7 +35,7 @@ fire. ### Code Quality #### `pg-version-check.yml` - Parameter Group Version Check -**Triggers:** Pull requests to maintenance-9.x and maintenance-10.x +**Triggers:** Pull requests to maintenance-9.x, maintenance-10.x, and release/9.1 **Purpose:** Detects parameter group struct modifications and verifies version increments **Why:** Prevents settings corruption when struct layout changes without version bump diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index 660ab5d7053..edf0dcdf904 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -6,6 +6,7 @@ on: branches: - maintenance-9.x - maintenance-10.x + - release/9.1 paths: - 'src/**/*.c' - 'src/**/*.h' From f874de624a1425c2adbb8b91873e760f1b3260c3 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Thu, 17 Sep 2026 11:20:37 -0500 Subject: [PATCH 09/10] Fix PG checker missing renamed+layout-changed structs git diff --name-only collapses renames to the new path, so a persisted struct whose header was renamed and its layout changed never loaded its old definition and was silently excluded from the version-bump check. Use --no-renames so both old and new paths are collected, and add a rename regression fixture. --- .github/scripts/check-pg-versions.py | 2 +- .github/scripts/test-check-pg-versions.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/scripts/check-pg-versions.py b/.github/scripts/check-pg-versions.py index 2122215491a..448093dda06 100644 --- a/.github/scripts/check-pg-versions.py +++ b/.github/scripts/check-pg-versions.py @@ -146,7 +146,7 @@ def layout(lines, values): def check(base, head): base = git('merge-base', base, head).strip() - changed = [path for path in git('diff', '--name-only', base + '..' + head).splitlines() if path.endswith(('.c', '.h'))] + changed = [path for path in git('diff', '--no-renames', '--name-only', base + '..' + head).splitlines() if path.endswith(('.c', '.h'))] if not changed: print('No C/H files changed') return 0 diff --git a/.github/scripts/test-check-pg-versions.py b/.github/scripts/test-check-pg-versions.py index 548cfabdf4d..62632277914 100644 --- a/.github/scripts/test-check-pg-versions.py +++ b/.github/scripts/test-check-pg-versions.py @@ -50,6 +50,23 @@ def registration(v): assert result.returncode==expected,result.stdout+result.stderr +# A renamed + layout-changed struct header must still be checked: the old path's +# definition is otherwise lost when git diff collapses the rename to the new name. +with tempfile.TemporaryDirectory() as d: + def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() + p=pathlib.Path(d);git('init') + (p/'config.h').write_text('typedef struct config_s {\n int old;\n} config_t;\n') + (p/'config.c').write_text('PG_REGISTER_WITH_RESET_FN(config_t, config, PG_CONFIG, 1);\n') + git('add','.');git('commit','-m','base') + git('mv','config.h','renamed_config.h') + (p/'renamed_config.h').write_text('typedef struct config_s {\n int old;\n int added;\n} config_t;\n') + git('add','.');git('commit','-m','head') + env={k:v for k,v in os.environ.items() if k not in ('GITHUB_BASE_REF','GITHUB_HEAD_REF')} + result=subprocess.run(['bash',script],cwd=d,capture_output=True,text=True,env=env) + print('renamed header missing bump','exit',result.returncode,'expected',1) + assert result.returncode==1,result.stdout+result.stderr + + # Advancing the base branch must not make changes outside the PR look like removals. with tempfile.TemporaryDirectory() as d: def git(*a): return subprocess.run(['git','-c','user.name=CI Test','-c','user.email=ci@example.invalid',*a],cwd=d,check=True,capture_output=True,text=True).stdout.strip() From e47bf801ae21d617a6cdc58ec52ed0b8d528bc05 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Thu, 17 Sep 2026 11:51:20 -0500 Subject: [PATCH 10/10] Post PG version warnings from a workflow_run job for fork PRs The pull_request workflow's GITHUB_TOKEN is read-only for fork-originated runs, so its github-script comment step failed with 403 and hard-failed the job instead of warning. Move comment posting to a privileged workflow_run consumer: the check workflow uploads the checker output and PR number as artifacts, and the consumer downloads them and posts/updates the comment (mirroring pr-test-builds.yml). --- .../workflows/pg-version-check-comment.yml | 122 ++++++++++++++++++ .github/workflows/pg-version-check.yml | 99 ++------------ 2 files changed, 136 insertions(+), 85 deletions(-) create mode 100644 .github/workflows/pg-version-check-comment.yml diff --git a/.github/workflows/pg-version-check-comment.yml b/.github/workflows/pg-version-check-comment.yml new file mode 100644 index 00000000000..5481b22e580 --- /dev/null +++ b/.github/workflows/pg-version-check-comment.yml @@ -0,0 +1,122 @@ +name: Parameter Group Version Check Comment + +# Posts the PG version warning comment from a privileged context so it also works +# for PRs opened from forks, whose pull_request GITHUB_TOKEN is read-only. The +# unprivileged "Parameter Group Version Check" workflow uploads the checker output +# and PR number as artifacts; this workflow only downloads them — it never executes +# pull-request code. +on: + workflow_run: + workflows: ["Parameter Group Version Check"] + types: [completed] + +jobs: + comment: + runs-on: ubuntu-latest + # Only act on pull_request-triggered runs that succeeded (a checker error fails + # the check job, so its conclusion is not 'success'). + if: > + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' + permissions: + actions: read + issues: write + pull-requests: write + + steps: + - name: Download PG check result + uses: actions/download-artifact@v4 + with: + name: pg-check-result + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Post or update comment + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const prNumber = Number(fs.readFileSync('pr_number.txt', 'utf8').trim()); + const output = fs.readFileSync('pg_output.txt', 'utf8'); + + if (!output.includes('PG version issue:')) { + console.log('No PG version issues to report; nothing to do.'); + return; + } + + let issuesContent = ''; + + try { + // Extract issues from output (everything after the warning line) + const lines = output.split('\n'); + let capturing = false; + let issues = []; + + for (const line of lines) { + if (line.includes('PG version issue:')) { + capturing = true; + } + if (capturing) { + issues.push(line); + } + } + + issuesContent = issues.join('\n'); + } catch (err) { + console.log('Error capturing issues:', err); + issuesContent = '*Unable to extract detailed issues*'; + } + + const commentBody = '## ⚠️ Parameter Group Version Check\n\n' + + 'The following parameter groups may need version increments:\n\n' + + issuesContent + '\n\n' + + '**Why this matters:**\n' + + 'Modifying PG struct fields without incrementing the version can cause settings corruption when users flash new firmware. The `pgLoad()` function validates versions and will use defaults if there\'s a mismatch, preventing corruption.\n\n' + + '**When to increment the version:**\n' + + '- ✅ Adding/removing fields\n' + + '- ✅ Changing field types or sizes\n' + + '- ✅ Reordering fields\n' + + '- ✅ Adding/removing packing attributes\n' + + '- ❌ Only changing default values in `PG_RESET_TEMPLATE`\n' + + '- ❌ Only changing comments\n\n' + + '**Reference:**\n' + + '- [Parameter Group Documentation](../docs/development/parameter_groups/)\n' + + '- Example: [PR #11236](https://github.com/iNavFlight/inav/pull/11236) (field removal requiring version increment)\n\n' + + '---\n' + + '*This is an automated check. False positives are possible. If you believe the version increment is not needed, please explain in a comment.*'; + + try { + // Check if we already commented + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + + const botComment = comments.find(comment => + comment.user.login === 'github-actions[bot]' && + comment.body.includes('Parameter Group Version Check') + ); + + if (botComment) { + // Update existing comment + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body: commentBody + }); + console.log('Updated existing PG version check comment'); + } else { + // Post new comment + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: commentBody + }); + console.log('Posted new PG version check comment'); + } + } catch (err) { + core.setFailed(`Failed to post comment: ${err}`); + } diff --git a/.github/workflows/pg-version-check.yml b/.github/workflows/pg-version-check.yml index edf0dcdf904..12cdd96964d 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -20,7 +20,6 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - pull-requests: write steps: - name: Checkout PR code @@ -55,90 +54,20 @@ jobs: GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_HEAD_REF: ${{ github.head_ref }} - - name: Post comment if issues found - if: steps.pg_check.outputs.exit_code == '1' - uses: actions/github-script@v7 + - name: Stage PG check result for the comment workflow env: - # Passed through the environment: inlining the multi-line script output - # into the JavaScript source breaks the string literal (SyntaxError). + # Multi-line output travels through the environment so shell + # metacharacters in it are not reinterpreted. PG_CHECK_OUTPUT: ${{ steps.pg_check.outputs.output }} - with: - script: | - // Use the captured output from the previous step - const output = process.env.PG_CHECK_OUTPUT || ''; - let issuesContent = ''; - - try { - // Extract issues from output (everything after the warning line) - const lines = output.split('\n'); - let capturing = false; - let issues = []; - - for (const line of lines) { - if (line.includes('PG version issue:')) { - capturing = true; - } - if (capturing) { - issues.push(line); - } - } - - issuesContent = issues.join('\n'); - } catch (err) { - console.log('Error capturing issues:', err); - issuesContent = '*Unable to extract detailed issues*'; - } - - const commentBody = '## ⚠️ Parameter Group Version Check\n\n' + - 'The following parameter groups may need version increments:\n\n' + - issuesContent + '\n\n' + - '**Why this matters:**\n' + - 'Modifying PG struct fields without incrementing the version can cause settings corruption when users flash new firmware. The `pgLoad()` function validates versions and will use defaults if there\'s a mismatch, preventing corruption.\n\n' + - '**When to increment the version:**\n' + - '- ✅ Adding/removing fields\n' + - '- ✅ Changing field types or sizes\n' + - '- ✅ Reordering fields\n' + - '- ✅ Adding/removing packing attributes\n' + - '- ❌ Only changing default values in `PG_RESET_TEMPLATE`\n' + - '- ❌ Only changing comments\n\n' + - '**Reference:**\n' + - '- [Parameter Group Documentation](../docs/development/parameter_groups/)\n' + - '- Example: [PR #11236](https://github.com/iNavFlight/inav/pull/11236) (field removal requiring version increment)\n\n' + - '---\n' + - '*This is an automated check. False positives are possible. If you believe the version increment is not needed, please explain in a comment.*'; - - try { - // Check if we already commented - const { data: comments } = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - }); - - const botComment = comments.find(comment => - comment.user.login === 'github-actions[bot]' && - comment.body.includes('Parameter Group Version Check') - ); + run: | + echo "${{ github.event.pull_request.number }}" > pr_number.txt + printf '%s\n' "$PG_CHECK_OUTPUT" > pg_output.txt - if (botComment) { - // Update existing comment - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: botComment.id, - body: commentBody - }); - console.log('Updated existing PG version check comment'); - } else { - // Post new comment - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: commentBody - }); - console.log('Posted new PG version check comment'); - } - } catch (err) { - core.setFailed(`Failed to post comment: ${err}`); - } + - name: Upload PG check result + uses: actions/upload-artifact@v4 + with: + name: pg-check-result + path: | + pr_number.txt + pg_output.txt + retention-days: 1