release: increase minimum compiler and lib(std)c++ requirements #23060

fanquake · 2021-09-22T08:19:20Z

This increases the minimum required compiler versions to Clang 7 and GCC 8.1. This has been split out of #20744 (migration to std::filesystem), as it's also a requirement for some other changes, such as #20452 or #20457 which want to make use of std::from_chars. As well as #20435, which is also std::filesystem related. Given that the std::filesystem changes are moving ahead, splitting out this change to let other PRs take advantage of the new requirements seems worthwhile.

Clang 7 has been available in Debian since Stretch (oldoldstable) and in Ubuntu since Bionic (18.04). GCC 8 has been available in Debian since Buster (oldstable) and in Ubuntu since Bionic (18.04). CentOS 8 also packages GCC 8.

The CI changes here give us one build with GCC 8, and another using Clang 7 on top of libc++.

Note that the minimum required libc++ in dependencies.md is unchanged as, at least for <filesystem>, and the *_chars use cases, libc++ 7 should be sufficient.

I've tested that building <filesystem> code using Clang 7 & libc++ works. i.e clang++-7 -std=c++17 fs.cpp -stdlib=libc++ -lc++fs. Also that building <filesystem> code with Clang 7 and libstdc++ 8 works. i.e clang++-7 -std=c++17 fs.cpp -lstdc++fs.

jonatack · 2021-09-22T08:29:13Z

Concept ACK.

maflcko

Is there a reason in ci to check libstdc++-8 twice, when one could use libc++-7?

ci/test/00_setup_env_native_qt5.sh

ci/test/00_setup_env_native_nowallet.sh

maflcko · 2021-09-22T10:39:18Z

review ACK 182de7b

Also tested on debian:stretch with the following diff:

diff --git a/ci/test/00_setup_env_native_nowallet.sh b/ci/test/00_setup_env_native_nowallet.sh
index e9a20fca7..6d0b36295 100755
--- a/ci/test/00_setup_env_native_nowallet.sh
+++ b/ci/test/00_setup_env_native_nowallet.sh
@@ -9,6 +9,6 @@ export LC_ALL=C.UTF-8
 export CONTAINER_NAME=ci_native_nowallet
 export DOCKER_NAME_TAG=ubuntu:18.04  # Use bionic to have one config run the tests in python3.6, see doc/dependencies.md
 export PACKAGES="python3-zmq clang-7 llvm-7 libc++abi-7-dev libc++-7-dev"  # Use clang-7 to test C++17 compatibility, see doc/dependencies.md
-export DEP_OPTS="NO_WALLET=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'"
+export DEP_OPTS="NO_QT=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'"
 export GOAL="install"
 export BITCOIN_CONFIG="--enable-reduce-exports CC=clang-7 CXX='clang++-7 -stdlib=libc++'"

maflcko · 2021-09-22T10:50:00Z

Thinking about bumping to clang-8 as well, because debian:stretch will enter commercial ELTS next year. So anyone paying for that can also pay for someone to compile Bitcoin Core. It is not even possible to run the functional tests on stretch, due to python3.5.

DrahtBot · 2021-09-22T17:35:57Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20744 (Use std::filesystem. Remove Boost Filesystem & System by fanquake)

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.

fanquake · 2021-09-23T08:49:58Z

Thinking about bumping to clang-8 as well,

If we did do that, we should increase the minimum required libc++ version to 8 as well, and revert #22342. I can't imagine anyone would be building with Clang 8 against libc++ 7.

laanwj · 2021-09-23T09:30:03Z

Concept ACK.

laanwj · 2021-09-23T12:12:03Z

Would this unblock #20457 and #20452?

fanquake · 2021-09-23T12:50:53Z

Would this unblock #20457 and #20452?

Yes. This PR as is, should be enough to unblock those, as support for the *_chars funcs (Elementary string conversions) landed in libstdc++ 8.

hebasto · 2021-09-25T16:47:39Z

Concept ACK.

hebasto · 2021-09-25T17:52:32Z

ci/test/00_setup_env_native_nowallet.sh

-export PACKAGES="python3-zmq clang-5.0 llvm-5.0"  # Use clang-5 to test C++17 compatibility, see doc/dependencies.md
-export DEP_OPTS="NO_WALLET=1"
+export PACKAGES="python3-zmq clang-7 llvm-7 libc++abi-7-dev libc++-7-dev"  # Use clang-7 to test C++17 compatibility, see doc/dependencies.md
+export DEP_OPTS="NO_WALLET=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'"


182de7b

Passing CCand CXX to DEP_OPTS does not work as expected. From the log:

... Configuring qt... ... Configure summary: Building on: linux-g++ (x86_64, CPU features: mmx sse sse2) Building for: linux-g++-64 (x86_64, CPU features: mmx sse sse2) Target compiler: gcc 7.5.0 ...

