Skip to content
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

ci: drop i686 builds, temporarily enable glibc backwards compatibility for most CI builds #5372

Merged
merged 10 commits into from
May 31, 2023

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented May 13, 2023

Description

This pull request aims to set the stage for the first set of Guix backports. Backporting Guix while keeping our CI system happy is non-trivial as we also support Gitian and therefore, cannot utilize the upstream approach for compatibility (i.e. bitcoin#22365) and must retain glibc backwards compatibility logic within the codebase itself.

This is only a temporary measure as it is planned in second part of Guix backports to render the backwards compatibility requirement obsolete by setting the minimum required glibc version to 2.27, which is done using bitcoin#27029.

This addresses why glibc backwards compatibility is being enabled, to ensure that CI and tests succeed between the time period we mandate symbol checking for all Linux builds, instead of just backwards compatible ones (which is introduced through portions of bitcoin#22405, which are later relevant in bitcoin#22392) and the time we update the minimum glibc version.

i686 Linux builds are being dropped to make way for bitcoin#26075. It also seems to be a project-wide decision upstream (as evident from the removal of 32-bit Guix build parameters)

Update

bitcoin#22305 was preferred as the PR to be backported instead of bitcoin#22287 as part of the fixes suggested in the former are already in the codebase (see 90c7f16) and due to concerns raised with the latter's approach in the former's comments (see comments)

Additional Information

@kwvg kwvg changed the title ci: drop i686 builds, temporarily enable glibc backwards compatibility for CI builds ci: drop i686 builds, temporarily enable glibc backwards compatibility for all CI builds May 13, 2023
@kwvg kwvg force-pushed the ci_prep branch 6 times, most recently from 075e8f5 to 6369614 Compare May 19, 2023 16:32
@kwvg kwvg changed the title ci: drop i686 builds, temporarily enable glibc backwards compatibility for all CI builds ci: drop i686 builds, temporarily enable glibc backwards compatibility for most CI builds May 19, 2023
@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg marked this pull request as ready for review May 29, 2023 15:38
@kwvg kwvg requested review from knst, UdjinM6 and PastaPastaPasta May 29, 2023 15:50
@kwvg kwvg force-pushed the ci_prep branch 2 times, most recently from 954f36e to 8f0a7f5 Compare May 29, 2023 16:15
@kwvg kwvg requested a review from UdjinM6 May 31, 2023 05:35
@kwvg kwvg requested a review from knst May 31, 2023 08:01
@UdjinM6 UdjinM6 added this to the 20 milestone May 31, 2023
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

kwvg added 2 commits May 31, 2023 11:06
TSan builds export all sorts of symbols that aren't part of the
allowlist and report sanitizer failures when enabled with glibc
compatibility enabled.

The result of `check-symbols` is valuable but Bitcoin does not
run them during their CI runs and for certain developer-oriented
builds, it's proving to be more of an obstruction. It will still
be enabled for all other builds and will prove to be important
in backporting Guix pull requests, a finger on the pulse, but that
doesn't apply to dev-oriented builds.
@PastaPastaPasta PastaPastaPasta merged commit e541e5d into dashpay:develop May 31, 2023
PastaPastaPasta added a commit 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants