Skip to content

CI: check dependency graph on fork PR heads - #285

Merged
matt-pharr merged 2 commits into
PrincetonUniversity:developfrom
krystophny:ci/fork-pr-deps-checkout
Oct 8, 2026
Merged

matt-pharr merged 2 commits into
PrincetonUniversity:developfrom
krystophny:ci/fork-pr-deps-checkout

Conversation

@krystophny

Copy link
Copy Markdown
Collaborator

The deps job currently checks out github.head_ref from the base repository. On every fork PR that branch is absent, so the job fails before generating the dependency graph even when all compiler jobs pass.

Check out the event's actual head repository and immutable SHA. workflow_dispatch retains the current-repository/current-SHA fallback. The existing push block already treats inability to push a regenerated file to a fork as a warning, as documented in the workflow comment.

This is intentionally separate from #284, which changes only two PENTRC spline boundary conditions and otherwise passes all GNU/Intel build jobs.

@logan-nc

logan-nc commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

@claude review this

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @logan-nc's task in 17s —— View job


Review of PR #285

The fix for fork PRs is correct, but it breaks the auto-commit push for same-repo PRs.

What works

  • github.event.pull_request.head.repo.full_name and head.sha resolve correctly on fork PRs, so the deps job now gets the head repo's code instead of failing on a missing branch.
  • On workflow_dispatch the || github.repository and || github.sha fallbacks apply, as described.
  • Pinning to the SHA is also safer than a branch name.

Issue: same-repo PRs can no longer push the regenerated file (.github/workflows/make_test.yml:21-22)

  • Checking out a raw SHA leaves a detached HEAD. The old ref: github.head_ref checked out the branch itself.
  • The later git commit -am ... && git push (line 36-37) then fails with fatal: You are not currently on a branch.
  • The failure falls into the else branch, which only emits the "Stale DEPENDENCIES.inc" warning.
  • So on same-repo PRs a stale DEPENDENCIES.inc is no longer auto-committed, and the job still passes. The header comment (lines 9-13) promises that behavior. The failure is silent, with only a warning.

Suggested fix: make the push target explicit. Using env avoids interpolating the ref name into the script:

      - name: Commit if the graph changed
        env:
          HEAD_REF: ${{ github.head_ref }}
        run: |
          ...
          if git push origin "HEAD:refs/heads/${HEAD_REF}"; then

github.head_ref is empty on workflow_dispatch, and the push there would also fail. The warning branch would still catch it, but you could guard with [ -n "$HEAD_REF" ].

On a fork PR the push still can't succeed, because the pull_request token is read-only. The warning path handles that, as the PR description says.

Minor

  • If the PR branch advances between checkout and push, the push is rejected as non-fast-forward and only a warning is emitted. That is acceptable, since the next run will fix it.
  • I couldn't test this: workflow edits are outside what I can modify here, and the checkout has no merge base with origin/develop. The review is from reading the diff and the full job.

@logan-nc

logan-nc commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

The above claude suggestion seems important, but I deffer to @matt-pharr for a official expert review

@krystophny

Copy link
Copy Markdown
Collaborator Author

@matt-pharr did the changes @logan-nc requested. Can you check?

@matt-pharr
matt-pharr merged commit 6a8d116 into PrincetonUniversity:develop Oct 8, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants