build: extract $(BOOST_CPPFLAGS) from $(BITCOIN_INCLUDES) #26056

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:finer_boost_cppflags changing 7 files +15 −15
  1. fanquake commented at 4:57 pm on September 9, 2022: member

    This leaves $(BITCOIN_INCLUDES) as internal dependencies, and gives finer control over Boost includes.

    Fixes #25947.

  2. fanquake added the label Build system on Sep 9, 2022
  3. hebasto commented at 5:03 pm on September 9, 2022: member
    Concept ACK.
  4. theuni commented at 5:28 pm on September 9, 2022: member

    Concept ACK.

    Hah, I was working on this at the same time (on top of master, though). To compare, here’s what I came up with: https://github.com/theuni/bitcoin/commit/400eede3e56b15db86a83d7785a57f179f6cba85

    Rather than adding it where it looked like it would be needed, I added a depends commit to move boost out of the common prefix path, that way we’ll actually detect missing headers: https://github.com/theuni/bitcoin/commit/e131d8f1e33baa9a9499d2e1a4a99b171566c5a5 . Then I rebuilt everything and added BOOST_CPPFLAGS one-by-one as needed to fix compilation.

  5. theuni commented at 5:34 pm on September 9, 2022: member

    Looks like the only difference between ours (other than test_util) is missing BOOST_CPPFLAGS for bitcoin_chainstate

    ACK after adding that (and after c-i is happy).

  6. hebasto commented at 9:54 pm on September 9, 2022: member

    @theuni

    here’s what I came up with: theuni@400eede

    With flag reordering as follows:

     0diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
 1index 4fe79652e..602a11825 100644
 2--- a/src/Makefile.qt.include
 3+++ b/src/Makefile.qt.include
 4@@ -295,7 +295,7 @@ BITCOIN_QT_RC = qt/res/bitcoin-qt-res.rc
 5 BITCOIN_QT_INCLUDES = -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER
 6 
 7 qt_libbitcoinqt_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
 8-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS)
 9+  $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS) $(BOOST_CPPFLAGS)
10 qt_libbitcoinqt_a_CXXFLAGS = $(AM_CXXFLAGS) $(QT_PIE_FLAGS)
11 qt_libbitcoinqt_a_OBJCXXFLAGS = $(AM_OBJCXXFLAGS) $(QT_PIE_FLAGS)
12 
13diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
14index afcd4106b..89c659d4b 100644
15--- a/src/Makefile.qttest.include
16+++ b/src/Makefile.qttest.include
17@@ -27,7 +27,7 @@ TEST_QT_H = \
18   qt/test/wallettests.h
19 
20 qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
21-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_TEST_INCLUDES)
22+  $(QT_INCLUDES) $(QT_TEST_INCLUDES) $(BOOST_CPPFLAGS)
23 
24 qt_test_test_bitcoin_qt_SOURCES = \
25   init/bitcoin-qt.cpp \

    your commit fixes #25947 as well.

  7. fanquake force-pushed on Sep 10, 2022
  8. fanquake commented at 9:34 am on September 10, 2022: member

    Looks like the only difference between ours (other than test_util) is missing BOOST_CPPFLAGS for bitcoin_chainstate

    The bin I didn’t test compiling 😅. Rebased on master and fixed this up.

  9. fanquake marked this as ready for review on Sep 10, 2022
  10. hebasto commented at 11:35 am on September 10, 2022: member

    This diff:

     0--- a/src/Makefile.am
 1+++ b/src/Makefile.am
 2@@ -753,7 +753,7 @@ bitcoin_node_LDADD = $(LIBBITCOIN_NODE) $(bitcoin_bin_ldadd) $(LIBBITCOIN_IPC) $
 3 
 4 # bitcoin-cli binary #
 5 bitcoin_cli_SOURCES = bitcoin-cli.cpp
 6-bitcoin_cli_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(EVENT_CFLAGS)
 7+bitcoin_cli_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS) $(EVENT_CFLAGS)
 8 bitcoin_cli_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 9 bitcoin_cli_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $(PTHREAD_FLAGS)
