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

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:23_0_min_compiler_libcpp_requires changing 3 files +8 −8
  1. fanquake commented at 8:19 am on September 22, 2021: member

    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.

  2. fanquake added the label Docs on Sep 22, 2021
  3. fanquake added the label Build system on Sep 22, 2021
  4. in ci/test/00_setup_env_native_qt5.sh:10 in cf1c9fbedd outdated 
     6@@ -7,13 +7,13 @@
 7 export LC_ALL=C.UTF-8
 8 
 9 export CONTAINER_NAME=ci_native_qt5
10-export DOCKER_NAME_TAG=ubuntu:18.04  # Check that bionic gcc-7 can compile our c++17 and run our functional tests in python3, see doc/dependencies.md
11-export PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libdbus-1-dev libharfbuzz-dev"
12+export DOCKER_NAME_TAG=ubuntu:18.04  # Check that bionic gcc-8 can compile our C++17 and run our functional tests in python3, see doc/dependencies.md
    MarcoFalke commented at 8:27 am on September 22, 2021: 
    0export DOCKER_NAME_TAG=ubuntu:18.04  # Check that bionic gcc-8 (with libstdc++-8) can compile our C++17 and run our functional tests in python3, see doc/dependencies.md
    fanquake commented at 8:35 am on September 22, 2021:
    If you’re compiling using GCC, using libstdc++ of the same version should be implied. I don’t even think you can use libc++ with GCC?
    MarcoFalke commented at 8:39 am on September 22, 2021:
    Fair enough
  5. jonatack commented at 8:29 am on September 22, 2021: member
    Concept ACK.
  6. in ci/test/00_setup_env_native_nowallet.sh:11 in cf1c9fbedd outdated 
     7@@ -8,7 +8,7 @@ export LC_ALL=C.UTF-8
 8 
 9 export CONTAINER_NAME=ci_native_nowallet
10 export DOCKER_NAME_TAG=ubuntu:18.04  # Use bionic to have one config run the tests in python3.6, see doc/dependencies.md
11-export PACKAGES="python3-zmq clang-5.0 llvm-5.0"  # Use clang-5 to test C++17 compatibility, see doc/dependencies.md
12+export PACKAGES="python3-zmq clang-7 llvm-7 libstdc++-8-dev"  # Use clang-7 to test C++17 compatibility, see doc/dependencies.md
    MarcoFalke commented at 8:29 am on September 22, 2021: 
    0export PACKAGES="python3-zmq clang-7 llvm-7 libc++abi-7-dev libc++-7-dev"  # Use clang-7 (with libc++-7) to test C++17 compatibility, see doc/dependencies.md
    fanquake commented at 8:56 am on September 22, 2021:

    Is there a reason in ci to check libstdc++-8 twice

    It’s not quite twice, given one build is Clang and one GCC. I had wanted this so that the Clang & libstdc++ case was tested, given that is going to be the most likely case when developers / users are compiling using (older) Clang. However happy to change this to use libc++ instead.

    MarcoFalke commented at 9:18 am on September 22, 2021:

    Currently no test coverage exists for libc++, so CI can’t catch “bugs” like #22342.

    Was there ever a time when clang&libstdc++ was broken, but gcc and clang&libc++ both worked?

    fanquake commented at 9:53 am on September 22, 2021:

    so CI can’t catch “bugs” like #22342.

    Yea for this case it’s worth using libc++.

    MarcoFalke commented at 10:06 am on September 22, 2021:
    The wallet code is disabled, so it can’t catch this specific instance, but in consensus code it should be able to catch.
    fanquake commented at 10:09 am on September 22, 2021:
    Sure, I’m just talking about introducing code that will break the libc++ 7 requirement.
  7. MarcoFalke approved
  8. MarcoFalke commented at 8:30 am on September 22, 2021: member
    Is there a reason in ci to check libstdc++-8 twice, when one could use libc++-7?
  9. doc: update minimum compiler requirements for std::filesystem 04f5bafb7b
  10. fanquake force-pushed on Sep 22, 2021
  11. ci: update minimum compiler requirements for std::filesystem 182de7ba10
  12. fanquake force-pushed on Sep 22, 2021
  13. MarcoFalke commented at 10:39 am on September 22, 2021: member

    review ACK 182de7ba10811ec39e24ec5bec7cd2119f776f2f

    Also tested on debian:stretch with the following diff:

     0diff --git a/ci/test/00_setup_env_native_nowallet.sh b/ci/test/00_setup_env_native_nowallet.sh
 1index e9a20fca7..6d0b36295 100755
 2--- a/ci/test/00_setup_env_native_nowallet.sh
 3+++ b/ci/test/00_setup_env_native_nowallet.sh
 4@@ -9,6 +9,6 @@ export LC_ALL=C.UTF-8
 5 export CONTAINER_NAME=ci_native_nowallet
 6 export DOCKER_NAME_TAG=ubuntu:18.04  # Use bionic to have one config run the tests in python3.6, see doc/dependencies.md
 7 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
 8-export DEP_OPTS="NO_WALLET=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'"
 9+export DEP_OPTS="NO_QT=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'"
