build: Minor build system fixes and amendments

hebasto commented at 5:22 pm on September 3, 2024: member

This PR addresses the following comments:

hebasto added the label Build system on Sep 3, 2024

DrahtBot commented at 5:22 pm on September 3, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipsorcery, maflcko

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30732 (build: improve cxx and linker flag caching by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto renamed this:
~~build: Minor build syystem fixes and amendments~~
build: Minor build system fixes and amendments
on Sep 3, 2024

in CMakeLists.txt:149 in ffcce50b2c outdated

145@@ -146,6 +146,8 @@ if(WITH_ZMQ)
146   else()
147     # The ZeroMQ project has provided config files since v4.2.2.
148     # TODO: Switch to find_package(ZeroMQ) at some point in the future.
149+    # Note: As of 2024, mainstream distributions do not yet provide
150+    # CMake config files for ZeroMQ packages.

maflcko commented at 5:28 pm on September 3, 2024:

0    # However, as of 2024, mainstream distributions do not yet provide
1    # CMake config files for ZeroMQ packages.
2    # If they do in the future, find_package(ZeroMQ) may be used instead.

nit: Could remove the TODO above and clarify that it is waiting on the distros.

hebasto commented at 5:33 pm on September 3, 2024:

Thanks! Reworked per your feedback.

maflcko approved

maflcko commented at 5:30 pm on September 3, 2024: member

review ACK c393acadf213ddf58a69759eaf341595ccbf4889

hebasto force-pushed on Sep 3, 2024

in CMakeLists.txt:148 in 2b7d22448b outdated

144@@ -145,7 +145,9 @@ if(WITH_ZMQ)
145     find_package(ZeroMQ CONFIG REQUIRED)
146   else()
147     # The ZeroMQ project has provided config files since v4.2.2.
148-    # TODO: Switch to find_package(ZeroMQ) at some point in the future.
149+    # However, as of 2024, mainstream distributions do not yet provide

fanquake commented at 8:45 pm on September 3, 2024:

I think we should just remove any dates entirely.

hebasto commented at 2:41 am on September 4, 2024:

Thanks! Done.

hebasto force-pushed on Sep 4, 2024

maflcko commented at 5:29 am on September 4, 2024: member

review ACK dee36a6746f18a3db748ef7bf52bfebddb546307

CI output:

Before: https://cirrus-ci.com/task/6415951824945152?logs=ci#L1057

0C++ compiler flags .................... -Wno-error=documentation -O0 -ftrapv -O1 -g3 -g3 -std=c++20 -fPIC -fdebug-prefix-map=/ci_container_base=. -fmacro-prefix-map=/ci_container_base=. -Werror -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE

After: https://cirrus-ci.com/task/5828698144768000?logs=ci#L1065

0C++ compiler flags .................... -m32 -Wno-error=documentation -O0 -ftrapv -O1 -g3 -g3 -std=c++20 -fPIC -fdebug-prefix-map=/ci_container_base=. -fmacro-prefix-map=/ci_container_base=. -Werror -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE

maflcko commented at 6:45 am on September 4, 2024: member

Could include #30454 (review) to avoid having to open another pull for that?

hebasto commented at 7:10 am on September 4, 2024: member

Could include #30454 (comment) to avoid having to open another pull for that?

Sure! Included.

maflcko commented at 7:21 am on September 4, 2024: member

lgtm ACK cc9d65dc05f802aaaa108a2edcf30c3758c3f459

Let’s wait for the outcome of the discussions in #30454 (review) and #30454 (review) as well?

fanquake commented at 8:55 am on September 4, 2024: member

Let’s also wait for #30454 (review). To clarify if this ever worked (and was later broken?), or was just never expected to work. In either case, it could be worth documenting.

in CMakeLists.txt:142 in cc9d65dc05 outdated

144@@ -145,7 +145,9 @@ if(WITH_ZMQ)
145     find_package(ZeroMQ CONFIG REQUIRED)
146   else()
147     # The ZeroMQ project has provided config files since v4.2.2.
148-    # TODO: Switch to find_package(ZeroMQ) at some point in the future.
149+    # However, mainstream distributions do not yet provide CMake

fanquake commented at 10:38 am on September 4, 2024:

It’s disappointing that so many distros don’t support CMake in their packaging. We basically can’t switch this (and any other packages where we might want to use the “native” CMake config) until every distro does, otherwise it’ll just be broken for any distro that doesn’t.

Some do, i.e Alpine, or Arch, or nix, but the majority do not, ie Debian, Ubuntu, Fedora, *BSDs, as well as other package managers, i.e brew or Guix. Gentoo has had an issue open for adding CMake support for ~5 years: https://bugs.gentoo.org/695202, but no change.

There are multiple issues here (and it’s not really all the fault of the distros):

One is that distros won’t switch build systems if the current one works fine, and switching doesn’t provide (m)any benefits, while having a 100% chance of breaking dependant packages/other things.
Another is that packages like zeromq, which have opted to provide two build systems, don’t properly support installing the config files from one, in the other.
- i.e if you build with CMake you’ll get CMake config files, and pkg-config files. However if you build with Autotools, you just get pkg-config files.
- One solution to try and speed up CMake adoption would be to upstream support for installing CMake config files from Autotools.

I assume one thing we could do, is try find_package(ZeroMQ) first, and fallback to pkg_check_modules(libzmq), and just maintain this (pretty much) forever. That might be better than somewhat vauge comments about “modern” distros, because, (some) modern distros do already support this, and it’s actually the case that all versions of all distros Bitcoin Core supports need to do this, before we could do a complete switch.

Sjors commented at 7:11 am on September 5, 2024:

This explanation makes more sense to me than the current code comment.

try find_package(ZeroMQ) first, and fallback to pkg_check_modules(libzmq)

That would be nice. Then a short clarifying comment could be:

0# Use CMake config file if the distro provides it (pseudo code)
1try find_package(ZeroMQ)
2# Otherwise fall back to Autotools
3catch pkg_check_modules(libzmq)

fanquake commented at 8:48 am on September 9, 2024:

Are you planning on updating this comment given the above? Seems like we can have something a bit less vauge, which makes it clear that this is something that is going to have to wait a long time, and isn’t actually directly dependant on the distros. Although, it seems like we could just switch the code to try both now, given that is likely what we’ll have to do (and keep) in any case.

fanquake commented at 10:36 am on September 9, 2024:

Discussed offline, and we will change the code to try both in a follow up.

fanquake commented at 9:27 am on September 12, 2024:

Opened an issue for this #30876.

doc: Amend comment about ZeroMQ config files 6f2cb0eafd

build, test: Add missed log options f03c942095

build: Print `CMAKE_CXX_COMPILER_ARG1` in summary

When `-DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-m32'` is provided,
`-stdlib=libc++ -m32` flags are printed in the summary now.

73b618582d

doc: Drop `ctest` command from Windows cross-compiling instructions

The ctest command was added hastily without considering the requirement
of Wine, which is generally not trivial to install.

b2a6f545b4

build: Stop enabling CMake's CMP0141 policy

The `CMAKE_MSVC_DEBUG_INFORMATION_FORMAT` variable has not been used
since the merge of https://github.com/hebasto/bitcoin/pull/215 in the
CMake staging branch.

fdad128b52

in doc/build-windows.md:70 in cc9d65dc05 outdated

66@@ -67,7 +67,6 @@ Build using:
67     gmake -C depends HOST=x86_64-w64-mingw32  # Use "-j N" for N parallel jobs.
68     cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
69     cmake --build build     # Use "-j N" for N parallel jobs.
70-    ctest --test-dir build  # Use "-j N" for N parallel tests. Some tests are disabled if Python 3 is not available.

Sjors commented at 7:13 am on September 5, 2024:

cc9d65dc05f802aaaa108a2edcf30c3758c3f459: maybe add a comment for why there’s not test command (yet?)

maflcko commented at 7:22 am on September 5, 2024:

See #30454 (review)

hebasto force-pushed on Sep 6, 2024

build: Delete MSVC special case for `BUILD_FOR_FUZZING` option 341ad23809

DrahtBot added the label CI failed on Sep 8, 2024

build: Delete dead code that implements `IF_CHECK_FAILED` option 1cc93fe7b4

hebasto commented at 3:38 pm on September 8, 2024: member

lgtm ACK cc9d65d

Let’s wait for the outcome of the discussions in #30454 (comment) and #30454 (comment) as well?

Thanks! Both comments have been addressed.

sipsorcery commented at 8:34 pm on September 8, 2024: member

tACK 1cc93fe7b40f10a7d1d1189058af98a2bce31381 (win11 msvc).

DrahtBot requested review from maflcko on Sep 8, 2024

DrahtBot removed the label CI failed on Sep 9, 2024

hebasto commented at 12:49 pm on September 10, 2024: member

@maflcko @fanquake

I believe all comments have been addressed, unless I missed something.

maflcko commented at 7:15 am on September 11, 2024: member

re-ACK 1cc93fe7b40f10a7d1d1189058af98a2bce31381

Only change since last review is MSVC fixups as well as dropping dead (and broken) code.

maflcko added the label DrahtBot Guix build requested on Sep 11, 2024

DrahtBot commented at 1:55 am on September 12, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 0725a374941355349bb4bc8a79dad1affb27d3b9(master)	commit 06163aa1ca4a431d9b6f2eb62081a5a716b8f3fc(master and this pull)
SHA256SUMS.part	`a02d778ecea785cb...`	`1149441268b4d8e7...`
*-aarch64-linux-gnu-debug.tar.gz	`2f9b090610716543...`	`5ef87a77c6a42c25...`
*-aarch64-linux-gnu.tar.gz	`cdddc2a011cbc71f...`	`69de6e38c3b46c37...`
*-arm-linux-gnueabihf-debug.tar.gz	`3dc368c59f717ba5...`	`a7a769b54412049f...`
*-arm-linux-gnueabihf.tar.gz	`25130ed715fb51dc...`	`541ae2cc199d0e4c...`
*-arm64-apple-darwin-unsigned.tar.gz	`af2c8d808c67cf11...`	`e655e219f2efc531...`
*-arm64-apple-darwin-unsigned.zip	`f9b25947985c1303...`	`4b2e8f52e5f96a3f...`
*-arm64-apple-darwin.tar.gz	`7511aa736ffea54b...`	`9daa67db5b1f709d...`
*-powerpc64-linux-gnu-debug.tar.gz	`e024d862e1d06449...`	`f2c49edb1f07df94...`
*-powerpc64-linux-gnu.tar.gz	`bd2e78a034cbf626...`	`b501c32fddd870b1...`
*-riscv64-linux-gnu-debug.tar.gz	`b3ee81fe9f44f0e8...`	`0ea0ba65152ef57e...`
*-riscv64-linux-gnu.tar.gz	`8406605d831d0a39...`	`cb1d082c328e7671...`
*-x86_64-apple-darwin-unsigned.tar.gz	`413c24a042a77e51...`	`07a89be76f7eb657...`
*-x86_64-apple-darwin-unsigned.zip	`8aa62b3d181cb236...`	`389572c47fed833e...`
*-x86_64-apple-darwin.tar.gz	`92c1741a2626021a...`	`82441379856d9bf1...`
*-x86_64-linux-gnu-debug.tar.gz	`4f05b270004745fa...`	`07a247bd99603bf4...`
*-x86_64-linux-gnu.tar.gz	`5873429dcd6b0d7b...`	`83bcb903cefe76b2...`
*.tar.gz	`2714f753f68195ae...`	`4ef67052bd7a228d...`
guix_build.log	`5c24a7305a189158...`	`69a34832af36b765...`
guix_build.log.diff		`b6669a0601ebbb09...`

DrahtBot removed the label DrahtBot Guix build requested on Sep 12, 2024

maflcko requested review from fanquake on Sep 12, 2024

fanquake merged this on Sep 12, 2024

fanquake closed this on Sep 12, 2024

hebasto deleted the branch on Sep 12, 2024

build: Minor build system fixes and amendments #30803

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]