build: Remove negated --enable-fuzz checks from build system #24291

maflcko · 2022-02-08T15:58:10Z

It is confusing to enable the unit test binary with ENABLE_TESTS && !ENABLE_FUZZ, but every other binary is enabled with a simple flag. For example ENABLE_BENCH, or ENABLE_FUZZ_BINARY.

Fix that by turning ENABLE_TESTS back into meaning "enable unit test binary".

hebasto · 2022-02-08T17:10:10Z

Concept ACK.

fanquake · 2022-02-09T09:28:51Z

Concept ACK

laanwj · 2022-02-09T15:45:47Z

Concept ACK, however implementing the logic "don't require boost-test if fuzz enabled" directly feels somewhat contorted to me. Can't we, say, disable use_tests when fuzz is enabled, or alternatively introduce a new variable for this.

Edit: or, even better, get rid of that code entirely as in #24301

maflcko · 2022-02-10T18:21:10Z

Thanks, I split up the enable_test vs enable_fuzz_binary entanglement.

5d399f9 build: remove native B2 package (fanquake) 2037a3b build: header-only Boost (fanquake) 39e66e9 build: use header-only Boost unit test (fanquake) Pull request description: This PR converts our Boost usage to header only. We switch from using our last remaining Boost lib (unit test), to using it's header-only implementation (see https://www.boost.org/doc/libs/1_78_0/libs/test/doc/html/boost_test/adv_scenarios/single_header_customizations/multiple_translation_units.html). Also related to #24291. Guix build: ```bash ``` ACKs for top commit: hebasto: re-ACK 5d399f9 MarcoFalke: approach ACK 5d399f9 📞 Tree-SHA512: e60835ee9c11aa941a64679616da2002d6cd86e464895372fafdd42ad6499d7eb1dde6f0013c60adaeb97bd191198430cb158a7a7417b38080dd7106b28e3ba5

fanquake · 2022-02-14T10:06:02Z

Is this resolved post-#24301 ?

laanwj · 2022-02-14T13:25:34Z

configure.ac

@@ -1471,7 +1472,7 @@ fi

 dnl libevent check

-if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench" != "nonononono"; then
+if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nonnoononono"; then


i sometimes wonder if we can switch to actual logic instead of this hilarious ever growing nononononono concatenation
(outside scope of this PR obviously)

Agree. Maybe the build people can shed some light on this?

If we used true/false instead of yes/no, we could just do

if $build_bitcoin_cli || $build_bitcoind || $bitcoin_enable_qt || $enable_fuzz_binary || $use_tests || $use_bench; then

Or even if we just add a:

yes() { true; } no() { false; }

somewhere...

laanwj · 2022-02-14T13:26:15Z

Code review ACK fa2c897

DrahtBot · 2022-02-16T15:02:30Z

Guix builds

File	commit `8fe6f5a` (master)	commit `51cdcfe` (master and this pull)
SHA256SUMS.part	`15fa6ceff410a599...`	`e550d7cb767c9bf5...`
*-aarch64-linux-gnu-debug.tar.gz	`64e903c3ab4a158d...`	`ae8eb953125440ca...`
*-aarch64-linux-gnu.tar.gz	`687cb2caced5a1d5...`	`c81ab02a43ddcce8...`
*-arm-linux-gnueabihf-debug.tar.gz	`278a4bd0ac3706e1...`	`5bc62b0dac34020a...`
*-arm-linux-gnueabihf.tar.gz	`b1c7afb5e8ea8e9d...`	`7d5a4063dfd81be2...`
*-arm64-apple-darwin.tar.gz	`ac175d1ead43ec66...`	`452ef5e207cfd916...`
*-osx-unsigned.dmg	`1e73babee30d4073...`	`5a5d96c45f3b7a9d...`
*-osx-unsigned.tar.gz	`8d8f0a29b9dccc1d...`	`37326d1491b9f618...`
*-osx64.tar.gz	`88222574867e6c1a...`	`64c48b2fc83db4d1...`
*-powerpc64-linux-gnu-debug.tar.gz	`9877ab4f41cbb5a2...`	`54ad7c653005b574...`
*-powerpc64-linux-gnu.tar.gz	`a6ab4f51fafc8802...`	`d3dc4458798bc6c4...`
*-powerpc64le-linux-gnu-debug.tar.gz	`ce1958eed5123e00...`	`1dbf68096bf2e007...`
*-powerpc64le-linux-gnu.tar.gz	`df479c7c8799f5a9...`	`0813b838510d4e08...`
*-riscv64-linux-gnu-debug.tar.gz	`bd591a23d9f5cf73...`	`994efb0db71d2082...`
*-riscv64-linux-gnu.tar.gz	`78c2f64ad952a8a3...`	`ce0dcaff2fe8e5d8...`
*-x86_64-linux-gnu-debug.tar.gz	`5e2bbc5d33779d93...`	`391e13532f0e7f8e...`
*-x86_64-linux-gnu.tar.gz	`469ec6f702d64710...`	`8d73d3b9d568a586...`
*.tar.gz	`1a95f4a3d9e2225b...`	`68814a1b5897eb50...`
guix_build.log	`72a3e57c4499ea20...`	`ee3ef6c642aeb89e...`
guix_build.log.diff		`d60c34fb79c45876...`

theStack

Concept ACK

theStack · 2022-03-19T17:53:49Z

configure.ac

@@ -1471,7 +1472,7 @@ fi

 dnl libevent check

-if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench" != "nonononono"; then
+if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nonnoononono"; then


This seems to be a typo

Suggested change

