build: stop overriding user autoconf flags

fanquake commented at 10:55 am on February 19, 2022: member

Historically our build system has hijacked CXXFLAGS and friends, and this has always been a source of complaints from users and developers. With this PR, we move away from using CXXFLAGS, CPPFLAGS and LDFLAGS, and instead use CORE_*FLAGS variables for our flags / options, leaving autoconfs FLAG vars to the user.

Note that there are currently two cases where we will at least clear CXXFLAGS (if not alreaddy overridden by the user), when doing debugging or when coverage is enabled, to avoid Autoconfs -g -O2 CXXFLAG default.

fanquake added the label Build system on Feb 19, 2022

fanquake force-pushed on Feb 19, 2022

DrahtBot commented at 8:11 pm on February 20, 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:

#24671 (build: remove unneeded configure $*val setting by fanquake)
#24322 ([kernel 1/n] Introduce initial libbitcoinkernel by dongcarl)
#23969 (build: remove use of TARGET_OS and BUILD_OS by fanquake)
#21570 (build, qt: Simplifies checks for QT_REDUCE_RELOCATIONS by hebasto)
#18427 (Bugfix? Restore linking to libmingwthrd by luke-jr)

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.

fanquake commented at 12:05 pm on February 23, 2022: member

@dongcarl @achow101 concept ACK/NACK?

dongcarl commented at 5:56 pm on February 23, 2022: contributor

Concept ACK

fanquake added this to the milestone 24.0 on Feb 24, 2022

achow101 commented at 12:39 pm on March 4, 2022: member

Concept ACK

in configure.ac:366 in a5268e0ed0 outdated

356@@ -357,7 +357,9 @@ case $host in
357 esac
358 
359 if test "$enable_debug" = "yes"; then
360-  dnl Clear default -g -O2 flags
361+  dnl If debugging is enabled, and the user hasn't overriden CXXFLAGS, clear
362+  dnl them, to prevent autoconfs "-g -O2" being added. Otherwise we'd end up
363+  dnl with "-O0 -g3 -g -O2".

luke-jr commented at 9:24 pm on March 5, 2022:

Probably we should differentiate between necessary flags (eg, include paths) and provided defaults (eg, -O2 -g), and do “$DEFAULTS $USER $NECESSARY” so this just works without needing a special hack.

real-or-random commented at 6:53 am on March 31, 2022:

I think the philosophy (also according to automake docs) is that the user has always the last word, so it should really be “$DEFAULTS $USER $NECESSARY”. This gives the user also the last word on what is “necessary”, e.g., the user may have a strange compiler/wrapper, where other flags are necessary.

If the user wants to break the build by overriding flags, that’s his choice. And even this can be helpful for debugging the build.

fanquake commented at 11:18 am on March 31, 2022:

so it should really be “$DEFAULTS $USER $NECESSARY”.

I assume you mean “$DEFAULTS $NECESSARY $USER” ?

I agree. The point of these change is to stop overriding user choice, so doing something that would prevent the user from having the final say would defeat the point.

real-or-random commented at 12:29 pm on March 31, 2022:

I assume you mean “$DEFAULTS $NECESSARY $USER” ?

Sure, sorry, copy and paste…

real-or-random commented at 6:54 am on March 31, 2022: contributor

Concept ACK

fanquake force-pushed on Mar 31, 2022

fanquake commented at 11:16 am on March 31, 2022: member

Rebased on top of #24722, which should solve the macOS build failure.

fanquake referenced this in commit a7f0c37c2b on Apr 1, 2022

fanquake force-pushed on Apr 1, 2022

fanquake marked this as ready for review on Apr 1, 2022

fanquake commented at 7:41 pm on April 1, 2022: member

Rebased after #24722, and made some small fixups. Now that the CI is passing, opening this up for review.

in configure.ac:2013 in d098996906 outdated

2009@@ -2009,7 +2010,7 @@ echo "  build os        = $build_os"
2010 echo
2011 echo "  CC              = $CC"
2012 echo "  CFLAGS          = $PTHREAD_CFLAGS $CFLAGS"
2013-echo "  CPPFLAGS        = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CPPFLAGS"
2014+echo "  CPPFLAGS        = $DEBUG_CPPFLAGS $HARDENED_CPPFLAGS $CORE_CPPFLAGS $CORE_CPPFLAGS $CPPFLAGS"

real-or-random commented at 5:37 pm on April 2, 2022:

One $CORE_CPPFLAGS is enough. :)

fanquake commented at 5:51 pm on April 2, 2022:

Whoops. Addressed now.

doc: explain why we clear CXXFLAGS with enable-debug bc7cc57607

fanquake force-pushed on Apr 2, 2022

sidhujag referenced this in commit c2266a52f5 on Apr 3, 2022

real-or-random approved

real-or-random commented at 8:53 am on April 3, 2022: contributor

utACK 300ebf6bb7c0516fad4c2bdf4bb88a73a7f4d02c diff looks good

fanquake commented at 10:00 am on April 3, 2022: member

@achow101 @dongcarl want to (re-)test with your systems?

achow101 commented at 3:15 pm on April 3, 2022: member

ACK 300ebf6bb7c0516fad4c2bdf4bb88a73a7f4d02c

hebasto commented at 5:12 pm on April 3, 2022: member

Concept ACK.

in configure.ac:622 in 300ebf6bb7 outdated

618@@ -617,7 +619,7 @@ CXXFLAGS="$TEMP_CXXFLAGS"
619 
620 fi
621 
622-CPPFLAGS="$CPPFLAGS -DHAVE_BUILD_INFO"
623+CORE_CPPFLAGS="-DHAVE_BUILD_INFO"

hebasto commented at 5:37 pm on April 3, 2022:

fe263822287ffbf650dd6dc6bae5f62b4c5d1ca7

This code works, but I’d suggest a safer future-proof one:

0CORE_CPPFLAGS="$CORE_CPPFLAGS -DHAVE_BUILD_INFO"

fanquake commented at 6:36 pm on April 3, 2022:

Done.

hebasto commented at 6:04 pm on April 3, 2022: member

Should the configure script check whether a preprocessor/compiler/linker accepts user-provided flags?

build: stop overriding user CPPFLAGS

Let the user have the final say in regards to CPPFLAGS

35c3fd43c3

build: stop overriding user LDFLAGS

Let the user have the final say in regards to LDFLAGS.

3e2ef23c3e

build: stop overriding user CXXFLAGS

Let users have the final say in regards to CXXFLAGS.

7b00595d33

fanquake force-pushed on Apr 3, 2022

fanquake commented at 6:37 pm on April 3, 2022: member

Should the configure script check whether a preprocessor/compiler/linker accepts user-provided flags?

No. Users are free to do what they want, and we shouldn’t be trying to second guess them. That would also get in the way of letting them set their own flags.

hebasto approved

hebasto commented at 6:40 pm on April 4, 2022: member

ACK 7b00595d335915dc2bf856e3569115996381a402

fanquake merged this on Apr 5, 2022

fanquake closed this on Apr 5, 2022

sidhujag referenced this in commit 1220beed76 on Apr 5, 2022

fanquake deleted the branch on Apr 20, 2022

laanwj referenced this in commit 8730bd3fc8 on Apr 28, 2022

sidhujag referenced this in commit 7c532b2829 on Apr 29, 2022

theuni commented at 3:43 pm on September 13, 2022: member

Nice. I missed this a few months ago. Post merge concept-ACK.

bitcoin locked this on Sep 13, 2023

build: stop overriding user autoconf flags #24391

Conflicts