Skip to content

GitHub clang scan build portability - #34275

Open
jamesmharvey wants to merge 2 commits into
ArduPilot:masterfrom
Float-Cargo:github-clang-scan-build-portability
Open

GitHub clang scan build portability#34275
jamesmharvey wants to merge 2 commits into
ArduPilot:masterfrom
Float-Cargo:github-clang-scan-build-portability

Conversation

@jamesmharvey

Copy link
Copy Markdown
Contributor

Summary

Make clang-scan-build CI derive temporary and artifact paths from the current checkout so the workflow also runs in renamed forks.

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

The configured pre-commit hooks pass for both changed files, including YAML validation, flake8, Ruff and codespell. Tools/scripts/check_branch_conventions.py --base-branch origin/master also passes.

A local regression harness relocated ProcessScanBuildOutput.repo_root to a temporary directory named renamed-fork and verified that the report directory and captured stdout were archived beneath that repository's tmp/scan-build directory.

The failure is also demonstrated by the same commit running in two repositories: the renamed fork fails, while ArduPilot/ardupilot succeeds. A complete patched GitHub Actions run remains to be performed after pushing the branch.

Description

autotest.py exports TMPDIR as the checkout's tmp directory. The clang-scan-build workflow instead created that directory under /__w/ardupilot/ardupilot, and the report processor and artifact upload repeated the same canonical-repository path. A fork with another repository name therefore gave scan-build a TMPDIR that did not exist. LLVM's scan-build diagnostic misleadingly reports this as an inaccessible /tmp directory even when another TMPDIR was selected.

Use GITHUB_WORKSPACE for workflow paths and the report processor's discovered repository root for its archive destination. Canonical-repository behavior is unchanged, while renamed forks keep all reports inside their own checkout

Derive the fixed archive destination from the discovered repository root
instead of a GitHub Actions path containing the canonical repository
name. This keeps reports inside the checkout when a fork is renamed.

AI-assisted: root cause analysis and patch drafted with OpenAI Codex.
autotest sets TMPDIR to the checkout tmp directory, but the workflow
creates and uploads reports through a path containing the canonical
repository name. Forks with another name therefore point scan-build at
a directory that does not exist.

Use GitHub's workspace path so the job is independent of repository
name.

AI-assisted: root cause analysis and patch drafted with OpenAI Codex.

@peterbarker peterbarker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tridge

tridge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_03/devcall_pr_reviews.html#pr34275

Reviewed at head 000e93350c. COMMENT — the portability fix works, but it switches on a code path that was previously dead outside CI.

The stated goal is achieved

The hardcoded /__w/ardupilot/ardupilot/tmp embeds the repository name twice and breaks on a renamed fork. A cross-check ran the current head under a renamed fork and the archive landed correctly under /__w/ardupilot-upstream/ardupilot-upstream, with counts matching the canonical run exactly (74 unique, 38 excluded, 36 in scope, 36 suppressed, 0 remaining).

archive_rename() was a no-op locally and now isn't

archive_rename() guards itself with if not os.path.isdir(os.path.dirname(dest)): return, and its docstring calls this "a no-op outside CI" — true only because /__w/ardupilot/ardupilot/tmp never exists on a developer machine. With dest now derived from the checkout, the guard passes whenever <repo>/tmp exists, including on a second local run — which scan_build_suppressions.py:47 explicitly documents people doing.

Reproduced: two sequential archives, and because shutil.move onto an existing directory nests rather than replaces, the second run ended up under scan-build/scan-second/. The non-recursive glob at process_scan_build_output.py:100 then found only 1 plist where recursive enumeration found both — so a repeat local run reads the current stdout while analysing the previous run's reports. Silently reduced coverage, failing in the direction of reporting fewer problems.

Gating the archive on an explicit CI signal, or refusing/replacing an existing destination, closes it.

Minor

Three mechanisms now derive one path where there used to be one literal: the run: step uses shell $GITHUB_WORKSPACE, upload-artifact uses the expression ${{ github.workspace }}, and the Python uses os.path.realpath(__file__/../..) — and only the third resolves symlinks. They agree in this container today, so it isn't a live defect; worth noting only because upload-artifact has if: always() and would warn rather than fail if they ever diverged, so the loss would be quiet.

CI still pending (49 jobs).

@github-actions github-actions Bot added CI github_actions Pull requests that update GitHub Actions code Python labels Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIReview CI github_actions Pull requests that update GitHub Actions code Python

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants