cmake, ci: Migrate "test-each-commit" task by hebasto · Pull Request #288 · hebasto/bitcoin

hebasto · 2024-07-26T19:47:28Z

It was overlooked.

An example of the CI log: https://github.com/hebasto/bitcoin/actions/runs/10117122581/job/27981470109.

hebasto · 2024-07-26T21:20:00Z

Friendly ping @maflcko :)

m3dwards · 2024-07-29T09:57:43Z

Do you need to explicitly disable Berkeley DB? Doesn't it default to off anyway? Also related, do we need to still install libdb++-dev?

hebasto · 2024-07-29T10:15:32Z

@m3dwards

Do you need to explicitly disable Berkeley DB? Doesn't it default to off anyway?

Yes we do. We use the ci-linux preset, which defines:

bitcoin/CMakePresets.json

Line 13 in e4f7e12

"WITH_BDB": "ON",

Hence, the command line -DWITH_BDB=OFF overrides it to follow the master branch behavior, which can be observed, for instance, here:

 Options used to compile and link:
  external signer = yes
  multiprocess    = no
  with wallet     = yes
    with sqlite   = yes
    with bdb      = no
...

Also related, do we need to still install libdb++-dev?

It seems you found an inconsistency in the master branch. It should either pass the --with-incompatible-bdb configure option or skip installing libdb++-dev.

Want to submit a PR to the main repository?

m3dwards · 2024-07-29T10:18:44Z

Yes we do. We use the ci-linux preset, which defines

Of course, makes sense.

It seems you found an inconsistency in the master branch. It should either pass the --with-incompatible-bdb configure option or skip installing libdb++-dev.

Want to submit a PR to the main repository?

Sure

m3dwards · 2024-07-29T10:20:48Z

I compared the configure step output of the CI run linked in the PR description with a random run in the main repository and I couldn't find any differences.

ACK 37e2b42

maflcko

lgtm, seems fine to merge, but I left a few questions

maflcko · 2024-07-29T10:20:51Z

          # Run tests on commits after the last merge commit and before the PR head commit
          # Use clang++, because it is a bit faster and uses less memory than g++
-          git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && ./autogen.sh && CC=clang CXX=clang++ ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }}
+          git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build --preset ci-linux -DBUILD_GUI=ON -DWITH_BDB=OFF -DWITH_ZMQ=OFF -DWITH_QRENCODE=ON -DWERROR=OFF && cmake --build build -j $(nproc) && ctest --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.TEST_BASE }}


What is the point of using ci-linux here, when most settings are overridden anyway? Would it not be better to just list each setting here explicitly?

Also, what it the point of ci-common and ci-linux at all? All CI tasks are highly orthogonal, so creating the impression that the is common stuff among them seems odd?

Also, what it the point of ci-common and ci-linux at all? All CI tasks are highly orthogonal, so creating the impression that the is common stuff among them seems odd?

In the past, these presets seemed useful for .github/workflows/cmake.yml, which is not a part of bitcoin#30454 though.

Ok, I'd say that it may be best to remove CI-only code no longer used for now from 30454. Given that CI presets are exposed to real devs and real users, it may otherwise be confusing at best, no?

(No objection to keep them in a separate commit in a testing branch, if you find them useful, but not sure about adding them to 30454)

Ok, I'd say that it may be best to remove CI-only code no longer used for now from 30454.

I'll do it during the next PR30454 branch update.

What is the point of using ci-linux here, when most settings are overridden anyway? Would it not be better to just list each setting here explicitly?

Done in the recent push.

m3dwards · 2024-07-29T10:27:00Z

It should either pass the --with-incompatible-bdb configure option or skip installing libdb++-dev.

I would say the more conservative approach is to enable bdb in CI rather than skipping the installation which if accepted on master would imply enabling it here too. Perhaps as an update if this gets merged.

Migrate the "test-each-commit" task

hebasto · 2024-07-29T11:56:22Z

Addressed @maflcko's comment.

Included changes from bitcoin#30542.

m3dwards · 2024-07-29T12:04:36Z

@hebasto do you have a CI log I could take a quick scan of that has run the "test each commit" task?

hebasto · 2024-07-29T12:22:54Z

@hebasto do you have a CI log I could take a quick scan of that has run the "test each commit" task?

I'll push a dummy commit.

maflcko · 2024-07-29T12:24:23Z

 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.

+


The commit can be empty

(This way it can be merged, but it will be dropped on the next rebase/squash by itself)

Right! I forgot about --allow-empty option :)

hebasto · 2024-07-29T12:26:00Z

@hebasto do you have a CI log I could take a quick scan of that has run the "test each commit" task?

I'll push a dummy commit.

https://github.com/hebasto/bitcoin/actions/runs/10144199347/job/28047196932

hebasto · 2024-07-29T12:56:48Z

@hebasto do you have a CI log I could take a quick scan of that has run the "test each commit" task?

I'll push a dummy commit.

https://github.com/hebasto/bitcoin/actions/runs/10144199347/job/28047196932

Reverted back and ready for re-reviewing.

m3dwards · 2024-07-29T12:58:29Z

reACK 5ecbd3f

hebasto force-pushed the 240726-cmake-HR branch 3 times, most recently from fed140c to 37e2b42 Compare July 26, 2024 21:17

hebasto added bug Something isn't working tests Tests, benchmarks, fuzzing, CI etc labels Jul 26, 2024

hebasto marked this pull request as ready for review July 26, 2024 21:18

maflcko approved these changes Jul 29, 2024

View reviewed changes

hebasto force-pushed the 240726-cmake-HR branch from 37e2b42 to 2353541 Compare July 29, 2024 11:13

hebasto marked this pull request as draft July 29, 2024 11:39

fixup! ci: Migrate CI scripts to CMake

5ecbd3f

Migrate the "test-each-commit" task

hebasto force-pushed the 240726-cmake-HR branch from 2353541 to 5ecbd3f Compare July 29, 2024 11:53

hebasto marked this pull request as ready for review July 29, 2024 11:54

maflcko reviewed Jul 29, 2024

View reviewed changes

hebasto force-pushed the 240726-cmake-HR branch from 1a603b8 to 5ecbd3f Compare July 29, 2024 12:56

m3dwards approved these changes Jul 29, 2024

View reviewed changes

maflcko approved these changes Jul 29, 2024

View reviewed changes

hebasto merged commit b29a18c into cmake-staging Jul 29, 2024

hebasto mentioned this pull request Jul 31, 2024

cmake: Regular rebasing of the cmake-staging branch #290

Closed

		# Distributed under the MIT software license, see the accompanying
		# file COPYING or http://www.opensource.org/licenses/mit-license.php.

Conversation

hebasto commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 26, 2024

Uh oh!

m3dwards commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 29, 2024

Uh oh!

m3dwards commented Jul 29, 2024

Uh oh!

m3dwards commented Jul 29, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

m3dwards commented Jul 29, 2024

Uh oh!

hebasto commented Jul 29, 2024

Uh oh!

m3dwards commented Jul 29, 2024

Uh oh!

hebasto commented Jul 29, 2024

Uh oh!

maflcko Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 29, 2024

Uh oh!

hebasto commented Jul 29, 2024

Uh oh!

m3dwards commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hebasto commented Jul 26, 2024 •

edited

Loading

m3dwards commented Jul 29, 2024 •

edited

Loading

maflcko Jul 29, 2024 •

edited

Loading