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

MarcoFalke commented at 3:58 pm on February 8, 2022: member

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”.

MarcoFalke added the label Build system on Feb 8, 2022

MarcoFalke added the label DrahtBot Guix build requested on Feb 8, 2022

MarcoFalke renamed this:
~~build: Remove libboost-test-dev depencency when --enable-fuzz~~
build: Remove libboost-test-dev dependency when --enable-fuzz
on Feb 8, 2022

MarcoFalke force-pushed on Feb 8, 2022

hebasto commented at 5:10 pm on February 8, 2022: member

Concept ACK.

fanquake commented at 9:28 am on February 9, 2022: member

Concept ACK

laanwj commented at 3:45 pm on February 9, 2022: member

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

MarcoFalke force-pushed on Feb 10, 2022

MarcoFalke commented at 6:21 pm on February 10, 2022: member

Thanks, I split up the enable_test vs enable_fuzz_binary entanglement.

fanquake referenced this in commit e0367e84b3 on Feb 14, 2022

fanquake commented at 10:06 am on February 14, 2022: member

Is this resolved post-#24301 ?

MarcoFalke force-pushed on Feb 14, 2022

MarcoFalke renamed this:
~~build: Remove libboost-test-dev dependency when --enable-fuzz~~
build: Remove negated --enable-fuzz checks from build system
on Feb 14, 2022

MarcoFalke force-pushed on Feb 14, 2022

in configure.ac:1475 in fa2c8979e8 outdated

1471@@ -1471,7 +1472,7 @@ fi
1472 
1473 dnl libevent check
1474 
1475-if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench" != "nonononono"; then
1476+if test "$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_tests$use_bench" != "nonnoononono"; then

laanwj commented at 1:25 pm on February 14, 2022:

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

MarcoFalke commented at 1:28 pm on February 14, 2022:

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

theStack commented at 5:53 pm on March 19, 2022:

This seems to be a typo

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

MarcoFalke commented at 9:22 am on March 21, 2022:

laanwj commented at 6:10 am on April 14, 2022:

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

MarcoFalke commented at 6:14 am on April 14, 2022:

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

laanwj commented at 6:43 am on April 14, 2022:

If only boolean logic was invented in this timeline !

luke-jr commented at 3:01 pm on May 30, 2022:

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

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

Or even if we just add a:

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

somewhere…

laanwj commented at 1:26 pm on February 14, 2022: member

Code review ACK fa2c8979e8114a2b6b57a547e97e61bac8cfaa75

DrahtBot commented at 3:02 pm on February 16, 2022: contributor

Guix builds

File	commit 8fe6f5a6fbcd8083d916cb630f35f8f5980d6825(master)	commit 51cdcfe24128fbbb240051dc7cc129aedf044861(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...`

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

theStack commented at 5:55 pm on March 19, 2022: contributor

Concept ACK

MarcoFalke force-pushed on Mar 21, 2022

MarcoFalke commented at 9:18 am on March 21, 2022: member

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

DrahtBot commented at 4:01 am on March 28, 2022: contributor

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.

DrahtBot added the label Needs rebase on Apr 6, 2022

MarcoFalke force-pushed on Apr 6, 2022

DrahtBot removed the label Needs rebase on Apr 6, 2022

MarcoFalke force-pushed on Apr 30, 2022

DrahtBot added the label Needs rebase on Jun 16, 2022

laanwj commented at 5:56 am on June 22, 2022: member

Code review ACK fa231f8ffc7337b61b6b6ee8eccbbb1a0ca32b4c

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

MarcoFalke force-pushed on Jun 22, 2022

DrahtBot removed the label Needs rebase on Jun 22, 2022

laanwj commented at 8:29 am on June 22, 2022: member

Code review re-ACK fa7cbc6e5c36f1e37d18ffe1084cfccf52cbc296

laanwj merged this on Jun 22, 2022

laanwj closed this on Jun 22, 2022

MarcoFalke deleted the branch on Jun 22, 2022

sidhujag referenced this in commit 18e1ed5035 on Jun 22, 2022

mzumsande commented at 7:00 pm on July 13, 2022: contributor

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

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

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

bitcoin locked this on Jul 13, 2023

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

Guix builds

Conflicts