Skip to content

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jun 5, 2024

Reverts part of fa67f09, now that we require a minimum of GCC 11.

See also:
#28349 (comment).

Reverts part of fa67f09, now that we
require a minimum of GCC 11.

See also:
bitcoin#28349 (comment).
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 5, 2024

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 maflcko, theuni

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

@maflcko
Copy link
Member

maflcko commented Jun 5, 2024

Is there any user facing difference? If so, it would be good to show it in a comment.

@fanquake
Copy link
Member Author

fanquake commented Jun 5, 2024

Is there any user facing difference? If so, it would be good to show it in a comment.

./configure CC=gcc-10 CXX=g++-10
<snip>
configure: error: *** A compiler with support for C++20 language features is required.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2024

Otherwise it would continue to compile (and miscompile RenameOver)?

@fanquake
Copy link
Member Author

fanquake commented Jun 5, 2024

Otherwise it would continue to compile (and miscompile RenameOver)?

I don't know where it would fail first, but I don't see the point of having it continue past configure in this case.
There are multiple devs using clang-14 who've been confused by clang-14 failing to compile code, and unsure what the issue was. So fixing the case here for GCC seems more useful than random compile errors.

@maflcko
Copy link
Member

maflcko commented Jun 5, 2024

I am asking, because this may need backport, if compilation succeeds. I assumed it would fail, which is why I left it as-is. However, if this compiles for users using g++10, then it seems dangerous. It would expose them to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98985 and possibly other bugs.

@fanquake
Copy link
Member Author

fanquake commented Jun 5, 2024

The first compile failure I see is in util/time.cpp:

util/time.cpp: In function ‘std::string FormatISO8601DateTime(int64_t)’:
util/time.cpp:50:24: error: ‘year_month_day’ in namespace ‘std::chrono’ does not name a type
   50 |     const std::chrono::year_month_day ymd{days};
      |                        ^~~~~~~~~~~~~~
util/time.cpp:51:24: error: ‘hh_mm_ss’ in namespace ‘std::chrono’ does not name a type
   51 |     const std::chrono::hh_mm_ss hms{secs - days};
      |                        ^~~~~~~~
util/time.cpp:52:63: error: ‘ymd’ was not declared in this scope
   52 |     return strprintf("%04i-%02u-%02uT%02i:%02i:%02iZ", signed{ymd.year()}, unsigned{ymd.month()}, unsigned{ymd.day()}, hms.hours().count(), hms.minutes().count(), hms.seconds().count());
      |                                                               ^~~
util/time.cpp:52:120: error: ‘hms’ was not declared in this scope
   52 |     return strprintf("%04i-%02u-%02uT%02i:%02i:%02iZ", signed{ymd.year()}, unsigned{ymd.month()}, unsigned{ymd.day()}, hms.hours().count(), hms.minutes().count(), hms.seconds().count());
      |                                                                                                                        ^~~
util/time.cpp: In function ‘std::string FormatISO8601Date(int64_t)’:
util/time.cpp:59:24: error: ‘year_month_day’ in namespace ‘std::chrono’ does not name a type
   59 |     const std::chrono::year_month_day ymd{days};
      |                        ^~~~~~~~~~~~~~
util/time.cpp:60:47: error: ‘ymd’ was not declared in this scope
   60 |     return strprintf("%04i-%02u-%02u", signed{ymd.year()}, unsigned{ymd.month()}, unsigned{ymd.day()});
      |                                               ^~~
make[2]: *** [Makefile:11756: util/libbitcoin_util_a-time.o] Error 1

@maflcko
Copy link
Member

maflcko commented Jun 5, 2024

Thanks for checking. Can you re-try on 27.x, because c353025 is not in 27.x.

@fanquake
Copy link
Member Author

fanquake commented Jun 5, 2024

Can you re-try on 27.x, because c353025 is not in 27.x.

27.x & fccd32e compiles with gcc-10 (Ubuntu 10.5.0-4ubuntu2) 10.5.0.