Reported in #22184.

Another issue:

... checking for QMinimalIntegrationPlugin (-lqminimal)... no configure: WARNING: QMinimalIntegrationPlugin not found.; bitcoin-qt frontend will not be built ... Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = no ...

Reported in #22344, fixed in #22814, some relevant discussion in #22815 (comment).

Passing CC and CXX to DEP_OPTS does not work as expected.

It works as expected with everything except for Qt, and that's not an issue introduced by this PR.

Reported in #22344, fixed in #22814, some relevant discussion in

Another Qt specific issue, that isn't introduced by this PR.

Frankly I'm getting sick of fighting against Qts build system, and don't consider either of this issues blockers for this change, especially if there's a potential fix available. I'll comment in #22814 shortly.

... isn't introduced by this PR.

This PR builds dependencies with the -stdlib=libc++. It was not the case before.

This PR builds dependencies with the -stdlib=libc++. It was not the case before.

The underlying issue, which is that the Qt build doesn't work properly in regards to building with -stdlib=libc++, was not introduced in this PR. I don't think it's an issue that we have one less CI building Qt in depends.

…++ requ…

It is outdated since bitcoin#23060.

94a7f09 doc: Drop outdated note (Hennadii Stepanov) Pull request description: It is outdated since #23060. ACKs for top commit: MarcoFalke: cr ACK 94a7f09 theStack: Code-review ACK 94a7f09 Tree-SHA512: 5a98cf39c3939845d1abb64cb62dbb9bdb1135aa07a5cd0d85de7b6a6d9a3397b0e5da06cdb89991aac31151cb3c0d38eacf4898c53014cb7f7600e992230f87

…s with -stdlib=libc++ 33796a9 build: Add ability to build qt in depends with -stdlib=libc++ (Hennadii Stepanov) Pull request description: This PR makes possible to build the `qt` package in depends against `libc++` for x86_64 platform. Fixes #22344. Required for #22815. Also this PR [fixes](bitcoin/bitcoin#23060 (comment)) the `[no wallet] [bionic]` task on CI: - on master (a8bbd4c), https://api.cirrus-ci.com/v1/task/5558609250615296/logs/ci.log: ``` Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = no ``` - this PR, https://api.cirrus-ci.com/v1/task/5502605561430016/logs/ci.log: ``` Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = yes ``` ACKs for top commit: fanquake: ACK 33796a9 - While this sort of string matching is fragile, I think the risk of this causing any actual issues is low. Tree-SHA512: 586dde2e9864cec7a49aeb4f2b77fb8c4ae96bd10b51f9c6de0cfe8512ad61db15bb7f8d1b0eb6a5a66fd2deee52ac52218f01eb6be107ac12f1a956190de54b

…stdlib=libc++ 33796a9 build: Add ability to build qt in depends with -stdlib=libc++ (Hennadii Stepanov) Pull request description: This PR makes possible to build the `qt` package in depends against `libc++` for x86_64 platform. Fixes bitcoin#22344. Required for bitcoin#22815. Also this PR [fixes](bitcoin#23060 (comment)) the `[no wallet] [bionic]` task on CI: - on master (a8bbd4c), https://api.cirrus-ci.com/v1/task/5558609250615296/logs/ci.log: ``` Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = no ``` - this PR, https://api.cirrus-ci.com/v1/task/5502605561430016/logs/ci.log: ``` Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = yes ``` ACKs for top commit: fanquake: ACK 33796a9 - While this sort of string matching is fragile, I think the risk of this causing any actual issues is low. Tree-SHA512: 586dde2e9864cec7a49aeb4f2b77fb8c4ae96bd10b51f9c6de0cfe8512ad61db15bb7f8d1b0eb6a5a66fd2deee52ac52218f01eb6be107ac12f1a956190de54b

It is outdated since bitcoin#23060.

…stdlib=libc++ 33796a9 build: Add ability to build qt in depends with -stdlib=libc++ (Hennadii Stepanov) Pull request description: This PR makes possible to build the `qt` package in depends against `libc++` for x86_64 platform. Fixes bitcoin#22344. Required for bitcoin#22815. Also this PR [fixes](bitcoin#23060 (comment)) the `[no wallet] [bionic]` task on CI: - on master (a8bbd4c), https://api.cirrus-ci.com/v1/task/5558609250615296/logs/ci.log: ``` Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = no ``` - this PR, https://api.cirrus-ci.com/v1/task/5502605561430016/logs/ci.log: ``` Options used to compile and link: external signer = yes multiprocess = no with libs = yes with wallet = no with gui / qt = yes ``` ACKs for top commit: fanquake: ACK 33796a9 - While this sort of string matching is fragile, I think the risk of this causing any actual issues is low. Tree-SHA512: 586dde2e9864cec7a49aeb4f2b77fb8c4ae96bd10b51f9c6de0cfe8512ad61db15bb7f8d1b0eb6a5a66fd2deee52ac52218f01eb6be107ac12f1a956190de54b

