Skip to content

ci: fix changed formula discovery - #123

Merged
luoliwoshang merged 4 commits into
xgo-dev:mainfrom
MeteorsLiu:fix/formula-ci-discovery
Aug 3, 2026
Merged

ci: fix changed formula discovery#123
luoliwoshang merged 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
Contributor

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
Contributor

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
Contributor

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.

@luoliwoshang

Copy link
Copy Markdown
Collaborator

之前的bash是扫不到什么内容吗?

@MeteorsLiu

Copy link
Copy Markdown
Collaborator Author

之前的bash是扫不到什么内容吗?

See #117

It scan the wrong formula.

@luoliwoshang

Copy link
Copy Markdown
Collaborator

这个扫描脚本功能就是,获取当前base,然后和main作比较,得出差异后去扫描出实际formula目录

是目标某个源定时检查上游库有没有更新么?

@MeteorsLiu

MeteorsLiu commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

这个扫描脚本功能就是,获取当前base,然后和main作比较,得出差异后去扫描出实际formula目录

是目标某个源定时检查上游库有没有更新么?

The issue was that CI used:

git diff "$BASE_SHA" "$HEAD_SHA"

For this PR, the head branch was forked from 331eeee, while the base branch had advanced to 1790880. Comparing those two snapshots also included changes made on the base branch after the fork, such as the madler/zlib changes, so CI incorrectly treated zlib as changed.

We now compute the merge base first and diff from it:

git diff "$(git merge-base "$BASE_SHA" "$HEAD_SHA")" "$HEAD_SHA"

This uses the actual fork point and includes only changes introduced by the PR. It correctly selects only google/crc32c, matching GitHub's three-dot PR diff semantics.

@luoliwoshang luoliwoshang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@luoliwoshang
luoliwoshang merged commit 39a4b3f into xgo-dev:main Aug 3, 2026
2 checks passed
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