@maflcko maflcko added this to the 28.0 milestone Jun 5, 2024
@maflcko
Copy link
Member

maflcko commented Jun 5, 2024

Oh, 27.x remains on g++-10, so no backport needed. I was wrong. See https://github.com/bitcoin/bitcoin/blob/27.x/doc/dependencies.md

@maflcko
Copy link
Member

maflcko commented Jun 5, 2024

utACK 232928b

Seems fine to fail early here, instead of running into a util/time compile failure later on.

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

utACK 232928b

@fanquake fanquake merged commit 1040a1f into bitcoin:master Jun 6, 2024
@fanquake fanquake deleted the no_longer_allow_gcc_10 branch June 6, 2024 09:59
kwvg added a commit to kwvg/dash that referenced this pull request Feb 8, 2025
kwvg added a commit to kwvg/dash that referenced this pull request Feb 8, 2025
kwvg added a commit to kwvg/dash that referenced this pull request Feb 9, 2025
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Feb 12, 2025
…oble`), merge bitcoin#29985

1599cc6 fix: suppress `float-cast-overflow` UBSan error from `qRound(double)` (Kittywhiskers Van Gogh)
a57d128 ci: drop i386 conditional altogether, we don't need `wine32` anymore (Kittywhiskers Van Gogh)
b862411 ci: drop conflicting `g++-multilib` package and related workaround (Kittywhiskers Van Gogh)
7b1caa9 fix: remove now-invalid package entry (Kittywhiskers Van Gogh)
9f9e965 fix: use default non-root user `ubuntu` introduced in Ubuntu 22.10 (Kittywhiskers Van Gogh)
9b4c95e ci: update containers and CI to use Ubuntu 24.04 LTS (`noble`) (Kittywhiskers Van Gogh)
8aec25b merge bitcoin#29985: Fix build of Qt for 32-bit platforms with recent glibc (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  Since [dash#6389](#6389), the minimum supported version of GCC has been 11.1 or later. Our current base, Ubuntu 22.04 LTS (`jammy`) ships with GCC 11.2 ([source](https://packages.ubuntu.com/jammy/gcc)). The cross-compilation package for `arm-linux-gnueabihf` is GCC 11.2 as well ([source](https://packages.ubuntu.com/jammy/gcc-arm-linux-gnueabihf)). Unfortunately, this isn't the case for `mingw-w64-x86-64`, which ships with GCC 10.3 ([source](https://packages.ubuntu.com/jammy/gcc-mingw-w64-x86-64-posix)).

  So far, an workaround was utilized to allow GCC 10.3 to pass muster upstream through bitcoin#28379 ([source](bitcoin@fa67f09#diff-0baeabda402ee522682c25cef25a248ff6d9e2904f6d66f93f6babf55b576675L986)). Dash Core's enablement of C++20 experimental support in [dash#4600](#4600) was relatively even more permissive ([source](https://github.com/dashpay/dash/pull/4600/files#diff-0baeabda402ee522682c25cef25a248ff6d9e2904f6d66f93f6babf55b576675R168)).

  Though since then, enforcement of those newer minimum requirements through backports like [bitcoin#30228](bitcoin#30228) are frustrated by the version of GCC shipped for Windows cross-compilation.

  This can be resolved by bumping the image to the next available LTS release, Ubuntu 24.04 (`noble`), which ships with GCC 13.2 natively ([source](https://packages.ubuntu.com/noble/gcc)), for ARM Linux ([source](https://packages.ubuntu.com/noble/gcc-arm-linux-gnueabihf)) and AMD64 Windows ([source](https://packages.ubuntu.com/noble/gcc-mingw-w64-x86-64-posix)), moreover, it is acknowledged as a _de facto_ lowest supported distro needed for Windows cross compilation by [bitcoin#30580](bitcoin#30580 (comment)).

  Skipping the enforcement of newer minimum requirements is inadvisable, primarily because it will eventually conflict with upcoming code changes ([comment](bitcoin#30228 (comment))) like [dash#6378](#6378).

  ## Additional Information

  * Dependency for #6380

  * A default unprivileged user named `ubuntu` was introduced in Ubuntu 22.10 ([source](https://bugs.launchpad.net/cloud-images/+bug/2005129)) with UID `1000` and GID `1000`.

    We allow specifying UID and GID to ensure they match with the ownership of directories expected to be mounted to avoid permissions issues (especially on platforms like macOS where the user can have a UID of `501` and GID of `20`).
    * To retain this behavior, the `ubuntu` user and the `ubuntu` group will have its UID and GID updated. This is a no-op if defaults are selected.
    * In some cases where it may be undesirable to have the username or home directory deviate from the present name  (`dash`), it will be additionally renamed from `ubuntu`.
      * GitLab is unhappy if the container doesn't have a user named `dash` ([build](https://gitlab.com/dashpay/dash/-/jobs/9082688485#L24)) and it results in a runner failure.

  * Due to a conflict between `gcc-multilib` and `gcc-arm-linux-gnueabihf`, installation of the latter will result in the uninstallation of the former. This has been documented behavior for a while now ([source](https://bugs.launchpad.net/ubuntu/+source/gcc-defaults/+bug/1300211)) and continues till today (see below).

    <details>

    <summary>Error message:</summary>

    ```
    ubuntu@ec46b76eeb2c:/src/dash$ sudo apt install gcc-multilib gcc-arm-linux-gnueabihf
    Reading package lists... Done
    Building dependency tree... Done
    Reading state information... Done
    gcc-arm-linux-gnueabihf is already the newest version (4:13.2.0-7ubuntu1).
    gcc-arm-linux-gnueabihf set to manually installed.
    Some packages could not be installed. This may mean that you have
    requested an impossible situation or if you are using the unstable
    distribution that some required packages have not yet been created
    or been moved out of Incoming.
    The following information may help to resolve the situation:

    The following packages have unmet dependencies:
     gcc-arm-linux-gnueabihf : Depends: gcc-13-arm-linux-gnueabihf (>= 13.2.0-11~) but it is not installable
    E: Unable to correct problems, you have held broken packages.
    ```

    </details>

    Since [dash#5372](#5372), we have stopped supporting i686 and dropped support for 32-bit Windows even earlier. There doesn't seem to be much reason to keep `gcc-multilib` around, especially since it _cannot_ be around and has been silently uninstalled for a while now, so the package has now been dropped.

    * Since Wine 7.0, WoW64 support has been included ([source](https://www.winehq.org/announce/7.0)), which allows for some applications to run without a corresponding `wine32` setup ([source](https://wiki.debian.org/Wine#Step_1:_Enable_multiarch)). Support has improved furthermore in Wine 9.0 ([source](https://gitlab.winehq.org/wine/wine/-/releases/wine-9.0)), which is available in `noble` ([source](https://packages.ubuntu.com/noble/wine64)). This allows us to drop the i386 conditional altogether.

  * `libncurses5`, while a valid package on `jammy` ([source](https://packages.ubuntu.com/jammy/libncurses5)), is not in any future version, including `noble` ([source](https://packages.ubuntu.com/noble/libncurses5), error page) though `libncurses5-dev` continues to remain a valid package ([source](https://packages.ubuntu.com/noble/libncurses5-dev)) and remains used as a Python dependency ([source](https://github.com/dashpay/dash/blob/1930572b05c53d2d8335bcfc22270871d5b0bf2f/contrib/containers/ci/Dockerfile#L91)). As a result, `libncurses5` has been dropped.

  * A bump in distro base also causes a bump in glibc version, which causes zlib, a Qt dependency to fail a compile-time check ([build](https://github.com/dashpay/dash/actions/runs/13220255380/job/36904408129?pr=6564#step:6:8050)), this is avoided by backporting [bitcoin#29985](bitcoin#29985).

  * Due to [QTBUG-133261](https://bugreports.qt.io/browse/QTBUG-133261), UBSan will raise a `float-cast-overflow` originating from `qRound(double)` ([build](https://gitlab.com/dashpay/dash/-/jobs/9083558880#L3132)), as of this writing, a fix is currently underway ([source](https://codereview.qt-project.org/c/qt/qtbase/+/621931)) but it is unclear when it would be available in the next Qt 5.15 revision.

    As this doesn't seem to be a regression ([source](https://bugreports.qt.io/browse/QTBUG-133261?focusedId=860311&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-860311)) despite being undesirable behavior, we can suppress the alarm.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 1599cc6
  UdjinM6:
    utACK 1599cc6

Tree-SHA512: 92f786e9da1440830057729a5d9cdf6448ef9da8c66e0d4c3b80da2e22dd599353f44a31dcbe0ed103febdf627d2a1a332d72de1c3ec312b6ceb665f4bbcb5d1
kwvg added a commit to kwvg/dash that referenced this pull request Feb 12, 2025
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Feb 15, 2025
, bitcoin#28065, bitcoin#30228, partial bitcoin#28349, bitcoin#28579 (drop c++17 support)

c4b3dac refactor: make namespace `ranges` mostly an alias of `std::ranges` (Kittywhiskers Van Gogh)
a0e133e refactor: avoid convoluted casting by not using `std::time_t` (Kittywhiskers Van Gogh)
690a719 refactor: use `constexpr` `std::`{`copy`, `fill`} in `V2ShortIDs` (Kittywhiskers Van Gogh)
1269ac2 refactor: remove `rfind` workaround with `starts_with` (Kittywhiskers Van Gogh)
ceedabb merge bitcoin#30228: no-longer allow GCC-10 in C++20 check (Kittywhiskers Van Gogh)
213d548 partial bitcoin#28579: Remove redundant checks in compat/assumptions.h (Kittywhiskers Van Gogh)
ee7a6b4 partial bitcoin#28349: Require C++20 compiler (Kittywhiskers Van Gogh)
0c6a0bd fix: suppress deprecated-volatile in fuzz build (pasta)
e3638a3 ci: drop c++20-only build variant from GitHub and GitLab (Kittywhiskers Van Gogh)
c2c51f3 merge bitcoin#28065: Flatten all FUZZ_TARGET macros into one (Kittywhiskers Van Gogh)
28e93f5 merge bitcoin#27766: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time setting (Kittywhiskers Van Gogh)
84d6a7c merge bitcoin#27672: Print error message when FUZZ is missing (Kittywhiskers Van Gogh)
1f4c0b5 merge bitcoin#24460: update ax_cxx_compile_stdcxx to serial 14 (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6377

  * Depends on #6564

  * Using an existing namespace as an alias for a namespace part of the `std` namespace (as done in `ranges`) has been done before, see namespace `fs`, an alias to `std::filesystem` ([here](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/src/fs.h#L18-L21)).

  * While the comment in `wallet.cpp` did mention that there would be a better way to do the cast than in C++17, the method to do so, using `std::chrono::clock_cast`, has still not been implemented in Clang ([source](https://stackoverflow.com/a/77540287)). The issue has been sidestepped by not using `std::time_t` at all and instead relying on `fs::file_time_type`, the type returned by `fs::last_write_time`.

  ## Breaking changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    light utACK c4b3dac
  UdjinM6:
    light ACK c4b3dac
  PastaPastaPasta:
    light ACK c4b3dac

Tree-SHA512: fe058815c1e3d424f050a5bddd5b335ba069c33f9e33c56197864be64106b38a47f9c3e7703e12c775170f0e7d458c141bff5f31c5e7c40557a0989a5469ad09
@bitcoin bitcoin locked and limited conversation to collaborators Jun 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants