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

fanquake · 2022-09-09T16:57:47Z

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

hebasto · 2022-09-09T17:03:27Z

Concept ACK.

theuni · 2022-09-09T17:28:48Z

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: theuni@400eede

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: theuni@e131d8f . Then I rebuilt everything and added BOOST_CPPFLAGS one-by-one as needed to fix compilation.

theuni · 2022-09-09T17:34:47Z

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

hebasto · 2022-09-09T21:54:57Z

@theuni

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

With flag reordering as follows:

diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index 4fe79652e..602a11825 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -295,7 +295,7 @@ BITCOIN_QT_RC = qt/res/bitcoin-qt-res.rc
 BITCOIN_QT_INCLUDES = -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER
 
 qt_libbitcoinqt_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS)
+  $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS) $(BOOST_CPPFLAGS)
 qt_libbitcoinqt_a_CXXFLAGS = $(AM_CXXFLAGS) $(QT_PIE_FLAGS)
 qt_libbitcoinqt_a_OBJCXXFLAGS = $(AM_OBJCXXFLAGS) $(QT_PIE_FLAGS)
 
diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
index afcd4106b..89c659d4b 100644
--- a/src/Makefile.qttest.include
+++ b/src/Makefile.qttest.include
@@ -27,7 +27,7 @@ TEST_QT_H = \
   qt/test/wallettests.h
 
 qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_TEST_INCLUDES)
+  $(QT_INCLUDES) $(QT_TEST_INCLUDES) $(BOOST_CPPFLAGS)
 
 qt_test_test_bitcoin_qt_SOURCES = \
   init/bitcoin-qt.cpp \

your commit fixes #25947 as well.

fanquake · 2022-09-10T09:34:55Z

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.

hebasto · 2022-09-10T11:35:28Z

This diff:

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

fixes macOS native build.

Sjors · 2022-09-12T08:03:24Z

Concept ACK

fanquake · 2022-09-12T08:04:17Z

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:

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

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.

theuni · 2022-09-12T13:49:42Z

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

theuni · 2022-09-12T15:38:58Z

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?

maflcko · 2022-09-12T15:48:39Z

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:

bitcoin/.cirrus.yml

Line 332 in 5558d2f

CI_USE_APT_INSTALL: "no"

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

theuni · 2022-09-12T17:37:54Z

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: theuni@7929a2a (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.

theuni · 2022-09-12T21:40:37Z

@theuni

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

With flag reordering as follows:

diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index 4fe79652e..602a11825 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -295,7 +295,7 @@ BITCOIN_QT_RC = qt/res/bitcoin-qt-res.rc
 BITCOIN_QT_INCLUDES = -DQT_NO_KEYWORDS -DQT_USE_QSTRINGBUILDER
 
 qt_libbitcoinqt_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS)
+  $(QT_INCLUDES) $(QT_DBUS_INCLUDES) $(QR_CFLAGS) $(BOOST_CPPFLAGS)
 qt_libbitcoinqt_a_CXXFLAGS = $(AM_CXXFLAGS) $(QT_PIE_FLAGS)
 qt_libbitcoinqt_a_OBJCXXFLAGS = $(AM_OBJCXXFLAGS) $(QT_PIE_FLAGS)
 
diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
index afcd4106b..89c659d4b 100644
--- a/src/Makefile.qttest.include
+++ b/src/Makefile.qttest.include
@@ -27,7 +27,7 @@ TEST_QT_H = \
   qt/test/wallettests.h
 
 qt_test_test_bitcoin_qt_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BITCOIN_QT_INCLUDES) \
-  $(BOOST_CPPFLAGS) $(QT_INCLUDES) $(QT_TEST_INCLUDES)
+  $(QT_INCLUDES) $(QT_TEST_INCLUDES) $(BOOST_CPPFLAGS)
 
 qt_test_test_bitcoin_qt_SOURCES = \
   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.

fanquake · 2022-09-13T09:38:50Z

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.

fanquake · 2022-09-13T11:04:46Z

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.

…rs installed from homebrew b50a4b7 build: quiet warnings in system headers installed from homebrew (Cory Fields) Pull request description: From the included comment: Homebrew may create symlinks in `/usr/local/include` for some packages. Because MacOS's clang internally adds `-I /usr/local/include` to its search paths, this will negate efforts to use `-isystem` for those packages, as they will be found first in `/usr/local`. Use the internal `-internal-isystem` option to system-ify all `/usr/local/include` paths without adding it to the list of search paths in case it's not already there. This fixes the issue explained here: bitcoin/bitcoin#26056 (comment) ~Also temporarily includes #26056 as a test. I will remove that commit if/when c-i is happy, and fanquake can rebase it post-merge.~ I've removed this commit now that c-i succeeded with it. ACKs for top commit: hebasto: ACK b50a4b7, tested as a part of bitcoin/bitcoin#26056 on macOS Monterey 12.6 (21G115, both Intel and Apple M1) + Apple clang 14.0.0: Tree-SHA512: 163aa359d27c31d52b444252762e32dd8a11acc043cf1a2aa953f902d1dab77ece52e2dfedcce637e6a1dda47e4c566bfeb8d3b092f82bfc73923843b7bc619c

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

fanquake · 2022-09-13T16:16:18Z

Rebased for #26070.

…lled from homebrew b50a4b7 build: quiet warnings in system headers installed from homebrew (Cory Fields) Pull request description: From the included comment: Homebrew may create symlinks in `/usr/local/include` for some packages. Because MacOS's clang internally adds `-I /usr/local/include` to its search paths, this will negate efforts to use `-isystem` for those packages, as they will be found first in `/usr/local`. Use the internal `-internal-isystem` option to system-ify all `/usr/local/include` paths without adding it to the list of search paths in case it's not already there. This fixes the issue explained here: bitcoin#26056 (comment) ~Also temporarily includes bitcoin#26056 as a test. I will remove that commit if/when c-i is happy, and fanquake can rebase it post-merge.~ I've removed this commit now that c-i succeeded with it. ACKs for top commit: hebasto: ACK b50a4b7, tested as a part of bitcoin#26056 on macOS Monterey 12.6 (21G115, both Intel and Apple M1) + Apple clang 14.0.0: Tree-SHA512: 163aa359d27c31d52b444252762e32dd8a11acc043cf1a2aa953f902d1dab77ece52e2dfedcce637e6a1dda47e4c566bfeb8d3b092f82bfc73923843b7bc619c

theuni · 2022-09-13T20:43:06Z

ACK 12de8f6

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.

kouloumos · 2022-09-13T20:46:56Z

Verified that it fixes #25947.

Tested on macOS 10.15.7 with

$ brew list --version | grep qt
qt 6.3.1_3
qt@5 5.15.5_2 5.15.3

hebasto

ACK 12de8f6, tested on macOS Monterey 12.6 (21G115, Intel).

jarolrod

ACK 12de8f6

… is not supported" 34a2f91 Revert "doc: note that brew installed qt is not supported" (Hennadii Stepanov) Pull request description: As bitcoin/bitcoin#26056 fixes bitcoin/bitcoin#25947 it looks reasonable to revert bitcoin/bitcoin#21988. ACKs for top commit: fanquake: ACK 34a2f91 - haven't tested at all. jarolrod: ACK bitcoin/bitcoin@34a2f91 Tree-SHA512: 4470f21fb6ea32970d7572c83ba064bcbe6e3282cea79122312f8ac203a5b1617b21952db1d6e47ba5b6f605abc23f72c04c07cef7251272e22fb593ff317beb

…INCLUDES)

…supported" 34a2f91 Revert "doc: note that brew installed qt is not supported" (Hennadii Stepanov) Pull request description: As bitcoin#26056 fixes bitcoin#25947 it looks reasonable to revert bitcoin#21988. ACKs for top commit: fanquake: ACK 34a2f91 - haven't tested at all. jarolrod: ACK bitcoin@34a2f91 Tree-SHA512: 4470f21fb6ea32970d7572c83ba064bcbe6e3282cea79122312f8ac203a5b1617b21952db1d6e47ba5b6f605abc23f72c04c07cef7251272e22fb593ff317beb

