Skip to content
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

Bump minimum required Boost version due to migration to C++20 #29066

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Bump minimum required Boost version due to migration to C++20 #29066

merged 1 commit into from
Dec 13, 2023

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Dec 12, 2023

Boost versions <1.73 have C++20-specific bugs that were fixed in the following commits:

I tested libboost1.71-dev in Ubuntu 20.04 and Boost 1.71, 1.72, 1.73 in our depends build system.

Closes #29063.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 12, 2023 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake
Stale ACK maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Docs label Dec 12, 2023
@hebasto hebasto mentioned this pull request Dec 12, 2023
Closed
@maflcko
Copy link
Member

maflcko commented Dec 12, 2023

boostorg/signals2@15fcf21

This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.

@maflcko
Copy link
Member

maflcko commented Dec 12, 2023

lgtm ACK 1e0ed3b

@hebasto hebasto changed the title doc: Bump minimum required Boost version due to migration to C++20 Bump minimum required Boost version due to migration to C++20 Dec 12, 2023
@hebasto
Copy link
Member Author

hebasto commented Dec 12, 2023

Oops... Forgot to update a version in the AX_BOOST_BASE macro.

Fixed now.

Sorry.

@fanquake
Copy link
Member

This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.

I know a [WIP] branch for this "exists" in some form. cc @theuni

@maflcko maflcko added this to the 27.0 milestone Dec 12, 2023
@maflcko maflcko removed the Docs label Dec 12, 2023
@maflcko
Copy link
Member

maflcko commented Dec 12, 2023

Should be fine to squash a 2-line diff into one commit?

@hebasto
build: Bump minimum required Boost to 1.73.0 to support C++20
49a9091 
Boost versions <1.73 have C++20-specific bugs that were fixed in the
following commits:
- boostorg/signals2@15fcf21
- boostorg/test@495c095
@hebasto
Copy link
Member Author

hebasto commented Dec 12, 2023

Should be fine to squash a 2-line diff into one commit?

Sure :)

fanquake
fanquake approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 49a9091

@DrahtBot DrahtBot requested a review from maflcko December 13, 2023 13:06
@fanquake fanquake merged commit 0830dcb into bitcoin:master Dec 13, 2023
16 checks passed
@hebasto hebasto deleted the 231212-boost branch December 13, 2023 14:44
@theuni
Copy link
Member

theuni commented Dec 14, 2023

This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.

I know a [WIP] branch for this "exists" in some form. cc @theuni

It's not a WIP so much as a POC: https://github.com/theuni/bitcoin/commits/replace-boost-signals/

That implements the subset of signals that we rely on as a drop-in replacement. It passes tests and would serve as a good guide, but I don't think that's how we'd want to do it.

I could look at picking it up again and doing it right if there's interest.

@maflcko
Copy link
Member

maflcko commented Dec 14, 2023

I was looking in moving boost signals2 into one translation unit (because it is so heavy and keeps the compiler busy), to only expose the stuff that is needed, but if it is possible to re-implement signals2 in 150 LOC, that seems preferable?

@theuni
Copy link
Member

theuni commented Dec 14, 2023

@maflcko IIRC I wanted to fix #26442 before going forward with the replacement, as that makes the implementation more straightforward and greatly simplifies testing. Any interest in taking a look there?

hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 16, 2023
@hebasto
fixup! cmake: Build bitcoind executable
a3a236c 
Mirror changes from bitcoin#29066
@hebasto hebasto mentioned this pull request Dec 16, 2023
Merged
hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 16, 2023
@hebasto
Merge #66: cmake: Backport bumping minimum required Boost version
831b0d7 
a3a236c fixup! cmake: Build `bitcoind` executable (Hennadii Stepanov)

Pull request description:

  This PR mirrors changes from bitcoin#29066.

Top commit has no ACKs.

