GitHub Actions don't build exactly the pushed commit

[hahnjo]

At the moment, push workflows pass --base_ref master to build_root.py. This means that they may actually pick up a later commit (= the current commit on the branch) at the time the job actually starts to run. In order to be usable as indications when a certain configuration broke, it would be more useful to test exactly the pushed commit, identified by its hash.

Unfortunately, fixing this isn't as easy as replacing

root/.github/workflows/root-ci.yml

Lines 456 to 465 in db6ff45

	- name: Update build cache after push to release branch
	if: github.event_name == 'push'
	run: ".github/workflows/root-ci-config/build_root.py
	--buildtype RelWithDebInfo
	--platform ${{ matrix.image }}
	--incremental false
	--base_ref ${{ github.ref_name }}
	--binaries ${{ startsWith(github.ref, 'refs/tags/') }}
	--repository ${{ github.server_url }}/${{ github.repository }}
	"

to pass --base_ref ${{ github.sha }} because build_root.py calls git clone --branch {branch} --single-branch {repository}. Passing a hash here doesn't work (in local tests), and would otherwise probably result in a detached HEAD. Another complication is that the sources are included in the build artifacts used by PR builds, so they have to look like an ordinary clone for the rebase operation to work.

[pcanal]

Actually, if/since it is passed by github, it wouldn't be so far. The cloning is just boot-strapping after that both the target branch and the incoming branch are being pulled.

Passing a hash here doesn't work (in local tests),

Yep, it is expected for PR to be of the form: refs/pull/14052/head:ci-rootest-cmake and both part are being used.

[UPDATE: "it" was the head_ref rather than the base_ref]

This should be straight-forward to improve.

[pcanal]

Side Notes

to pass --base_ref ${{ github.sha }} "

in build_root.py, the base_ref is the target branch in the origin (master, v6-32-00-patches) while head_ref is pointing to the new commits to be merged in (which I think is what you meant to correct/fix).

[hahnjo]

No, this is a misunderstanding: I mean the push workflows that run when commits are pushed to master. These only have base_ref, no head_ref, and produce the build artifacts for the PR builds that I mentioned above.

The PR builds probably have a similar problem for the head_ref, but it's less of an issue since for PRs we always want the latest version anyway.

[pcanal]

Related problem that I don't think I can fix (at least I don't see an obvious way). Is that if the target branch (for example master) is updated while the CI is running only the not-yet started runners will use the newest commit.

[pcanal]

Ah .. in this case, this is a little more involved but not that bad either. We would need to pass github.sha in addition to the current arguments and reset the branch to that after cloning.

[hahnjo]

Sure, I'm not saying it's impossible, just that the potentially obvious approach to pass the hash as base_ref doesn't work. A git reset ${ github.sha } might work, to be tested how it interacts with the rest of the system.

[pcanal]

I think this is addressed within #14052 (already merged)
and in particular 8c17cbf (#14052)

[hahnjo]

I think this is addressed within #14052 (already merged) and in particular 8c17cbf (#14052)

That commit is about PR builds; in this issue, I'm discussing builds after pushes to branches. AFAICT they still use a git pull of the branch name, which may get you different commits depending on when the job is scheduled to start.

[pcanal]

Line 434 and 437 in the 8c17cbf (#14052) add the same logic to the "Pull Request" part. Are we still missing something?

[hahnjo]

Let me quote my answer from yesterday:

That commit is about PR builds; in this issue, I'm discussing builds after pushes to branches.

So maybe PR builds are now doing better than before, but that's entirely besides the point: A build triggered by a push to a branch should build exactly what was pushed, not some later commit.

AFAICT they still use a git pull of the branch name, which may get you different commits depending on when the job is scheduled to start.

[pcanal]

Right, I was looking in the wrong place. The part of root-ci.yml that still need to be updated is line 480 to 488 rather than the one I was refering to ...