10

    fixes macOS native build.

  11. Sjors commented at 8:03 am on September 12, 2022: member
    Concept ACK
  12. fanquake commented at 8:04 am on September 12, 2022: member

    fixes macOS native build.

    I don’t think that’s the right solution, and would actually just hide the underlying problem. Re-adding $(BOOST_CPPFLAGS) where it isn’t needed, also defeats the point of this change.

    Given that the CI runs with --enable-suppress-external-warnings, and the failure is due to warnings coming from libevent, there is some other problem here; -Wdocumentation warnings for an external dependency shouldn’t be appearing at all.

    Looking at the log, we’ve got -isystem /usr/local/Cellar/libevent/2.1.12/include, but then:

    0In file included from bitcoin-cli.cpp:40:
1/usr/local/include/event2/buffer.h:210:11: error: parameter 'buffer' not found in the function declaration [-Werror,-Wdocumentation]
2 * [@param](/bitcoin-bitcoin/contributor/param/) buffer the evbuffer that the callback is watching.
3          ^~~~~~

    so I assume the reason these are currently being suppressed is actually due to the Boost cppflags always being present, which adds -isystem /usr/local/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE to the compile, and suppresses all warnings coming from /usr/local/include.

  13. theuni commented at 1:49 pm on September 12, 2022: member
    @fanquake thanks for the explanation. I was struggling to understand why only osx would need boost when there doesn’t seem to be any dependency, but that explanation makes sense.
  14. theuni commented at 3:38 pm on September 12, 2022: member

    From ./ci/test/00_setup_env_native_asan.sh :

    export PACKAGES=“systemtap-sdt-dev bpfcc-tools clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libqrencode-dev libsqlite3-dev”

    From .cirrus.yml:

    brew_install_script: - brew install boost libevent qt@5 miniupnpc libnatpmp ccache zeromq qrencode libtool automake gnu-getopt

    Seems we’re pulling libevent (and a few others) from 2 places for MacOS. Why?

  15. MarcoFalke commented at 3:48 pm on September 12, 2022: member

    It is a bit messy, but the brew macos build doesn’t use the Ubuntu apt, so there should only be one place where this is installed: https://github.com/bitcoin/bitcoin/blob/5558d2f5496d8fe1c16f9edd1ef395fcd842e6fb/.cirrus.yml#L332

    You can also check the logs produced at runtime to double check.

  16. theuni commented at 5:37 pm on September 12, 2022: member

    I played around a little bit with this locally and was able to reproduce with a minimal test-case.

    I believe this commit (untested) should fix the root issue here: https://github.com/theuni/bitcoin/commit/7929a2a426624740fcb007ab16af3b5a11f35a68 (see the comment for a description of the issue). @hebasto do you have an easy way of testing? If not, I can just open a PR with these two commits and see if c-I is happy.

    Edit: updated to include a test.

  17. theuni commented at 9:40 pm on September 12, 2022: member

    @theuni

    here’s what I came up with: theuni@400eede

    With flag reordering as follows:

     0diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
 1index 4fe79652e..602a11825 100644
 2--- a/src/Makefile.qt.include
 3+++ b/src/Makefile.qt.include
 4@@ -295,7 +295,7 @@ BITCOIN_QT_RC = qt/res/bitcoin-qt-res.rc
 5 BITCOIN_QT_INCLUDES = -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER
 6 
 7 qt_libbitcoinqt_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
 8-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS)
 9+  $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS) $(BOOST_CPPFLAGS)
