Skip to content

Add github actions CI that will test building ADTimePix3 on RHEL 8 through 10#7

Open
jwlodek wants to merge 1 commit intoareaDetector:masterfrom
jwlodek:add-ci
Open

Add github actions CI that will test building ADTimePix3 on RHEL 8 through 10#7
jwlodek wants to merge 1 commit intoareaDetector:masterfrom
jwlodek:add-ci

Conversation

@jwlodek
Copy link
Copy Markdown
Member

@jwlodek jwlodek commented Apr 13, 2026

No description provided.

@kgofron
Copy link
Copy Markdown
Member

kgofron commented Apr 16, 2026

PR #7: GitHub Actions CI (EL8–EL10) + pixi / pre-commit scaffolding

What changed (from diff)

  • CI workflow .github/workflows/ci.yaml:
    • lint job: pixi run pre-commit run --all-files on ubuntu-latest.
    • build-and-test job: matrix over EL 8/9/10, runs inside ghcr.io/nsls2/epics-alma{8,9,10}:latest, generates configure/CONFIG_SITE.local and configure/RELEASE.local, installs build deps via dnf, runs make -j4.
  • pixi.toml and a large pixi.lock (marked generated/binary via .gitattributes) to pin pre-commit + clang-format.
  • .pre-commit-config.yaml (pre-commit-hooks set, excludes ^tpx3Support/).
  • .gitignore updated to ignore .pixi/* (except .pixi/config.toml).

Impact

  • Large positive: Repeatable, multi-distro build checks that catch “works on my machine” drift early—especially useful for EPICS/module builds.
  • Repo hygiene: Baseline pre-commit for whitespace / EOF / YAML / JSON / XML sanity, which tends to reduce noisy diffs and CI failures.

Critical issues to fix before merge

  1. Wrong default branch in workflow: Push CI is configured for main, but this repo uses master (and the PR targets master). As written, push CI may not run on the primary branch.
    Fix: Set on.push.branches to master (or list both).

  2. Invalid checkout action version: The build job uses actions/checkout@v6, which is not a valid major release as of now; v4 is the current stable major line.
    Fix: Use actions/checkout@v4 in both jobs.

Likely CI breakages / fragility points

  • Root-in-container + Git safety: Running as root in a container can trigger Git’s “dubious ownership” / safe.directory checks. If that happens, add a step such as:
    git config --global --add safe.directory "$GITHUB_WORKSPACE".

  • Assumptions about EPICS layout: Generating configure/RELEASE.local with paths under /usr/lib64/epics may or may not match the image’s layout and versions (and may omit areaDetector deps this driver needs). If epics-alma* images are curated for this stack, fine; otherwise expect early failures.

  • BUILD_TESTS=YES without running tests: The workflow compiles tests but does not run them. Still useful, but the job name “Build and Test” is misleading unless you add a test step—or rename to “Build”.

  • pixi + pre-commit scope:

    • clang-format is in the pixi environment but no clang-format hook is configured, so it is unused.
    • The current pre-commit set does not enforce C/C++ style unless you add e.g. clang-format or clang-tidy hooks.

Suggested improvements (pragmatic)

  • Cheaper / faster CI: Add concurrency to cancel superseded runs on the same PR/branch; consider enabling pixi caching (cache: false today), at least for the lint job.
  • Supply chain (optional): Prefix-dev setup-pixi is already pinned by commit SHA (good). Consider pinning actions/checkout by SHA as well if your org wants maximum hardening.
  • Generated files: .gitattributes marks pixi.lock as generated and limits diffs/merges—reasonable given size. Trade-off: dependency updates are harder to review.

Alternative approaches

  • pre-commit/action instead of pixi for lint — Simpler for contributors who already use Python/pre-commit; trade-off: less lockfile-style determinism than pixi unless tool versions are pinned carefully.
  • Dedicated Dockerfile or devcontainer instead of ad-hoc dnf — More reproducible, easier local iteration, pin OS packages; trade-off: more maintenance (image/Dockerfile updates).
  • Single “known-good” EPICS toolchain container (instead of EL8–10 matrix) — Faster CI, less matrix complexity; trade-off: less coverage for downstream sites on older/newer EL.

Bottom line

High-value PR, but please apply two critical correctness fixes (mainmaster and checkout@v6@v4). Next highest impact: either run tests in CI or rename the job to “Build” if tests are not executable yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants