Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 3, 2024

@hebasto hebasto added this to the 28.0 milestone Aug 3, 2024
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 3, 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 m3dwards, maflcko

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

@fanquake
Copy link
Member

fanquake commented Aug 3, 2024

~0. I think it's easier to document nothing here. Listing 2 distros out of the many others where this works/is supported seems arbitrary, and the change is also not really correct correct in any case, i.e building on earlier versions (23.10) works fine (regardless of if "support" has ended). The compiler is the requirement, not the distro.

@@ -13,6 +13,8 @@ Other options which may work, but which have not been extensively tested are (pl

* On Windows, using a POSIX compatibility layer application such as [cygwin](https://www.cygwin.com/) or [msys2](https://www.msys2.org/).

Currently only building on Ubuntu 24.04 or later and Debian 12 or later is supported.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just "The instructions below work on Ubuntu and Debian. Make sure the distribution's g++-mingw-w64-x86-64-posix package meets the minimum required G++ version specified in [doc/dep.md]"?

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! Reworked using your suggestion.

This stems from a requirement for the g++ minimum supported version
being >= 11.
@hebasto hebasto force-pushed the 240803-mingw-docs branch from f237d9b to ed83974 Compare August 3, 2024 11:55
@hebasto hebasto changed the title doc: Specify minimum distro versions for MinGW builds doc: Add note about distro's g++-mingw-w64-x86-64-posix version Aug 3, 2024
@fanquake fanquake removed this from the 28.0 milestone Aug 5, 2024
@m3dwards
Copy link
Contributor

m3dwards commented Aug 5, 2024

I was testing a different PR and wanted to compile on WSL so I ran the default wsl --install which by default install Ubuntu 22.04 (confusingly just called ubuntu - no version) which has ming32 gcc 10 and therefore is no good.

The original version of the PR with explicit distribution recommendations would have been extremely easy to guide me in the right direction but I can see that the amended version suggesting to compare mingw against dependencies.md would be less maintenance burden.

ACK ed83974

@maflcko
Copy link
Member

maflcko commented Aug 7, 2024

review-only ACK ed83974

@fanquake fanquake merged commit 4c879c4 into bitcoin:master Aug 12, 2024
16 checks passed
@hebasto hebasto deleted the 240803-mingw-docs branch August 12, 2024 10:41
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2024
…64-posix` version

ed83974 doc: Add note about distro's g++-mingw-w64-x86-64-posix version (Hennadii Stepanov)

Pull request description:

  This PR stems from a requirement for the `g++` minimum supported version [being >= 11](bitcoin#29091):
  - https://packages.ubuntu.com/noble/g++-mingw-w64-x86-64-posix
  - https://packages.debian.org/bookworm/g++-mingw-w64-x86-64-posix

ACKs for top commit:
  m3dwards:
    ACK ed83974
  maflcko:
    review-only ACK ed83974

Tree-SHA512: a4c4d5239df5540b2dac922c2834c51337ec67310d596620bea02fe54334917e83da8954107c6a3fdd08143d1eac6a2cd4b0520ad023d7ccad8bcbdc4e03a7c0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 27, 2024
…64-posix` version

ed83974 doc: Add note about distro's g++-mingw-w64-x86-64-posix version (Hennadii Stepanov)

Pull request description:

  This PR stems from a requirement for the `g++` minimum supported version [being >= 11](bitcoin#29091):
  - https://packages.ubuntu.com/noble/g++-mingw-w64-x86-64-posix
  - https://packages.debian.org/bookworm/g++-mingw-w64-x86-64-posix

ACKs for top commit:
  m3dwards:
    ACK ed83974
  maflcko:
    review-only ACK ed83974

Tree-SHA512: a4c4d5239df5540b2dac922c2834c51337ec67310d596620bea02fe54334917e83da8954107c6a3fdd08143d1eac6a2cd4b0520ad023d7ccad8bcbdc4e03a7c0
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 27, 2024
b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script)
745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow)
01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow)
432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script)
1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script)
8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script)
f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script)
ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script)
e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script)
df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script)
57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script)
e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script)
62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script)
745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow)
4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov)
69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script)
ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script)
9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow)
479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script)
ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script)
63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script)
3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script)
3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script)

Pull request description:

  ## Issue being fixed or feature implemented
  Trivial backports

  ## What was done?

  ## How Has This Been Tested?
  built locally

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK b654479
  kwvg:
    utACK b654479

Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043
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
@bitcoin bitcoin locked and limited conversation to collaborators Aug 12, 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.

5 participants