ci: run test-each-commit on merge to master

willcl-ark commented at 1:13 pm on March 20, 2025: member

Previously this CI job was checking out the head ref, which meant it is not being run on the result of merging into master.

Use the default checkout action ref (merge into default branch) to avoid this. Increment the checkout depth to HEAD~~ to compensate for the new merge commit.

This would have likely helped avoid both failures reported in #31946, and seems to me to be a more robust way of running this test job in any case.

(Probably) closes: #31946

cc @maflcko: Is there a historical reason I’m missing here why a merge-checkout was not chosen for this job in fafcd2e9ef1209d614de5763a2733098537919dd?

ci: run test-each-commit on merge to master

Previously this CI job was checking out the head ref, which meant it was
not being run on the result of merging into master.

Use the default checkout action ref (merge to default branch) to avoid
this. Increment the checkout depth to HEAD~~ to compensate for the new
merge commit.

84dc7533ff

DrahtBot commented at 1:13 pm on March 20, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32103.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label Tests on Mar 20, 2025

in .github/workflows/ci.yml:37 in 84dc7533ff

33@@ -34,14 +34,13 @@ jobs:
34         run: echo "FETCH_DEPTH=$((${{ github.event.pull_request.commits }} + 2))" >> "$GITHUB_ENV"
35       - uses: actions/checkout@v4
36         with:
37-          ref: ${{ github.event.pull_request.head.sha }}
38           fetch-depth: ${{ env.FETCH_DEPTH }}

maflcko commented at 1:51 pm on March 20, 2025:

wouldn’t this also need to increase fetch depth?

maflcko commented at 1:58 pm on March 20, 2025: member

Not sure I understand the goal nor the changes here.

It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.

However, you check out the previous commit, so overall this change is a no-op , unless I am missing something?

Maybe you wanted to instead rebase the changes onto master? However, this is not possible in general, as some commits are not possible to rebase. For example, a subtree merge commit is not possible to rebase.

Also, the goal of this check is to find bisect errors early, so the whole point is to run on the original changes, not merged into master.

Thinking about alternative fixes, obviously the best fix would be to not have any intermittent issues. Absent that, it would be good for devs to rebase on latest master if they want to ensure the CI gets all the latest fixes. Alternatively, failures in this task could be ignored or the task could be re-run.

willcl-ark commented at 2:45 pm on March 20, 2025: member

It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.

Correct. Whilst we can try to rely on developers keeping up-to-date with rebases on master it seems less error-prone to do it as part of the task.

However, you check out the previous commit, so overall this change is a no-op , unless I am missing something?

Maybe you wanted to instead rebase the changes onto master? However, this is not possible in general, as some commits are not possible to rebase. For example, a subtree merge commit is not possible to rebase.

Yes you’re right, I think the correct checkout command would in fact be git checkout HEAD^2 as HEAD~~ does indeed skip back to the previous merge commit, which is not what was intended here. AFAIK a merge would work with a subtree merge commit?

Also, the goal of this check is to find bisect errors early, so the whole point is to run on the original changes, not merged into master.

Thinking about alternative fixes, obviously the best fix would be to not have any intermittent issues. Absent that, it would be good for devs to rebase on latest master if they want to ensure the CI gets all the latest fixes. Alternatively, failures in this task could be ignored or the task could be re-run.

Fair enough. Ignoring failures in this task seems a little sub-optimal to me though.

Another alternative could be to auto-prompt a rebase on failure, something like:

 0diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
 1index 4f0ed47b951..9e75bf0a258 100644
 2--- a/.github/workflows/ci.yml
 3+++ b/.github/workflows/ci.yml
 4@@ -69,11 +69,18 @@ jobs:
 5           sudo apt-get update
 6           sudo apt-get install clang ccache build-essential cmake pkgconf python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libzmq3-dev qtbase5-dev qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y
 7       - name: Compile and run tests
 8+        id: run_tests
 9         run: |
