diff --git a/.github/scripts/check-pg-versions.py b/.github/scripts/check-pg-versions.py index b6964d05d6d..448093dda06 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 @@ -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 888bd851d12..62632277914 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: @@ -49,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() @@ -64,3 +82,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-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 c9caac7eaaf..bf0d3887670 100644 --- a/.github/workflows/pg-version-check.yml +++ b/.github/workflows/pg-version-check.yml @@ -19,7 +19,6 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - pull-requests: write steps: - name: Checkout PR code @@ -42,7 +41,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 @@ -54,90 +53,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('###')) { - 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