ci: Merge master in test-each-commit task (take 2)

maflcko commented at 6:42 am on April 4, 2025: member

Calling the script .github/ci-test-each-commit-exec.sh, which merges master, obviously doesn’t work, if the script itself is missing.

Fix it by a move-only to first merge master and then call the script.

DrahtBot commented at 6:42 am on April 4, 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/32218.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot renamed this:
~~ci: Merge master in test-each-commit task (take 2)~~
ci: Merge master in test-each-commit task (take 2)
on Apr 4, 2025

DrahtBot added the label Tests on Apr 4, 2025

maflcko force-pushed on Apr 4, 2025

maflcko commented at 7:06 am on April 4, 2025: member

I’ve re-created an ancient pull request in a different repo. If that passes, the stuff here should hopefully be fine: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/14259891774/job/39974233487?pr=6

maflcko force-pushed on Apr 4, 2025

maflcko commented at 9:42 am on April 4, 2025: member

(Both relevant CI tasks passed, so this should be ready for review now)

in .github/workflows/ci.yml:83 in fa74fb3f74 outdated

79       - run: |
80           sudo apt-get update
81           sudo apt-get install clang ccache build-essential cmake ninja-build pkgconf python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libzmq3-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev -y
82       - name: Compile and run tests
83         run: |
84           # Run tests on commits after the last merge commit and before the PR head commit

l0rinc commented at 9:59 am on April 4, 2025:

does the comment still make sense, the new script already explains this

maflcko commented at 11:25 am on April 4, 2025:

Not sure what you mean. I don’t think the new script explains this. The comment is unrelated to the new script. The comment explains ${{ env.TEST_BASE }}, which I am not changing in this pull.

l0rinc commented at 12:13 pm on April 4, 2025:

Run tests on commits after the last merge commit and before the PR head commit

This sounds to me like a more verbose explanation for ci-test-each-commit.

maflcko commented at 1:34 pm on April 4, 2025:

Idk, it was added in 88c8e3a0e4d6bee015a348536c6e12a2c7835896. It makes sense to document this, because it isn’t testing each commit, but it excludes merge commits (even if they are in the pull request). In any case, I’ll leave it as-is for now, because it is unrelated.

in .github/ci-test-each-commit-exec.sh:21 in fa74fb3f74 outdated

21 cmake --build build -j "$( nproc )" && ctest --output-on-failure --stop-on-failure --test-dir build -j "$( nproc )"
22 
23 ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999
24 
25-git merge --abort
26+git merge --abort || echo "Nothing to abort. The state was already up to date."

l0rinc commented at 10:02 am on April 4, 2025:

if we need to keep this change, maybe mention that the abort is related to merging

maflcko commented at 11:23 am on April 4, 2025:

if we need to keep this change, maybe mention that the abort is related to merging

Yes, the abort in git merge --abort is related to merging. I think the code is self-explanatory.

The CI log is also self-explanatory, because the code will print itself before being executed (xtrace).

You can see this by reading the CI logs of this pull and the one I linked to earlier.

in .github/workflows/ci.yml:84 in fa74fb3f74 outdated

81           sudo apt-get install clang ccache build-essential cmake ninja-build pkgconf python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libzmq3-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools libqrencode-dev -y
82       - name: Compile and run tests
83         run: |
84           # Run tests on commits after the last merge commit and before the PR head commit
85-          git rebase --exec "./.github/ci-test-each-commit-exec.sh" ${{ env.TEST_BASE }}
86+          git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && ./.github/ci-test-each-commit-exec.sh" ${{ env.TEST_BASE }}

l0rinc commented at 10:08 am on April 4, 2025:

we’re setting the author info in a separate step above - we could localize it as:

0          git rebase --exec "GIT_COMMITTER_NAME="CI" GIT_COMMITTER_EMAIL="ci@ci.com" git merge --no-commit origin/${GITHUB_BASE_REF} && ./.github/ci-test-each-commit-exec.sh" ${{ env.TEST_BASE }}

l0rinc commented at 10:29 am on April 4, 2025:

given that we initiate the merge here, would it make sense to do the abort here as well? And maybe we could do a reset instead which works regardless of whether we’re already up-to-date or not, e.g.:

0          git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && ./.github/ci-test-each-commit-exec.sh && git reset --hard" ${{ env.TEST_BASE }}

 0# GIT_COMMITTER_NAME="CI" GIT_COMMITTER_EMAIL="ci@ci.com" git merge master --no-commit
 1Automatic merge went well; stopped before committing as requested
 2
 3# git status
 4On branch feature
 5All conflicts fixed but you are still merging.
 6  (use "git commit" to conclude merge)
 7
 8Changes to be committed:
 9        new file:   xyz.txt