It is outdated since bitcoin/bitcoin#23060.

fanquake added Docs Build system labels Sep 22, 2021

maflcko approved these changes Sep 22, 2021

View reviewed changes

ci/test/00_setup_env_native_qt5.sh Show resolved Hide resolved

ci/test/00_setup_env_native_nowallet.sh Outdated Show resolved Hide resolved

doc: update minimum compiler requirements for std::filesystem

04f5baf

fanquake force-pushed the 23_0_min_compiler_libcpp_requires branch from cf1c9fb to 50a0eb7 Compare September 22, 2021 09:53

ci: update minimum compiler requirements for std::filesystem

182de7b

fanquake force-pushed the 23_0_min_compiler_libcpp_requires branch from 50a0eb7 to 182de7b Compare September 22, 2021 10:18

This was referenced Sep 23, 2021

Use std::filesystem. Remove Boost Filesystem & System #20744

Merged

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

Multiprocess bitcoin #10102

Draft

hebasto reviewed Sep 25, 2021

View reviewed changes

fanquake merged commit 67eae69 into bitcoin:master Sep 28, 2021

fanquake deleted the 23_0_min_compiler_libcpp_requires branch September 28, 2021 06:04

hebasto mentioned this pull request Sep 28, 2021

build: Add ability to build qt in depends with -stdlib=libc++ #22814

Merged

vasild mentioned this pull request Sep 28, 2021

util: use stronger-guarantee rename method #20435

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 28, 2021

Merge bitcoin#23060: release: increase minimum compiler and lib(std)c…

484ed0d

…++ requ…

fanquake mentioned this pull request Oct 2, 2021

In file included from util/strencodings.cpp:6:0: fatal error: charconv: No such file. #23158

Closed

fanquake mentioned this pull request Oct 17, 2021

fatal error: charconv: No such file or directory #include <charconv> #23291

Closed

Gapcoin1 mentioned this pull request Nov 3, 2021

Fatal Compiling Error namecoin/namecoin-core#475

Closed

hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 25, 2021

doc: Drop outdated note

94a7f09

It is outdated since bitcoin#23060.

hebasto mentioned this pull request Dec 25, 2021

doc: Drop outdated note #23864

Merged

mzumsande pushed a commit to mzumsande/bitcoin that referenced this pull request Jan 17, 2022

doc: Drop outdated note

84a7b26

It is outdated since bitcoin#23060.

rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Feb 3, 2022

doc: Drop outdated note

9cbe27a

It is outdated since bitcoin#23060.

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Jul 10, 2022

doc: Drop outdated note

aa1e3a1

It is outdated since bitcoin/bitcoin#23060.

Pythonix mentioned this pull request Aug 9, 2022

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) gridcoin-community/Gridcoin-Research#2564

Merged

barton2526 mentioned this pull request Oct 5, 2022

build: Require gcc8 on Ubuntu Bionic to enable C++17 features gridcoin-community/Gridcoin-Research#2579

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

release: increase minimum compiler and lib(std)c++ requirements #23060

release: increase minimum compiler and lib(std)c++ requirements #23060

Uh oh!

fanquake commented Sep 22, 2021 •

edited

Loading

Uh oh!

jonatack commented Sep 22, 2021

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Sep 22, 2021

Uh oh!

maflcko commented Sep 22, 2021

Uh oh!

DrahtBot commented Sep 22, 2021 •

edited

Loading

Uh oh!

fanquake commented Sep 23, 2021

Uh oh!

laanwj commented Sep 23, 2021

Uh oh!

laanwj commented Sep 23, 2021

Uh oh!

fanquake commented Sep 23, 2021

Uh oh!

hebasto commented Sep 25, 2021

Uh oh!

hebasto Sep 25, 2021

Uh oh!

fanquake Sep 28, 2021

Uh oh!

hebasto Sep 28, 2021 •

edited

Loading

Uh oh!

fanquake Sep 28, 2021

Uh oh!

Uh oh!

release: increase minimum compiler and lib(std)c++ requirements #23060

release: increase minimum compiler and lib(std)c++ requirements #23060

Uh oh!

Conversation

fanquake commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Sep 22, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Sep 22, 2021

Uh oh!

maflcko commented Sep 22, 2021

Uh oh!

DrahtBot commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented Sep 23, 2021

Uh oh!

laanwj commented Sep 23, 2021

Uh oh!

laanwj commented Sep 23, 2021

Uh oh!

fanquake commented Sep 23, 2021

Uh oh!

hebasto commented Sep 25, 2021

Uh oh!

hebasto Sep 25, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Sep 22, 2021 •

edited

Loading

DrahtBot commented Sep 22, 2021 •

edited

Loading

hebasto Sep 28, 2021 •

edited

Loading