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

pull promag wants to merge 1 commits into bitcoin:master from promag:220222-boost changing 3 files +23 −0

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 12: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

2022-03-29 13:03:47     laanwj  it's kind of an ugly workaround though, not sure why the warning matters so much
2022-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
2022-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

Contributors

Labels

Milestone
23.0

Linked (view graph)

#24413 Homebrew Boost 1.78 breaks external signer configure #24415 build: Fix Boost.Process test for Boost 1.78 #24512 23.x backports #24907 RFC: Replacing Boost Process

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

Conflicts