Tree-SHA512: faf01c231abffbfdf725951e2dc2ece4bac0de430c4ce972ae797b09e04caeb8d67c7a13c4b9df4f46a470e5c96eff053d002afde87af281dd260a1f15161f62
@hebasto hebasto mentioned this pull request May 29, 2024
Closed
@maflcko maflcko mentioned this pull request Sep 4, 2024
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 4, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29066: Bump minimum required Boost version due to migra…
facd1fb 
…tion to C++20

49a9091 build: Bump minimum required Boost to 1.73.0 to support C++20 (Hennadii Stepanov)

Pull request description:

  Boost versions <1.73 have C++20-specific bugs that were fixed in the following commits:
  - boostorg/signals2@15fcf21
  - boostorg/test@495c095

  I tested [`libboost1.71-dev`](https://packages.ubuntu.com/focal/libboost1.71-dev) in Ubuntu 20.04 and Boost 1.71, 1.72, 1.73 in our depends build system.

  Closes bitcoin#29063.

ACKs for top commit:
  fanquake:
    ACK 49a9091

Tree-SHA512: b8ebc08af85abfa3fda70961bd1136ee9e5149dd76a3f901e43acba624d231971873cba5cbf30837f9e5ab58790b8330f241a76cb76d8cf5dce5ad0cca33fba8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 4, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29066: Bump minimum required Boost version due to migra…
b7b48cd 
…tion to C++20

49a9091 build: Bump minimum required Boost to 1.73.0 to support C++20 (Hennadii Stepanov)

Pull request description:

  Boost versions <1.73 have C++20-specific bugs that were fixed in the following commits:
  - boostorg/signals2@15fcf21
  - boostorg/test@495c095

  I tested [`libboost1.71-dev`](https://packages.ubuntu.com/focal/libboost1.71-dev) in Ubuntu 20.04 and Boost 1.71, 1.72, 1.73 in our depends build system.

  Closes bitcoin#29063.

ACKs for top commit:
  fanquake:
    ACK 49a9091

Tree-SHA512: b8ebc08af85abfa3fda70961bd1136ee9e5149dd76a3f901e43acba624d231971873cba5cbf30837f9e5ab58790b8330f241a76cb76d8cf5dce5ad0cca33fba8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 7, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29066: Bump minimum required Boost version due to migra…
49fcd4a 
…tion to C++20

49a9091 build: Bump minimum required Boost to 1.73.0 to support C++20 (Hennadii Stepanov)

Pull request description:

  Boost versions <1.73 have C++20-specific bugs that were fixed in the following commits:
  - boostorg/signals2@15fcf21
  - boostorg/test@495c095

  I tested [`libboost1.71-dev`](https://packages.ubuntu.com/focal/libboost1.71-dev) in Ubuntu 20.04 and Boost 1.71, 1.72, 1.73 in our depends build system.

  Closes bitcoin#29063.

ACKs for top commit:
  fanquake:
    ACK 49a9091

Tree-SHA512: b8ebc08af85abfa3fda70961bd1136ee9e5149dd76a3f901e43acba624d231971873cba5cbf30837f9e5ab58790b8330f241a76cb76d8cf5dce5ad0cca33fba8
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 8, 2024
@PastaPastaPasta
Merge #6313: backport: bump boost min
0c39d14 
c0154c0 partial merge bitcoin#27783: Add public Boost headers explicitly (fanquake)
49fcd4a Merge bitcoin#29066: Bump minimum required Boost version due to migration to C++20 (fanquake)
355a69c Merge bitcoin#24558: build: explicitly disable Boost multi_index serialization (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  See commit

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK c0154c0
  knst:
    utACK c0154c0

Tree-SHA512: d11045903e1b665b8bbb21326ce3f9b1ee2c83b881e48a08482f1c5103c7b9909f1defac27b222fa28ee6c1ae52c98c924850eb0a38993e53d6008c81791181d
@bitcoin bitcoin locked and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

@maflcko maflcko Awaiting requested review from maflcko

Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
27.0
Development

Successfully merging this pull request may close these issues.

build: GCC 10.5 fails to compile test_bitcoin using Boost.Test 1.71
5 participants
@hebasto @DrahtBot @maflcko @fanquake @theuni