Skip to content

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

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
Closed
hebasto wants to merge 1 commit into from
Closed

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

hebasto wants to merge 1 commit into from

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Feb 22, 2022

Fixes #24413.

An alternative to #24414.

@hebasto hebasto mentioned this pull request Feb 22, 2022
Closed
@Sjors Sjors mentioned this pull request Feb 22, 2022
Closed
@Sjors
Copy link
Member

Sjors commented Feb 22, 2022

tACK d092dfb

@Sjors
Copy link
Member

Sjors commented Feb 22, 2022 •
edited
Loading

The MSVC failure seems unrelated (see #24151):

 test  2022-02-22T11:44:36.374000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
                                       self.run_test()
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\wallet_send.py", line 560, in run_test
                                       assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001))
                                     File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\util.py", line 46, in assert_fee_amount
                                       raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
                                   AssertionError: Fee of 0.00003160 BTC too high! (Should be 0.0000313 BTC)

@Sjors Sjors mentioned this pull request Feb 22, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 22, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24397 (build: Fix Boost.Process check for Boost 1.73 and older by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Feb 22, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 28, 2022

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

luke-jr
luke-jr suggested changes Mar 7, 2022
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

Seems like -Werror should simply always use --enable-suppress-external-warnings

And never enable -Werror for configure checks...

configure.ac
TEMP_CXXFLAGS="$CXXFLAGS"
dnl Boost 1.78 requires the following workaround.
dnl See: https://github.com/boostorg/process/issues/235
CXXFLAGS="$CXXFLAGS -Wno-narrowing"
Copy link
Member

@luke-jr luke-jr Mar 7, 2022

Choose a reason for hiding this comment

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

Suggested change
CXXFLAGS="$CXXFLAGS -Wno-narrowing"
CXXFLAGS="$CXXFLAGS -Wno-error=narrowing"

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2022

Not able to continue to work on this PR (no access to macOS x86_64), and will be happy if someone grab it.

@hebasto hebasto closed this Mar 10, 2022
@maflcko
Copy link
Member

maflcko commented Mar 10, 2022

Looks like this had a tested ACK, so if we want it, it can be merged as-is, unless there are concerns with it.

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2022

Looks like this had a tested ACK, so if we want it, it can be merged as-is, unless there are concerns with it.

It requires rebasing, and @luke-jr's suggestion deserves testing.

@maflcko
Copy link
Member

maflcko commented Mar 10, 2022

Ah sorry, I missed the rebase label.

@promag promag mentioned this pull request Mar 10, 2022
Merged
laanwj added a commit that referenced this pull request Mar 29, 2022
@laanwj
Merge #24523: build: Fix Boost.Process test for Boost 1.78
9e32adb 
532c64a build: Fix Boost.Process test for Boost 1.78 (Hennadii Stepanov)

Pull request description:

  Rebased #24415 with Luke's suggestion.

  Fixes #24413.

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

Tree-SHA512: 74f779695f6bbc45a2b7341a1402f747cc0d433d74825c7196cb9f156db0c0299895365f01665bd0bff12a8ebb5ea33a29b9a52f5eac0007ec35d1dca6544705
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
@hebasto hebasto deleted the 220222-boost branch April 4, 2022 13:31
@bitcoin bitcoin locked and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@luke-jr luke-jr luke-jr requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system Needs rebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Homebrew Boost 1.78 breaks external signer configure

6 participants

@hebasto @Sjors @DrahtBot @maflcko @luke-jr @fanquake