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: update containers and CI to use Ubuntu 22.04 LTS (jammy) #6379

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Nov 2, 2024

Motivation

While some aspects of C++20 are supported by GCC 9.3 (the version shipped with focal, source), P0896R4 ("The One Ranges Proposal") is not (source), required for moving away from dash#4622 to adopt std::ranges and for backporting PRs like bitcoin#28687.

They're supported from GCC 10.1 onwards (source) and the next available LTS, jammy, currently ships with GCC 11.2 (source). As we're now using Guix to build our releases, there shouldn't be any adverse effects from our containers having a higher version of glibc.

Additionally, upstream uses 24.04 (noble) for their build images (source) and jammy to determine the minimum version to CMake (source) (for reference, focal ships with CMake 3.16, source).

Additional Information

  • Builds will no longer run check-symbols (and the option to do so, RUN_SYMBOL_TESTS has been removed) as portable builds (builds expected for distributions) are built with Guix, which pins its version of glibc (source, currently 2.28), runs check-symbols (source), which in turn make sure that it doesn't contain symbols expected in versions higher than glibc 2.31 (source).

    Upstream does not use check-symbols during their builds nor do we have a reason to (as we no longer using Gitian).

  • CI build failures w.r.t. pthread_yield (build, build) should be resolved by rebuilding bdb4 (see bitcoin#26423). This problem has only been observed on GitHub CI and locally (the latter resolved by rebuilding depends, I use a separate target triple for testing, x86_64-jammy-linux-gnu), GitLab CI has not observed such failures (build).

Breaking changes

None expected.

Checklist

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

@kwvg kwvg added this to the 22 milestone Nov 2, 2024
kwvg added 2 commits November 2, 2024 21:34
Guix runs `check-symbols` during its builds and retains value there as
those builds are expected to run on platforms/distros that have that
pinned version of glibc or higher (pinned in `manifest.scm` as 2.28 and
checked to make sure ABIs present in versions higher than 2.31 aren't
used by `symbol-check.py`).

There's no reason to do this on regular builds, which aren't expected
to be as portable, built in an environment that's much less customizable.
@kwvg kwvg marked this pull request as ready for review November 3, 2024 07:19
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.

CI build failures w.r.t. pthread_yield ...

Probably some cache issues. Completes with no failures in my repo https://github.com/UdjinM6/dash/actions/runs/11653234872

utACK 1592a0f

@@ -63,7 +63,3 @@ fi
if [ "$RUN_SECURITY_TESTS" = "true" ]; then
make test-security-check
fi

if [ "$RUN_SYMBOL_TESTS" = "true" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have guix builds for each PR? If we do that, we will find a potential issue during guix build in pre-release time, which can make mess and release delay.

I checked logs of some guix guild here https://github.com/dashpay/dash/actions/runs/11645989967/job/32429737929?pr=6379#step:20:1 and I don't see check-symbols running. Does it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unwanted symbols have a chance to sneak in only if anything used during building is built outside Guix's environment but Guix builds its own depends and the Guix environment controls the glibc version it uses for building, so I don't see how that'll be an issue.

It does run the tests under the header "Running symbol and dynamic library checks..." (source), which corresponds to check-symbols (source).

@kwvg kwvg requested a review from knst November 4, 2024 13:45
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK 1592a0f

let's add release notes, that minimum version that we tested Dash Core is bumped from 20.04 to 22.04

@kwvg
Copy link
Collaborator Author

kwvg commented Nov 4, 2024

let's add release notes, that minimum version that we tested Dash Core is bumped from 20.04 to 22.04

@knst, what determines if Guix-built binaries will work is the glibc version, 20.04 ships with glibc 2.31 (source) while Guix is (as of this PR), targeting glibc 2.28 (source). There wouldn't be a reason it wouldn't work on 20.04.

Also, in #6382, the glibc version will be bumped to 2.31 (leaving behind 18.04 LTS bionic) and that change does have release notes (source), which I think should be sufficient in indicating where compatibility stops.

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 1592a0f

@PastaPastaPasta PastaPastaPasta merged commit 444d7a9 into dashpay:develop Nov 4, 2024
38 of 44 checks passed
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