10
11# git reset --hard
12HEAD is now at f7c4340 first 6
13
14# git status
15On branch feature
16nothing to commit, working tree clean

maflcko commented at 11:26 am on April 4, 2025:

Happy to review a follow-up doing this refactor, if people want it. However, I currently don’t have the time to try if it works and if it is better.

maflcko commented at 11:27 am on April 4, 2025:

Happy to review a follow-up doing this refactor, if people want it. However, I currently don’t have the time to try if it works and if it is better.

edit: I tested this and it works locally, so it may work when I push it.

Happy to push it, if this is the last blocker for you.

l0rinc commented at 12:13 pm on April 4, 2025:

Happy to push it, if this is the last blocker for you.

yes, only the things I’ve posted

maflcko commented at 1:35 pm on April 4, 2025:

thx, pushed

l0rinc commented at 1:50 pm on April 4, 2025:

Please see the reproducer I posted to #32218 (review) - I think this would simplify the code further (untangle separate setup steps and reduce global state)

l0rinc commented at 1:54 pm on April 4, 2025:

should we reset if ci-test-each-commit-exec.sh returns with an error?

maflcko commented at 1:59 pm on April 4, 2025:

setup and global state is already required, because the master branch has to be cloned and stored for this. Storing a dummy name and email is harmless in any case.

maflcko commented at 2:02 pm on April 4, 2025:

should we reset if ci-test-each-commit-exec.sh returns with an error?

Why? The CI will discard itself after failure

l0rinc commented at 2:04 pm on April 4, 2025:

Yes, this would only reduce global state, not eliminate it completely.

l0rinc commented at 3:01 pm on April 4, 2025:

That’s why I asked, my understanding was that it doesn’t matter.

What about in case of merge conflict, what is the expected outcome, should the script still be executed?

And the script only measures a single commit, not “each commit”, is the naming deliberate?

maflcko commented at 3:06 pm on April 4, 2025:

I have a slight preference for the current way. Also, It is extremely tedious and time consuming to test the changes here remotely on GitHub in a separate repo on every tiny change. So I’ll leave this as-is.

If there is need for this refactor, it can be done in the future.

maflcko commented at 3:10 pm on April 4, 2025:

I am not changing the name of the script here, so it is unrelated and can be changed in a separate pull, if needed.

The name of the script simply includes the task name.

What about in case of merge conflict, what is the expected outcome, should the script still be executed?

No. Why? The CI will discard itself after it failed after a merge conflict, so there is no need to delay and execute anything.

l0rinc commented at 10:34 am on April 4, 2025: contributor

I wonder how this new requirement will perform for heavy refactors - but overall it’s a good change!

maflcko commented at 11:33 am on April 4, 2025: member

I wonder how this new requirement will perform for heavy refactors - but overall it’s a good change!

See #32203 (comment)

ci: Merge master in test-each-commit task (take 2)

* Run git config earlier and only once
* Run git merge in the yaml, before calling the bash script
* Run git reset in the yaml as well, for symmetry
* Replace "git merge --abort" with "git reset --hard", because it does
  not fail when already up to date and no merge was started.

fa0d0be05c

ci: Use GITHUB_BASE_REF over hard-coded master fa10a1ded5

maflcko force-pushed on Apr 4, 2025

l0rinc commented at 1:53 pm on April 4, 2025: contributor

Code review ACK fa10a1ded5b747e9db6d6c1942fceb279f1abedc

Would prefer the scope reduction suggested in #32218 (review) as well, but I think the current version is already better than before

edit: I haven’t tested the merge conflict or test failures scenarios

maflcko force-pushed on Apr 4, 2025

maflcko commented at 2:59 pm on April 4, 2025: member

Force pushed for new CI run in the remote repo: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/14268201238/job/39995098276?pr=6

(no changes here)

sipa commented at 11:28 am on April 5, 2025: member

ACK fa10a1ded5b747e9db6d6c1942fceb279f1abedc, this fixed the CI issue in #31444.

fanquake merged this on Apr 6, 2025

fanquake closed this on Apr 6, 2025

Jassor909 commented at 10:37 pm on April 6, 2025: none

0> ~~~~

maflcko deleted the branch on Apr 7, 2025

ci: Merge master in test-each-commit task (take 2) #32218

Code Coverage & Benchmarks

Reviews