From 5f0bae7bace663950fe588071c26d2dae0dfe7eb Mon Sep 17 00:00:00 2001 From: Raphael Hunziker Date: Wed, 9 Sep 2026 15:47:03 +0200 Subject: [PATCH 1/6] 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 3d624bc4c49c0f71a998e6527efa15eefe703f44 Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Wed, 9 Sep 2026 18:02:59 +0200 Subject: [PATCH 2/6] 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 2a5f080bc141d568235eebde2e01b51bdc55f594 Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Thu, 10 Sep 2026 17:32:01 +0200 Subject: [PATCH 3/6] 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 5351f640361c1e94705f9bb6247520b3d7e08788 Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Thu, 10 Sep 2026 17:35:50 +0200 Subject: [PATCH 4/6] 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 9f2c63ce0e8b13a736b41355bb19c2eb9c71dd25 Mon Sep 17 00:00:00 2001 From: Raffi1202 Date: Fri, 11 Sep 2026 18:28:29 +0200 Subject: [PATCH 5/6] 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 6c1f6daaecd1cc0a82c3bfe47a77e80b924e4b0c Mon Sep 17 00:00:00 2001 From: Raphael Hunziker Date: Sun, 13 Sep 2026 21:25:02 +0200 Subject: [PATCH 6/6] 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 d8f486386c1..283679524dd 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 @@ -166,7 +171,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