Skip to content

ci: fix changed formula discovery - #123

Open
MeteorsLiu wants to merge 4 commits into
xgo-dev:mainfrom
MeteorsLiu:fix/formula-ci-discovery
Open

ci: fix changed formula discovery#123
MeteorsLiu wants to merge 4 commits into
xgo-dev:mainfrom
MeteorsLiu:fix/formula-ci-discovery

Conversation

@MeteorsLiu

@MeteorsLiu MeteorsLiu commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Fix formula CI so it tests only modules changed on the current branch.

The implementation includes:

  • Replace the inline Bash scanner with .github/scripts/find_changed_modules.gsh, executed through ixgo v1.1.1.
  • Diff pull requests and feature branches from their merge base while preserving before..after behavior for default-branch pushes.
  • Deduplicate changed modules and reject changes spanning multiple Formula directories for one module until llar test supports version ranges.
  • Include deletions and fail when Formula files remain without versions.json, while allowing complete module removals.
  • Validate the original Fenno crc32c branch shape across all four runners; only google/crc32c was scheduled.

This prevents unrelated formulas from entering the CI matrix and turns invalid module metadata into an explicit failure.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review summary

This PR replaces the inline bash "find changed modules" step with a typed XGo script (find_changed_modules.gsh) run via ixgo, and simplifies the zlib formula. The rewrite is a clear readability win and the script correctly avoids shell-injection in its own git calls (env vars bound to XGo variables, NUL-delimited diff parsing, json.marshal for outputs). Docs (.claude vs .agents copies) are in sync and the new Zlib_llar.gox matches the documented formula API.

Main items below. The most impactful is a first-push regression on the default branch.

Findings (see inline comments)

  • find_changed_modules.gsh:49 — default-branch first-push all-zero before SHA is no longer handled (regression).
  • find_changed_modules.gsh:73 — deletion handling (ACMRTACDMRT) relies on re-probing the filesystem; fragile.
  • test.yml:43ixgo built from source uncached on the critical path of every run.

Additional note (no reliable inline location)

  • Pre-existing shell-injection surface, test.yml:100-101,112 (not introduced by this PR, but this PR reworks the discovery that feeds it): ${{ matrix.module }} is interpolated directly into run: shell scripts, and matrix.module is derived entirely from PR-controlled file paths (parts[0]+"/"+parts[1]). A PR adding a path like `evil`/name/versions.json could inject shell into the test job (public repo, pull_request event, contents: read limits blast radius but arbitrary code execution on the runner remains). Recommend passing matrix.module via env: and referencing a quoted "$MODULE" in run:. Since this PR restructures module discovery, it's a good moment to also constrain/validate discovered module names.

Minor / optional

  • find_changed_modules.gsh:64 — the trailing empty token from -z NUL split is harmlessly skipped by the parts.len < 3 check today; an explicit empty-changedPath skip would be clearer.
  • find_changed_modules.gsh:71os.stat(versions.json) runs per changed path rather than per module; harmless at current diff sizes, could dedupe by module.
  • test.yml:43,105ixgo@v1.1.1 / llar are pinned to mutable tags and installed with -checklinkname=0; consider pinning to a commit and documenting why the linkname check is disabled.

Comment thread .github/scripts/find_changed_modules.gsh
versionsPath := filepath.join(module, "versions.json")
_, err := os.stat(versionsPath)
if os.isNotExist(err) {
has, walkErr := hasFormula(module)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Deletion handling relies on re-probing the filesystem (ACMRTACDMRT added D).

Including deletions means changedPath can reference paths no longer present at HEAD. To distinguish "module deleted" from "module misconfigured (missing versions.json)", the code stats versions.json and then walks the module dir, treating os.isNotExist(walkErr) as skip. That discrimination depends on filesystem state rather than the diff status of the path: a deleted module is silently dropped, while a module whose dir still exists but lost its versions.json panics. This is fragile — branching on the git delete status (e.g. git diff --name-status / a D marker) would be more robust and intentional than re-probing with os.stat + walkDir + os.isNotExist. Worth an explicit decision/comment on whether deletions should be in scope at all.

echo "has_modules=true" >> "$GITHUB_OUTPUT"
fi
echo "Changed modules: $modules_json" >> "$GITHUB_STEP_SUMMARY"
go install -tags=linknamefix -ldflags="-checklinkname=0" github.com/goplus/ixgo/cmd/ixgo@v1.1.1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ixgo is built from source, uncached, on the critical path of every run.

setup-go here uses cache: false (line 32) and this step does go install ...ixgo@v1.1.1 from source on every trigger. Since the test matrix has needs: changes, this uncached compile of the ixgo interpreter is added serially to the front of every PR/push. Enabling Go build/module caching (drop cache: false) or using a prebuilt/pinned binary would remove this repeated work from the hot path.

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.

1 participant