if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nonnoononono"; then

if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nononononono"; then

So the construction is not only absurd but also error-prone and hard to review, as proven here.

Is there any way to fix this? Switching to cmake, maybe? @fanquake

If only boolean logic was invented in this timeline !

maflcko · 2022-03-21T09:18:52Z

Thanks, fixed nonono typo. Should be trivial to re-ACK with git range-diff --word-diff-regex=. bitcoin-core/master fa2c8979e8 fa4feabaf5.

DrahtBot · 2022-03-28T04:01:40Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25282 (Bugfix: configure: Define default for use_libevent by luke-jr)
#25037 (build: Create noinst_LTLIBRARIES conditionally by hebasto)

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.

laanwj · 2022-06-22T05:56:33Z

Code review ACK fa231f8

laanwj · 2022-06-22T08:29:22Z

Code review re-ACK fa7cbc6

…build s…

mzumsande

I currently get autogen.sh warnings on master, which I backtraced to this PR:

src/Makefile.bench.include:97: warning: %.raw.h was already defined in condition TRUE, which includes condition ENABLE_BENCH ...
src/Makefile.am:1059:   'src/Makefile.bench.include' included from here
src/Makefile.test.include:420: ... '%.raw.h' previously defined here
src/Makefile.am:1056:   'src/Makefile.test.include' included from here

[Edit] Oh, there is already an issue for this, #25501.

maflcko added Build system DrahtBot Guix build requested labels Feb 8, 2022

maflcko changed the title ~~build: Remove libboost-test-dev depencency when --enable-fuzz~~ build: Remove libboost-test-dev dependency when --enable-fuzz Feb 8, 2022

maflcko force-pushed the 2202-fuzzNoBoost branch from fa132bf to fa738dc Compare February 8, 2022 15:59

Sagor2214 mentioned this pull request Feb 9, 2022

Concept ACK Sagor2214/github-actions-continuous-delivery-azure#2

Open

fanquake mentioned this pull request Feb 9, 2022

build: header-only Boost #24301

Merged

maflcko force-pushed the 2202-fuzzNoBoost branch from fa738dc to fa4bd83 Compare February 10, 2022 18:18

maflcko force-pushed the 2202-fuzzNoBoost branch from fa4bd83 to ab4b7e8 Compare February 14, 2022 12:28

maflcko changed the title ~~build: Remove libboost-test-dev dependency when --enable-fuzz~~ build: Remove negated --enable-fuzz checks from build system Feb 14, 2022

maflcko force-pushed the 2202-fuzzNoBoost branch from ab4b7e8 to fa2c897 Compare February 14, 2022 12:36

laanwj reviewed Feb 14, 2022

View reviewed changes

DrahtBot removed the DrahtBot Guix build requested label Feb 16, 2022

theStack reviewed Mar 19, 2022

View reviewed changes

maflcko force-pushed the 2202-fuzzNoBoost branch from fa2c897 to fa4feab Compare March 21, 2022 09:18

DrahtBot mentioned this pull request Mar 27, 2022

build: Bump libevent minimum version up to 2.1.8 #24681

Merged

DrahtBot mentioned this pull request Mar 31, 2022

build: remove unneeded configure $*val setting #24671

Closed

DrahtBot added the Needs rebase label Apr 6, 2022

maflcko force-pushed the 2202-fuzzNoBoost branch from fa4feab to fad3d75 Compare April 6, 2022 14:57

DrahtBot removed the Needs rebase label Apr 6, 2022

DrahtBot mentioned this pull request Apr 30, 2022

build: Create noinst_LTLIBRARIES conditionally #25037

Closed

maflcko force-pushed the 2202-fuzzNoBoost branch from fad3d75 to fa231f8 Compare April 30, 2022 08:07

DrahtBot mentioned this pull request Jun 6, 2022

Bugfix: configure: Define default for use_libevent #25282

Merged

DrahtBot added the Needs rebase label Jun 16, 2022

build: Remove negated --enable-fuzz checks from build system

fa7cbc6

maflcko force-pushed the 2202-fuzzNoBoost branch from fa231f8 to fa7cbc6 Compare June 22, 2022 06:12

DrahtBot removed the Needs rebase label Jun 22, 2022

laanwj merged commit b1a824d into bitcoin:master Jun 22, 2022

maflcko deleted the 2202-fuzzNoBoost branch June 22, 2022 08:38

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2022

Merge bitcoin#24291: build: Remove negated --enable-fuzz checks from …

18e1ed5

…build s…

mzumsande reviewed Jul 13, 2022

View reviewed changes

bitcoin locked and limited conversation to collaborators Jul 13, 2023

	if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nonnoononono"; then
	if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nononononono"; then

build: Remove negated --enable-fuzz checks from build system #24291

build: Remove negated --enable-fuzz checks from build system #24291

Uh oh!

Conversation

maflcko commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Feb 8, 2022

Uh oh!

fanquake commented Feb 9, 2022

Uh oh!

laanwj commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 10, 2022

Uh oh!

fanquake commented Feb 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 14, 2022

Uh oh!

DrahtBot commented Feb 16, 2022

Guix builds

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 21, 2022

Uh oh!

DrahtBot commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Jun 22, 2022

Uh oh!

laanwj commented Jun 22, 2022

Uh oh!

mzumsande left a comment • 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!

Uh oh!

maflcko commented Feb 8, 2022 •

edited

Loading

laanwj commented Feb 9, 2022 •

edited

Loading

DrahtBot commented Mar 28, 2022 •

edited

Loading

mzumsande left a comment •

edited

Loading