10 export GOAL="install"
11 export BITCOIN_CONFIG="--enable-reduce-exports CC=clang-7 CXX='clang++-7 -stdlib=libc++'"
  14. MarcoFalke commented at 10:50 am on September 22, 2021: member
    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.
  15. DrahtBot commented at 5:35 pm on September 22, 2021: member

    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.

  16. fanquake commented at 8:49 am on September 23, 2021: member

    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.

  17. laanwj commented at 9:30 am on September 23, 2021: member
    Concept ACK.
  18. laanwj commented at 12:12 pm on September 23, 2021: member
    Would this unblock #20457 and #20452?
  19. fanquake commented at 12:50 pm on September 23, 2021: member

    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.

  20. hebasto commented at 4:47 pm on September 25, 2021: member
    Concept ACK.
  21. in ci/test/00_setup_env_native_nowallet.sh:12 in 182de7ba10 
     7@@ -8,7 +8,7 @@ export LC_ALL=C.UTF-8
 8 
 9 export CONTAINER_NAME=ci_native_nowallet
10 export DOCKER_NAME_TAG=ubuntu:18.04  # Use bionic to have one config run the tests in python3.6, see doc/dependencies.md
11-export PACKAGES="python3-zmq clang-5.0 llvm-5.0"  # Use clang-5 to test C++17 compatibility, see doc/dependencies.md
12-export DEP_OPTS="NO_WALLET=1"
13+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
14+export DEP_OPTS="NO_WALLET=1 CC=clang-7 CXX='clang++-7 -stdlib=libc++'"
    hebasto commented at 5:52 pm on September 25, 2021:

    182de7ba10811ec39e24ec5bec7cd2119f776f2f

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

    0...
1Configuring qt...
2...
3
4Configure summary:
5
6Building on: linux-g++ (x86_64, CPU features: mmx sse sse2)
7Building for: linux-g++-64 (x86_64, CPU features: mmx sse sse2)
8Target compiler: gcc 7.5.0
9...

    Reported in #22184.

    Another issue:

     0...
 1checking for QMinimalIntegrationPlugin (-lqminimal)... no
 2configure: WARNING: QMinimalIntegrationPlugin not found.; bitcoin-qt frontend will not be built
 3...
 4
 5Options used to compile and link:
 6  external signer = yes
 7  multiprocess    = no
 8  with libs       = yes
 9  with wallet     = no
10  with gui / qt   = no
11...

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

    fanquake commented at 5:30 am on September 28, 2021:

    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.

    hebasto commented at 9:10 am on September 28, 2021:

    … isn’t introduced by this PR.

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

    fanquake commented at 9:35 am on September 28, 2021:

    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.

  22. fanquake merged this on Sep 28, 2021
  23. fanquake closed this on Sep 28, 2021
  24. fanquake deleted the branch on Sep 28, 2021
  25. sidhujag referenced this in commit 484ed0d354 on Sep 28, 2021
  26. hebasto referenced this in commit 94a7f09de6 on Dec 25, 2021
  27. MarcoFalke referenced this in commit 6a363720ff on Dec 26, 2021
  28. fanquake referenced this in commit de28c348a0 on Dec 30, 2021
  29. sidhujag referenced this in commit 4975b13579 on Dec 30, 2021
  30. mzumsande referenced this in commit 84a7b26306 on Jan 17, 2022
  31. rebroad referenced this in commit 9cbe27af8e on Feb 3, 2022
  32. PastaPastaPasta referenced this in commit 86c0067754 on Apr 7, 2022
  33. PastaPastaPasta referenced this in commit 015034a31d on Apr 11, 2022
  34. gades referenced this in commit 5031959363 on May 12, 2022
  35. janus referenced this in commit aa1e3a1eab on Jul 10, 2022
  36. DrahtBot locked this on Oct 30, 2022


fanquake MarcoFalke jonatack DrahtBot laanwj hebasto

Labels
Docs Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me