10 qt_libbitcoinqt_a_CXXFLAGS = $(AM_CXXFLAGS) $(QT_PIE_FLAGS)
11 qt_libbitcoinqt_a_OBJCXXFLAGS = $(AM_OBJCXXFLAGS) $(QT_PIE_FLAGS)
12 
13diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
14index afcd4106b..89c659d4b 100644
15--- a/src/Makefile.qttest.include
16+++ b/src/Makefile.qttest.include
17@@ -27,7 +27,7 @@ TEST_QT_H = \
18   qt/test/wallettests.h
19 
20 qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
21-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_TEST_INCLUDES)
22+  $(QT_INCLUDES) $(QT_TEST_INCLUDES) $(BOOST_CPPFLAGS)
23 
24 qt_test_test_bitcoin_qt_SOURCES = \
25   init/bitcoin-qt.cpp \

    your commit fixes #25947 as well.

    I do think specifying explicitly probed include paths (bdb/sqlite/qt5/miniupnpc/natpmp) before the others for brew whenever we can makes sense as a policy. And in this case, I agree that it would fix the problem. That said, I’m not sure we can always do this (include order may matter for other reasons), and homebrew isn’t exactly our top priority.

    So, since I don’t see any reason not to in this case… ACK reversing the order as you suggested.

  18. fanquake force-pushed on Sep 13, 2022
  19. fanquake commented at 9:38 am on September 13, 2022: member

    Rebased on master / #26070.

    That said, I’m not sure we can always do this (include order may matter for other reasons), and homebrew isn’t exactly our top priority. So, since I don’t see any reason not to in this case… ACK reversing the order as you suggested.

    I’ve ordered the flags in this way, however I agree that doing this is not ideal, and somewhat fragile.

  20. fanquake commented at 11:04 am on September 13, 2022: member
    Updated the description to note that this should also fix #25947 (I haven’t tested myself). Also going to tag this (and implicitly #26070) for 24.0, as they are fixing annoying bugs in --suppress-external-warnings, and now building on macOS when multiple versions of qt installed.
  21. fanquake added this to the milestone 24.0 on Sep 13, 2022
  22. fanquake referenced this in commit 29d540b7ad on Sep 13, 2022
  23. build: extract $(BOOST_CPPFLAGS) from $(BITCOIN_INCLUDES) 
    This leaves $(BITCOIN_INCLUDES) as internal dependencies, and gives
finer control over Boost includes.
    12de8f6262
  24. fanquake force-pushed on Sep 13, 2022
  25. fanquake commented at 4:16 pm on September 13, 2022: member
    Rebased for #26070.
  26. sidhujag referenced this in commit ec769c1e22 on Sep 13, 2022
  27. theuni commented at 8:43 pm on September 13, 2022: member

    ACK 12de8f6262e9e74eb3c1a1c46070f7bc6a4cdac6

    It’s nice that we’re now forced to make all external dependencies explicit. I suspect this will eventually lead to a few more boost removals.

  28. kouloumos commented at 8:46 pm on September 13, 2022: contributor

    Verified that it fixes #25947.

    Tested on macOS 10.15.7 with

    0$ brew list --version | grep qt
1qt 6.3.1_3
2qt@5 5.15.5_2 5.15.3
  29. hebasto approved
  30. hebasto commented at 9:55 pm on September 13, 2022: member
    ACK 12de8f6262e9e74eb3c1a1c46070f7bc6a4cdac6, tested on macOS Monterey 12.6 (21G115, Intel).
  31. jarolrod approved
  32. jarolrod commented at 4:19 am on September 14, 2022: member
    ACK 12de8f6262e9e74eb3c1a1c46070f7bc6a4cdac6
  33. fanquake merged this on Sep 14, 2022
  34. fanquake closed this on Sep 14, 2022
  35. fanquake deleted the branch on Sep 14, 2022
  36. fanquake referenced this in commit c67d6f5b5f on Sep 14, 2022
  37. sidhujag referenced this in commit 72669d67fe on Sep 14, 2022
  38. sidhujag referenced this in commit f9a8249fff on Sep 14, 2022
  39. Zero-1729 commented at 10:30 am on September 17, 2022: contributor

    Posthumous ACK 12de8f6262e9e74eb3c1a1c46070f7bc6a4cdac6

    P.S. Tested on macOS 12.6 (21G115), M1

    Thanks @fanquake!

  40. bitcoin locked this on Sep 17, 2023


fanquake hebasto theuni Sjors MarcoFalke kouloumos jarolrod Zero-1729

Labels
Build system

Milestone
24.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me