Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Mar 29, 2022
Merged

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

laanwj merged 1 commit into from
Mar 29, 2022

Conversation

@promag
Copy link
Contributor

@promag promag commented Mar 10, 2022

Rebased #24415 with Luke's suggestion.

Fixes #24413.

@promag
Copy link
Contributor Author

promag commented Mar 10, 2022

fyi @hebasto 🇺🇦

@promag promag mentioned this pull request Mar 10, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 11, 2022 •
edited
Loading

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

Conflicts

No conflicts as of last run.

fanquake added a commit that referenced this pull request Mar 11, 2022
@fanquake
Merge #24521: build: Fix Boost.Process detection on macOS arm64
f661da7 
1d4157a build: Fix Boost.Process detection on macOS arm64 (Hennadii Stepanov)

Pull request description:

  Could be tested as follows:
  ```
  % brew install boost@1.76
  % ./autogen.sh
  % ./configure --with-boost='/opt/homebrew/opt/boost@1.76'
  ```

ACKs for top commit:
  promag:
    Tested ACK on 1d4157a with boost 1.76 on macOS arm64. #24523 is required for boost 1.78.

Tree-SHA512: 7abd39a78e970ecc051e53b5923dfc31d3f0576cf4ff7fcfb9c8708c857c46a7a595ec36238def83f41158970eeee209980da4b8b70f0ff68f940a38ac9a0471
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 11, 2022
@fanquake
Merge bitcoin#24521: build: Fix Boost.Process detection on macOS arm64
8f46674 
1d4157a build: Fix Boost.Process detection on macOS arm64 (Hennadii Stepanov)

Pull request description:

  Could be tested as follows:
  ```
  % brew install boost@1.76
  % ./autogen.sh
  % ./configure --with-boost='/opt/homebrew/opt/boost@1.76'
  ```

ACKs for top commit:
  promag:
    Tested ACK on 1d4157a with boost 1.76 on macOS arm64. bitcoin#24523 is required for boost 1.78.

Tree-SHA512: 7abd39a78e970ecc051e53b5923dfc31d3f0576cf4ff7fcfb9c8708c857c46a7a595ec36238def83f41158970eeee209980da4b8b70f0ff68f940a38ac9a0471
promag
promag commented Mar 14, 2022
View reviewed changes
configure.ac Outdated
Copy link
Contributor Author

@promag promag Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@hebasto hebasto Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@hebasto hebasto Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@hebasto hebasto Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@promag promag Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll try that.

Copy link
Contributor Author

@promag promag Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed an alternative that only ignores the error locally.

hebasto
hebasto reviewed Mar 16, 2022
View reviewed changes
src/util/system.cpp Outdated
Comment on lines 9 to 14
Copy link
Member

@hebasto hebasto Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@hebasto hebasto Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

luke-jr commented Mar 17, 2022

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

@fanquake
Copy link
Member

fanquake commented Mar 17, 2022

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

Was #24413 (comment) unclear?

hebasto
hebasto approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Sjors
Copy link
Member

Sjors commented Mar 22, 2022

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

@Sjors Sjors mentioned this pull request Mar 29, 2022
Merged
@laanwj
Copy link
Member

laanwj commented Mar 29, 2022 •
edited
Loading

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 laanwj merged commit 9e32adb into bitcoin:master Mar 29, 2022
@hebasto
Copy link
Member

hebasto commented Mar 29, 2022

#24512 (comment):

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

@laanwj laanwj added this to the 23.0 milestone Mar 29, 2022
@jonatack
Copy link
Member

jonatack commented Mar 29, 2022

Backported to v23 in #24512.

@promag promag deleted the 220222-boost branch March 29, 2022 14:10
jonatack pushed a commit to jonatack/bitcoin that referenced this pull request Mar 31, 2022
@hebasto @jonatack
build: Fix Boost.Process test for Boost 1.78
ded10fe 
Github-Pull: bitcoin#24523
Rebased-From: 532c64a
fanquake added a commit that referenced this pull request Mar 31, 2022
@fanquake
Merge #24512: 23.x backports
c243e08 
174af33 util: Add inotify_rm_watch to syscall sandbox (AllowFileSystem) (Hennadii Stepanov)
ded10fe build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov)
26c2f23 build: Fix Boost.Process detection on macOS arm64 (Hennadii Stepanov)
85f85c7 util: add linkat to syscall sandbox (AllowFileSystem) (fanquake)
eaa0419 contrib: fix signet miner (sighash mismatch) (Sebastian Falbesoner)
235b042 rpc: Exclude descriptor when address is excluded (MarcoFalke)
b05a59b ci: Temporarily use clang-13 to work around clang-14 TSan bug (MarcoFalke)
65b9667 doc, init: add links to doc/cjdns.md (Jon Atack)
7a553d4 doc: update i2p.md with cjdns, improve local addresses section (Jon Atack)
4148396 doc: update tor.md with cjdns and getnodeaddresses, fix tor grep, (Jon Atack)
4690e8a doc: create initial doc/cjdns.md for cjdns how-to documentation (Jon Atack)
5d24f61 Clarify in -maxtimeadjustment that only outbound peers influence time data (Jon Atack)
b1646f1 test: set segwit height back to 0 on regtest (Martin Zumsande)
ef6a37b rpc: rename getdeploymentinfo status-next to status_next (Jon Atack)
2a6fcf9 init, doc: improve -onlynet help and tor/i2p documentation (Jon Atack)

Pull request description:

  Backport the following to 23.x:

  - #24468
  - #24528
  - #24527
  - #24609
  - #24555
  - #24663
  - #24572
  - #24636
  - #24553
  - #24659
  - #24521
  - #24523
  - #24690
  - #24710

  Possibly also:
  - #24579
  - #24691

ACKs for top commit:
  laanwj:
    List-of-commits ACK 174af33, I think we should merge this and move forward with rc3..
  hebasto:
    ACK 174af33

Tree-SHA512: 5a493e1652b780b527767d6ca9e67012abd2fa5573496e85e0d8aa4bed3eb332bfcd72610b8dfb954ff274d42450623233c96c479de2085b9c8344ba5abf1935
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 3, 2022
@laanwj
Merge bitcoin#24523: build: Fix Boost.Process test for Boost 1.78
dd7ed96 
532c64a build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov)

Pull request description:

  Rebased bitcoin#24415 with Luke's suggestion.

  Fixes bitcoin#24413.

ACKs for top commit:
  hebasto:
    ACK 532c64a, tested on Mac mini (M1, 2020) + macOS Monterey 12.3 (21E230).

Tree-SHA512: 74f779695f6bbc45a2b7341a1402f747cc0d433d74825c7196cb9f156db0c0299895365f01665bd0bff12a8ebb5ea33a29b9a52f5eac0007ec35d1dca6544705
@fanquake fanquake mentioned this pull request Apr 18, 2022
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Jun 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

Assignees

@fanquake fanquake

Labels

Build system

Projects

None yet

Milestone

23.0

Development

Successfully merging this pull request may close these issues.

Homebrew Boost 1.78 breaks external signer configure

8 participants

@promag @DrahtBot @luke-jr @fanquake @Sjors @laanwj @hebasto @jonatack