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.

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

build: Require libc++-16 or later #29077

Closed
wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Dec 14, 2023

I am not familiar with macOS, but given that #28622 prescribes a minimum of libc++-16 for macOS, doing the same on Linux could make sense?

This is needed for stuff:

I don't think anyone is using -stdlib=libc++ on Linux, except for the CI and OSS-Fuzz?

The minimum required clang version remains at clang++-15.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 14, 2023

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.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)
  • #28687 (C++20 std::views::reverse by stickies-v)

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.

@maflcko
Copy link
Member Author

maflcko commented Dec 14, 2023

Draft for now to gather conceptual feedback first.

@@ -8,7 +8,7 @@ You can find installation instructions in the `build-*.md` file for your platfor
| --- | --- |
| [Autoconf](https://www.gnu.org/software/autoconf/) | [2.69](https://github.com/bitcoin/bitcoin/pull/17769) |
| [Automake](https://www.gnu.org/software/automake/) | [1.13](https://github.com/bitcoin/bitcoin/pull/18290) |
| [Clang](https://clang.llvm.org) | [13.0](https://github.com/bitcoin/bitcoin/pull/28210) |
| [Clang](https://clang.llvm.org) | [13.0](https://github.com/bitcoin/bitcoin/pull/28210) (with at least libstdc++-10, or at least [libc++ 16.0](https://github.com/bitcoin/bitcoin/pull/29077))|
Copy link
Member

Choose a reason for hiding this comment

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

Given #28687 (comment) and https://godbolt.org/z/n4oTE9sTE the libstdc++-10 requirement isn't going to work here? Looks like this really needs to be clang-16.

Copy link
Member Author

Choose a reason for hiding this comment

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

So as long as you don't need deferred concepts instantiation, it should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

More missing in v10: #29081 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, clarified that "libstdc++ (any version) requires at least clang-16"

@fanquake
Copy link
Member

and OSS-Fuzz?

So won't this break as soon as we have code that requires libstdc++-16, given that OSS-Fuzz is still pinned to 15?

Are there no other projects using (some?) C++20 code on OSS-Fuzz?

@maflcko maflcko changed the title build: Require libc++-16 or later [WIP, DO NOT MERGE!!11111!!!!!] build: Require libc++-16 or later Dec 14, 2023
@maflcko maflcko modified the milestones: 27.0, 28.0 Dec 15, 2023
@maflcko maflcko removed this from the 28.0 milestone Apr 10, 2024
@maflcko maflcko changed the title [WIP, DO NOT MERGE!!11111!!!!!] build: Require libc++-16 or later build: Require libc++-16 or later May 6, 2024
@maflcko maflcko marked this pull request as ready for review May 6, 2024 10:21
@maflcko maflcko marked this pull request as draft May 6, 2024 10:43
@maflcko
Copy link
Member Author

maflcko commented May 7, 2024

Not sure if there is much value in being overly accurate here (clang-15 and libc++-16 or clang-16 and libstdc++). If someone can install clang-16 or libc++-16, they can probably also install the other as well?

So I'd say to close this and bump to 16 wholesale?

@fanquake
Copy link
Member

fanquake commented May 8, 2024

So I'd say to close this and bump to 16 wholesale?

I think generally, that would be more straightforward, then supporting varying compiler + std lib combinations, and seems possible to do at this point.

@maflcko maflcko closed this May 8, 2024
@maflcko maflcko deleted the 2312-libc++-16- branch May 8, 2024 09:11
fanquake added a commit that referenced this pull request May 15, 2024
fa90ad2 ci: Roll test-each-commit Ubuntu (MarcoFalke)
fa6c82d ci: Remove clang version pin in test-each-commit (MarcoFalke)

Pull request description:

  Needed for #29077 (comment)

ACKs for top commit:
  hebasto:
    re-ACK fa90ad2.

Tree-SHA512: 753a3a77d967f308b5b5dd0bc7ea9f3268fc93c5ac978da3d79b85461cb1e994c6ac481888dc472b9a08be45ad0fad66ad3fda241a8955f999b7c2cb2a2b1f58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants