-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
backport: bump gcc and clang minimums; enforce c++20 #5971
base: develop
Are you sure you want to change the base?
Conversation
contrib/containers/ci/Dockerfile
Outdated
@@ -1,4 +1,4 @@ | |||
FROM ubuntu:focal | |||
FROM ubuntu:jammy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
focal
is still supported, it's 5 years, until April 2025.
can we still keep focal
for C++20? is gcc-10 available for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to bump it for CI because the version of g++-mingw-w64-x86-64
in focal in only based on gcc-9 whereas in jammy it's on gcc-10; and unlike most other gcc packages, there's no way to specify a version for it. I don't think bumping CI to jammy precludes anyone from running or building on focal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitcoin lets to use for different testing targets to use different ubuntu version, and even not ubuntu, but something like CentOS (in the past at least).
We have an option to use the one docker image for all CI tests. So, if we test with newer version ubuntu - the older one is not tested (and we won't know if build is broken). For newer version of ubuntu (the latest one) usually I run builds on my localhost :)
@UdjinM6 any though if we can use different docker images for different CI tests? I'd like to see the oldest supported (20.04 probably now) and the newest release (23.10 which will be superseeded by 24.04 soon).
could we get g++-mingw-w64-x86-64
version 10 from the third party repo maybe?
CI fails in unit tests due to difference in path, seems as on different version of boost returned path are different due to '/' at the end.
(I assume so, haven't tried to fix/deeply investigate) |
f59d40e
to
27a386e
Compare
I think this backport is problematic for experimental CI cxx20:
also, what's about this fix? knst@019824d |
This pull request has conflicts, please rebase. |
659fcb9
to
ebc5695
Compare
check this one also: knst@5e8921b |
Anyone know what'a going on here: https://gitlab.com/dashpay/dash/-/jobs/6614869261? |
this warning "error: volatile-qualified parameter type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]" becomes an error due to flag |
ci/test/00_setup_env_native_cxx20.sh
Outdated
@@ -13,5 +13,5 @@ export CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" | |||
export PYZMQ=true | |||
export RUN_FUNCTIONAL_TESTS=false | |||
export GOAL="install" | |||
export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20" | |||
export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports --enable-crash-hooks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this changes are super-seed this commit: ebc5695 (I assume)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
979a8c9: should probably rename linux64_cxx20
job (and ci/test/00_setup_env_native_cxx20.sh
file) now
…n to 10 fa5423b refactor: Remove unused gcc-9 workaround in txrequest (MarcoFalke) fa918d3 Always enable -Wsuggest-override (MarcoFalke) faea58e Bump g++ minimum supported version to 10 (MarcoFalke) Pull request description: All supported operating systems ship with g++ 10 (or later), so bumping the minimum should not cause any issues. The bump allows to drop some now-unused workarounds. For reference: * https://packages.debian.org/bullseye/g++ (`g++-10`) * https://packages.ubuntu.com/focal/g++-10 * FreeBSD 12/13 ships with g++ 12 * CentOS-like 9 ships with g++ 11 * OpenSuse Tumbleweed ships with g++ 13 https://software.opensuse.org/package/gcc13-c++ (No idea about OpenSuse Leap) This is for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month. ACKs for top commit: fanquake: ACK fa5423b Tree-SHA512: 6f0697ae4c0f578873591b7872bf158aba3af17f171c3556b593a70ec379bf94c7a9dd7697e8e79173edd4ac3c81a376e0cbbc0cfabde1a1cfe5f9b5eaea6831
fix: add update alternatives for arm gcc fix: add update alternatives for arm g++
1f8e5b2
to
9402366
Compare
@@ -39,7 +39,7 @@ $(package)_config_opts_release += -silent | |||
$(package)_config_opts_debug = -debug | |||
$(package)_config_opts_debug += -optimized-tools | |||
$(package)_config_opts += -bindir $(build_prefix)/bin | |||
$(package)_config_opts += -c++std c++17 | |||
$(package)_config_opts += -c++std c++2a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
28349 is partial: some code is supposed to be removed by this backport, but it is added only in bitcoin#27401.
ci/test/00_setup_env_native_cxx20.sh
Outdated
@@ -13,5 +13,5 @@ export CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" | |||
export PYZMQ=true | |||
export RUN_FUNCTIONAL_TESTS=false | |||
export GOAL="install" | |||
export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20" | |||
export BITCOIN_CONFIG="--enable-zmq --enable-reduce-exports --enable-crash-hooks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
979a8c9: should probably rename linux64_cxx20
job (and ci/test/00_setup_env_native_cxx20.sh
file) now
ff42d81 guix: use clang-toolchain-15 for macOS compilation (fanquake) 94955b4 depends: use LLVM/Clang 15.0.6 for macOS cross-compile (fanquake) Pull request description: This will end up being a blocker for bitcoin#28210, and is already part of bitcoin#21778, even though an even newer LLVM/Clang combination is required (and still missing from upstream Guix). Seems straight-forward enough to just bump the macOS compiler to a more modern Clang. ACKs for top commit: TheCharlatan: re-ACK ff42d81 Tree-SHA512: 8af4b54c3a56abb3825c6470444a28e14e9c69820c09ec4a33acebb8ae434df9ae18163c088a582130cc68755293a7e2bde5d065763919d94064ff9b3f83730f
fa7dada build: update ax_cxx_compile_stdcxx to serial 14 (MarcoFalke) Pull request description: No strong reason for the bump, but this makes it easier to experiment with cpp20, see bitcoin#24169 (comment) ACKs for top commit: Empact: Code Review ACK bitcoin@fa7dada fanquake: ACK fa7dada Tree-SHA512: bd3e884bae5319d434520a2947608913c3884de89aa563aa46ef17ba4e5b41ba209bd4780c8eaf81648297b2dc9534803be88d1b214ad05a93ee5992bee887c0
fa6e50d fuzz: Use C++20 starts_with in rpc.cpp (MarcoFalke) faa4838 Revert "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings" (MarcoFalke) fae3b77 refactor: Drop unused _Pragma to ignore -Wgnu-zero-variadic-macro-arguments (MarcoFalke) fa02fc0 refactor: modernize-use-default-member-init for bit-fields (C++20) (MarcoFalke) fa67f09 build: Require C++20 compiler (MarcoFalke) Pull request description: C++20 allows to write safer code, because it allows to enforce more stuff at compile time (`constinit`, `conteval`, `constexpr`, `std::span`, ...). Also, it allows to write less verbose and easier to understand code (C++ 20 Concepts). See bitcoin#23363 and https://en.cppreference.com/w/cpp/compiler_support#cpp20 With g++-10 (bitcoin#28348) and clang-13 (bitcoin#28210), there is broad support for almost all features of C++20. It should be fine to require a C++20 compiler for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month. This pull request includes three small cleanups to make use of C++20 features. If any issues are detected before or after merge, this should be easy to revert. If no issues arise, it should be fine to make use of more involved C++20 features later on. ACKs for top commit: fanquake: ACK fa6e50d Tree-SHA512: 244d79bfb0b750a4bdd713f40573b9ca33816fb84b6c84a58f027b9d7d4bb0cc4f18642959e4cf3d094808a69e5b8a327ca8521d7c0c08af27dacb5da3e78e71
9402366
to
b26c957
Compare
win64 build failed:
|
CI is passing now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guix build failed with
INFO: Building 20.1.1-525-gc9668b121751 for platform triple x86_64-apple-darwin:
...
/root/dash/contrib/guix/manifest.scm:618:17: error: clang-toolchain-15: unbound variable
hint: Did you forget a `use-modules' form?
… prepare for building with upstream LLVM 3df6070 contrib: remove macOS lazy_bind check (fanquake) 9bc357e build: explicitly opt-in to new fixup_chains functionality for darwin (Cory Fields) fb61bc0 depends: Bump MacOS minimum runtime requirement to 11.0 (Cory Fields) c2cd472 depends: bump darwin clang to 11.1 (Cory Fields) Pull request description: This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](bitcoin#21778) for building Darwin binaries. For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well. The commits may seem unrelated, so in detail: lld (llvm's linker) has been a work-in-progress for Darwin for years. Recently though, it has gained nearly all of the features we require. The last missing feature from ld64, `-Wl,-bind_at_load`, is not implemented in lld; as far as I can tell [lazy loading has conceptually been replaced by fixup chains](https://www.emergetools.com/blog/posts/iOS15LaunchTime). So that means we don't need ld64's `bind_at_load` as long as lld can handle `-Wl,-fixup_chains` (which it can). I've added it to our configure as a linker option mostly so that we can see it in the logs; it's default-on as long as the minimum version is >11.0. About that: the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp#L1021). Hence the commit that bumps the minimum version. Our current min runtime of `10.15` has been unsupported since September 2022, so I don't expect this bump to be controversial. Lastly, with the minimum runtime version bumped to 11.0, our current version of pre-compiled clang we use for macOS is too old to understand `-mmacosx-version-min=11.0` because it expects `=10.x`. So I've made the smallest possible bump (from 10.0.1 to 11.1.0) to a version that understands. This bump is arbitrary and unfortunate, but likely to be short-lived as we may end up replacing it with llvm+lld for v26 anyway. I've held off on bumping the SDK as I think that makes sense to do as part of the lld switch instead. ACKs for top commit: hebasto: ACK 3df6070 gruve-p: ACK bitcoin@3df6070 fanquake: ACK 3df6070 TheCharlatan: ACK 3df6070 Tree-SHA512: 0200ec4a3b88df33877ae82c15b5c04d745852550923f491a354b391cac65f88e4df116a40055c23a8cbcfcdfb9a376c6ae8fdd0e898e7b966bc213dcb5857cf
Guix Automation has began to build this PR tagged as v21.0.0-devpr5971.439f6d16. A new comment will be made when the image is pushed. |
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.0.0-devpr5971.439f6d16. The image should be on dockerhub soon. |
still fails with the same error... |
Some progress here https://gitlab.com/UdjinM6/dash/-/commits/pr5971: backporting 28294, 28324 and 28328 solves this specific issue it seems but then guix fails to build EDIT: also, boost 1.81 breaks wallet unit tests |
This pull request has conflicts, please rebase. |
Issue being fixed or feature implemented
Backport some PRs to move towards cpp20; this bumps clang and gcc versions as well as boost
What was done?
How Has This Been Tested?
Built
Breaking Changes
None really
Checklist:
Go over all the following points, and put an
x
in all the boxes that apply.