Zero-1729 · 2022-09-17T10:30:54Z

Posthumous ACK 12de8f6

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

Thanks @fanquake!

fanquake added the Build system label Sep 9, 2022

fanquake mentioned this pull request Sep 9, 2022

test: remove Boost Test from libtest_util #26009

Merged

hebasto mentioned this pull request Sep 9, 2022

MacOS Build Error: Qt 5 and 6 Simultaneously Installed. #25947

Closed

fanquake force-pushed the finer_boost_cppflags branch from 3549509 to eb8b52d Compare September 10, 2022 09:34

fanquake marked this pull request as ready for review September 10, 2022 09:34

hebasto mentioned this pull request Sep 10, 2022

doc: added note about brew and Qt 6 on macOS #25948

Closed

theuni mentioned this pull request Sep 12, 2022

build: Quiet warnings in symlinked headers installed from homebrew #26070

Merged

fanquake force-pushed the finer_boost_cppflags branch from eb8b52d to 814582b Compare September 13, 2022 09:09

fanquake added this to the 24.0 milestone Sep 13, 2022

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

12de8f6

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

fanquake force-pushed the finer_boost_cppflags branch from 814582b to 12de8f6 Compare September 13, 2022 16:15

hebasto approved these changes Sep 13, 2022

View reviewed changes

hebasto mentioned this pull request Sep 13, 2022

Revert "doc: note that brew installed qt is not supported" #26079

Merged

jarolrod approved these changes Sep 14, 2022

View reviewed changes

fanquake merged commit 13fd9ee into bitcoin:master Sep 14, 2022

fanquake deleted the finer_boost_cppflags branch September 14, 2022 08:00

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 14, 2022

Merge bitcoin#26056: build: extract $(BOOST_CPPFLAGS) from $(SYSCOIN_…

72669d6

…INCLUDES)

bitcoin locked and limited conversation to collaborators Sep 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fanquake commented Sep 9, 2022 •

edited

Loading

hebasto commented Sep 9, 2022

theuni commented Sep 9, 2022 •

edited

Loading

theuni commented Sep 9, 2022 •

edited

Loading

hebasto commented Sep 9, 2022

fanquake commented Sep 10, 2022

hebasto commented Sep 10, 2022

Sjors commented Sep 12, 2022

fanquake commented Sep 12, 2022

theuni commented Sep 12, 2022

theuni commented Sep 12, 2022

maflcko commented Sep 12, 2022

theuni commented Sep 12, 2022 •

edited

Loading

theuni commented Sep 12, 2022

fanquake commented Sep 13, 2022

fanquake commented Sep 13, 2022

fanquake commented Sep 13, 2022

theuni commented Sep 13, 2022

kouloumos commented Sep 13, 2022

hebasto left a comment

jarolrod left a comment

Zero-1729 commented Sep 17, 2022

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

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

Conversation

fanquake commented Sep 9, 2022 • edited Loading

hebasto commented Sep 9, 2022

theuni commented Sep 9, 2022 • edited Loading

theuni commented Sep 9, 2022 • edited Loading

hebasto commented Sep 9, 2022

fanquake commented Sep 10, 2022

hebasto commented Sep 10, 2022

Sjors commented Sep 12, 2022

fanquake commented Sep 12, 2022

theuni commented Sep 12, 2022

theuni commented Sep 12, 2022

maflcko commented Sep 12, 2022

theuni commented Sep 12, 2022 • edited Loading

theuni commented Sep 12, 2022

fanquake commented Sep 13, 2022

fanquake commented Sep 13, 2022

fanquake commented Sep 13, 2022

theuni commented Sep 13, 2022

kouloumos commented Sep 13, 2022

hebasto left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

Zero-1729 commented Sep 17, 2022

fanquake commented Sep 9, 2022 •

edited

Loading

theuni commented Sep 9, 2022 •

edited

Loading

theuni commented Sep 9, 2022 •

edited

Loading

theuni commented Sep 12, 2022 •

edited

Loading