build: Fix Boost.Process test for Boost 1.78

promag commented at 1:14 pm on March 10, 2022: member

Rebased #24415 with Luke’s suggestion.

Fixes #24413.

promag commented at 1:15 pm on March 10, 2022: member

fyi @hebasto 🇺🇦

DrahtBot added the label Build system on Mar 10, 2022

DrahtBot commented at 6:10 am on March 11, 2022: member

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

Conflicts

No conflicts as of last run.

fanquake referenced this in commit f661da70b9 on Mar 11, 2022

DrahtBot added the label Needs rebase on Mar 11, 2022

sidhujag referenced this in commit 8f466748fa on Mar 11, 2022

promag force-pushed on Mar 14, 2022

DrahtBot removed the label Needs rebase on Mar 14, 2022

in configure.ac:1452 in 2a3f3d0d73 outdated

1445@@ -1446,6 +1446,10 @@ if test "$use_external_signer" != "no"; then
1446     ;;
1447     *)
1448       AC_MSG_CHECKING([whether Boost.Process can be used])
1449+      TEMP_CXXFLAGS="$CXXFLAGS"
1450+      dnl Boost 1.78 requires the following workaround.
1451+      dnl See: https://github.com/boostorg/process/issues/235
1452+      CXXFLAGS="$CXXFLAGS -Wno-error=narrowing"

promag commented at 10:30 am on March 14, 2022:

@hebasto I have to keep this flag in order to build, not sure what is the correct approach is, or if we only want to do it when boost=1.78.

hebasto commented at 10:38 am on March 14, 2022:

The reverse CXXFLAGS="$TEMP_CXXFLAGS" is required after the test.

promag commented at 10:45 am on March 14, 2022:

But then it won’t compile src/util/system.cpp, gives the same error.

hebasto commented at 11:44 am on March 14, 2022:

The reverse CXXFLAGS="$TEMP_CXXFLAGS" is required after the test.

I mean, https://github.com/hebasto/bitcoin/commits/220222-boost-m

But then it won’t compile src/util/system.cpp, gives the same error.

What configuration with?

hebasto commented at 12:22 pm on March 14, 2022:

But then it won’t compile src/util/system.cpp, gives the same error.

Then something like https://github.com/hebasto/bitcoin/commits/220222-boost-m2? It keeps the -Wno-error=narrowing flag only when it is necessary.

promag commented at 9:34 am on March 15, 2022:

Ok, I’ll try that.

promag commented at 11:37 am on March 15, 2022:

Pushed an alternative that only ignores the error locally.

promag force-pushed on Mar 15, 2022

in src/util/system.cpp:14 in 0f410ca815 outdated

 5@@ -6,7 +6,16 @@
 6 #include <util/system.h>
 7 
 8 #ifdef ENABLE_EXTERNAL_SIGNER
 9+#if defined(__GNUC__) && !defined(__clang__)
10+// Boost 1.78 requires the following workaround.
11+// See: https://github.com/boostorg/process/issues/235
12+#pragma GCC diagnostic push
13+#pragma GCC diagnostic ignored "-Wnarrowing"
14+#endif

hebasto commented at 10:37 am on March 16, 2022:

It does not work on macOS, where __clang__ is defined.

hebasto commented at 11:10 am on March 16, 2022:

Also the similar changes should be applied to src/test/system_tests.cpp.

promag commented at 4:56 pm on March 21, 2022:

According to docs clang processes pragma GCC, so just dropped && !defined(__clang__). Also added in the test file. I wonder if we should add a header file to avoid repeating this.

luke-jr commented at 0:15 am on March 17, 2022: member

Still not clear to me how -Werror is getting enabled for this in the first place. :/

fanquake commented at 6:49 am on March 17, 2022: member

Still not clear to me how -Werror is getting enabled for this in the first place. :/

Was #24413 (comment) unclear?

build: Fix Boost.Process test for Boost 1.78 532c64a726

promag force-pushed on Mar 21, 2022

hebasto approved

hebasto commented at 7:02 pm on March 21, 2022: member

ACK 532c64a7264dd3c7329e8839547837c57da7dbe8, tested on Mac mini (M1, 2020) + macOS Monterey 12.3 (21E230).

Sjors commented at 2:12 pm on March 22, 2022: member

Tested that 532c64a7264dd3c7329e8839547837c57da7dbe8 compiles and runs the tests on macOS 12.3 on an Intel mac.

MarcoFalke assigned fanquake on Mar 29, 2022

laanwj commented at 11:13 am on March 29, 2022: member

Still not clear to me how -Werror is getting enabled for this in the first place. :/

I had a similar comment about this on IRC

02022-03-29 13:03:47     laanwj  it's kind of an ugly workaround though, not sure why the warning matters so much
12022-03-29 13:05:04     laanwj  i wouldn't really like cluttering gcc pragmas over the code to silence warnings in third-party deps to become a common thing
22022-03-29 13:07:10     laanwj  "just don't use Werror with broken deps" would be fine with me too

I might be missing some pressing reason for this, of course.

Edit: ok just read #24413 (comment) , it’s not about Werror but clang regarding this as a compilation error always.

laanwj merged this on Mar 29, 2022

laanwj closed this on Mar 29, 2022

hebasto commented at 11:38 am on March 29, 2022: member

#24512 (comment):

#24523 would be a good backport candidate too, if it happens to be merged before the next release candidate

laanwj added this to the milestone 23.0 on Mar 29, 2022

laanwj added the label Needs backport (23.x) on Mar 29, 2022

jonatack commented at 2:09 pm on March 29, 2022: member

Backported to v23 in #24512.

promag deleted the branch on Mar 29, 2022

fanquake removed the label Needs backport (23.x) on Mar 29, 2022

jonatack referenced this in commit 5dbed641eb on Mar 29, 2022

jonatack referenced this in commit 4a668a36f2 on Mar 30, 2022

hebasto referenced this in commit 9c755df7d6 on Mar 31, 2022

jonatack referenced this in commit ded10fe3ea on Mar 31, 2022

fanquake referenced this in commit c243e08351 on Mar 31, 2022

sidhujag referenced this in commit dd7ed96fc1 on Apr 3, 2022

DrahtBot locked this on Jun 24, 2023

build: Fix Boost.Process test for Boost 1.78 #24523

Conflicts