10           # Run tests on commits after the last merge commit and before the PR head commit
11           # Use clang++, because it is a bit faster and uses less memory than g++
12           git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --stop-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999" ${{ env.TEST_BASE }}
13 
14+      - name: Suggest rebase
15+        if: failure() && steps.run_tests.outcome == 'failure'
16+        run: |
17+          echo "If you haven't already, please try rebasing your branch on the latest master"
18+          exit 1
19+
20   macos-native-arm64:
21     name: ${{ matrix.job-name }}
22     # Use any image to support the xcode-select below, but hardcode version to avoid silent upgrades (and breaks).

But it sounds like it would be best to close this (and #31946 ? ) ?

maflcko commented at 2:52 pm on March 20, 2025: member

It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.

Correct. Whilst we can try to rely on developers keeping up-to-date with rebases on master it seems less error-prone to do it as part of the task.

It is possible to write code in a commit that compiles fine on master, but not when compiling the commit itself. (The reverse of a silent merge conflict) Though, there only was one instance of this. More likely would be that devs are wholesale not compiling/testing intermediate commits, which would also be covered if the changes are “rebased”.

Yes you’re right, I think the correct checkout command would in fact be git checkout HEAD^2 as HEAD~~ does indeed skip back to the previous merge commit, which is not what was intended here. AFAIK a merge would work with a subtree merge commit?

I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with git?

Fair enough. Ignoring failures in this task seems a little sub-optimal to me though.

Another alternative could be to auto-prompt a rebase on failure, something like: Details But it sounds like it would be best to close this (and #31946 ? ) ?

Yeah, I meant “manually ignoring”, not silently ignoring the return code. :sweat_smile:

Other than that, no strong opinion. I think anything is fine.

maflcko commented at 4:10 pm on March 20, 2025: member

I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with git?

To clarify, my suggestion would be to modify the git rebase --exec 'run_test' base into git rebase --exec 'git log -1 && git merge --no-commit master && echo run_test && git merge --abort' base. However, I haven’t tested this.

willcl-ark commented at 11:24 pm on March 20, 2025: member

I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with git?

To clarify, my suggestion would be to modify the git rebase --exec 'run_test' base into git rebase --exec 'git log -1 && git merge --no-commit master && echo run_test && git merge --abort' base. However, I haven’t tested this.

Would it not in that case be simpler to checkout the PR HEAD commit, fetch and rebase onto master, before doing a more “normal” rebase from the MERGE_BASE? Something like:

 0#!/usr/bin/env bash
 1set -x
 2
 3git switch master
 4
 5# Create a dummy branch with a few empty commits
 6git switch -c dummy-branch
 7for i in {1..5}; do
 8    git commit --allow-empty -m "empty commit $i"
 9done
10
11# This state we are in here currently is the same as the PR HEAD commit we currently check out
12
13# Rebase these commits onto latest master (origin in the CI, may be upstream locally)
14git pull --rebase origin master
15git log --oneline -n7
16
17# This checks out "empty commit 4"
18git checkout HEAD~
19MERGE_BASE=$(git rev-list -n1 --merges HEAD)
20EXCLUDE_MERGE_BASE_ANCESTORS=
21
22# MERGE_BASE can be empty due to limited fetch-depth
23if test -n "$MERGE_BASE"; then
24    EXCLUDE_MERGE_BASE_ANCESTORS=^${MERGE_BASE}^@
25fi
26TEST_BASE=$(git rev-list -n7 --reverse HEAD "$EXCLUDE_MERGE_BASE_ANCESTORS" | head -1)
27
28echo would run: git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --stop-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999" "$TEST_BASE"
29
30git branch -D dummy-branch

This gets the following log output for me, which appears to be at least what I would expect this CI job to be doing:

 0$ ./checkout.sh
 1+ git switch master
 2Already on 'master'
 3Your branch is up to date with 'origin/master'.
 4+ git switch -c dummy-branch
 5Switched to a new branch 'dummy-branch'
 6+ for i in {1..5}
 7+ git commit --allow-empty -m 'empty commit 1'
 8[dummy-branch 58ecc5950e8] empty commit 1
 9
10<snip>
11
12+ git pull --rebase origin master
13From https://github.com/willcl-ark/bitcoin
14 * branch                    master     -> FETCH_HEAD
15Current branch dummy-branch is up to date.
16+ git log --oneline -n7
17af33d1bef61 (HEAD -> dummy-branch) empty commit 5
1882be04b802c empty commit 4
197f0435dc544 empty commit 3
20d82729928f9 empty commit 2
2158ecc5950e8 empty commit 1
22998386d4462 (upstream/master, upstream/HEAD, origin/master, master) Merge bitcoin/bitcoin#31866: test, refactor: Add TestNode.binaries to hold binary paths
23aa87e0b4460 Merge bitcoin/bitcoin#31519: refactor: Use std::span over Span
24+ git checkout HEAD~
25HEAD is now at 82be04b802c empty commit 4
26++ git rev-list -n1 --merges HEAD
27+ MERGE_BASE=998386d4462f5e06412303ba559791da83b913fb
28+ EXCLUDE_MERGE_BASE_ANCESTORS=
29+ test -n 998386d4462f5e06412303ba559791da83b913fb
30+ EXCLUDE_MERGE_BASE_ANCESTORS='^998386d4462f5e06412303ba559791da83b913fb^@'
31++ git rev-list -n7 --reverse HEAD '^998386d4462f5e06412303ba559791da83b913fb^@'
32++ head -1
33+ TEST_BASE=998386d4462f5e06412303ba559791da83b913fb
34++ nproc
35++ nproc
36++ nproc
37+ echo would run: git rebase --exec 'echo Running test-one-commit on $( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='\''-Wno-error=unused-member-function'\'' && cmake --build build -j 16 && ctest --output-on-failure --stop-on-failure --test-dir build -j 16 && ./build/test/functional/test_runner.py -j 32 --combinedlogslen=99999999' 998386d4462f5e06412303ba559791da83b913fb
38would run: git rebase --exec echo Running test-one-commit on $( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j 16 && ctest --output-on-failure --stop-on-failure --test-dir build -j 16 && ./build/test/functional/test_runner.py -j 32 --combinedlogslen=99999999 998386d4462f5e06412303ba559791da83b913fb
39+ git branch -D dummy-branch
40Deleted branch dummy-branch (was af33d1bef61).

maflcko commented at 7:21 am on March 21, 2025: member

Would it not in that case be simpler to checkout the PR HEAD commit, fetch and rebase onto master

Again, I don’t think this is possible.

In your example the rebase succeeds, because it is not needed. See:

0git pull --rebase origin master
1From https://github.com/willcl-ark/bitcoin
2 * branch                    master     -> FETCH_HEAD
3Current branch dummy-branch is up to date.

However, in the general case, it is not possible to rebase all commits of all pull requests on top of current master, even if the pull request merges fine into current master. For example:

One commit in the pull reqeust could be changing a file that was deleted/modified in master and the next commit fully or partially reverts those changes (for whatever reason). A merge with master may succeed, but a rebase may fail.
Another example would be a subtree merge (or any unclean merge commit) as part of the pull request.

With exact steps to reproduce:

0sh-5.2$ git fetch origin 0e8254fef9f0d37f88d1f077f0165e7febd3bfa0  # subtree merge
1...
2 * branch                  0e8254fef9f0d37f88d1f077f0165e7febd3bfa0 -> FETCH_HEAD
3sh-5.2$ git checkout 0e8254fef9f0d37f88d1f077f0165e7febd3bfa0
4...
5HEAD is now at 0e8254fef9 Update secp256k1 subtree to latest master
6sh-5.2$ git rebase 998386d4462  # master
7...
8Could not apply dd59896431... Squashed 'src/secp256k1/' changes from 0cdc758a56..70f149b9a1

ci: run test-each-commit on merge to master #32103

Code Coverage & Benchmarks

Reviews