ci: merge bitcoin#30451, add macOS builds to GitHub Actions, refactor cache keys #6421
Conversation
|
GitHub Actions run for 66b7197, https://github.com/kwvg/dash/actions/runs/11955577182. |
kwvg
force-pushed
the
add_mac_win
branch
3 times, most recently
from
20d87fb to
fe08442
Compare
|
GitHub Actions run for fe08442, https://github.com/kwvg/dash/actions/runs/11973573180 |
kwvg
changed the title
kwvg
changed the title
|
GitLab Actions run for a5959da, https://github.com/kwvg/dash/actions/runs/11976668589. |
|
This pull request has conflicts, please rebase. |
04ce1fe ci: deduplicate macOS SDK setup logic (Kittywhiskers Van Gogh) 8dd0db7 ci: fix "LC_ALL: cannot change locale (en_US.UTF-8)" in Guix container (Kittywhiskers Van Gogh) 187fe17 ci: don't stage packages in `/tmp`, reduce layers for `cppcheck` build (Kittywhiskers Van Gogh) eef8635 ci: install `i386` packages only if host is `amd64`, merge layers (Kittywhiskers Van Gogh) e770229 ci: purge package manager cache after each interaction (Kittywhiskers Van Gogh) b7099ee ci: remove redundant `version` attribute, avoid `lldb` personality error (Kittywhiskers Van Gogh) 64cdc42 ci: add LLVM library path to `LD_LIBRARY_PATH` and GDB allowlist (Kittywhiskers Van Gogh) 440fd3f ci: drop distro LLVM packages, move Clang install up, set defaults (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * This pull request pulls container-specific changes from [dash#6387](#6387), [dash#6400](#6400) and [dash#6421](#6421) * The `HOST` check before running `setup_sdk.sh` isn't a part of the script itself as the script is written to be independent of external variables set. The caller is expected to know the conditions needed to run `setup_sdk.sh` as the script is _relatively_ agnostic to its environment. * The `version` attribute in the [`develop`](https://github.com/dashpay/dash/blob/a8e2316d6f9c6726a498bcae2c5c5d7354769511/contrib/containers/develop/docker-compose.yml) and [`guix`](https://github.com/dashpay/dash/blob/a8e2316d6f9c6726a498bcae2c5c5d7354769511/contrib/containers/guix/docker-compose.yml) container's `docker-compose.yml` has been dropped as the attribute has been deprecated in the compose spec ([source](https://github.com/compose-spec/compose-spec/blob/65ef9f4a5d713b405a77c45c64263f2543e65267/spec.md#version-top-level-element-obsolete)). * Using `LD_LIBRARY_PATH` to point to LLVM's libraries are acceptable and will not interfere with executing binaries built using the distro's packaged compiler as it will eventually search default paths and find the libraries shipped with the distro ([source](https://man7.org/linux/man-pages/man8/ld.so.8.html)). * Currently, running LLDB will result in a "personality set failed: Operation not permitted" error ([source](https://discourse.llvm.org/t/running-lldb-in-a-container/76801)). This is caused by its attempt at disabling ASLR for debugging. To work around this error, the container will now operate under relaxed restrictions (`seccomp=unconfined`). As disabling ASLR is valuable when debugging and the container is meant for developers (i.e. it isn't used for CI), we have opted to relax restrictions instead of skipping ASLR disablement. * As of `develop` (a8e2316), packages built by the container are stored in `/tmp`, which is inadvisable as it is the same directory used to store functional test runs and it's not too difficult to delete `/tmp`'s contents to save space in a long running [`develop`](https://github.com/dashpay/dash/blob/a8e2316d6f9c6726a498bcae2c5c5d7354769511/contrib/containers/develop/docker-compose.yml) container and then realize that both `shellcheck` and `cppcheck` are stored there and now you have to ditch the container you're working in and restart it. * To remedy this, packages are now built and stored in `/opt` in accordance with the FHS ([source](https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch03s13.html)). `/usr/local` was a contender but it's pre-populated, meanwhile `ls /opt` would give you a very quick picture of what's built for the container. * `/tmp` will not be entirely empty because [pypa/pip#10753](pypa/pip#10753) results in residual `.pem` files leaking into `/tmp` and `pyenv` stores its build log there and keeping it around has some debug value. ## 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 - [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: UdjinM6: ACK 04ce1fe PastaPastaPasta: utACK 04ce1fe Tree-SHA512: 5442ae06cb73b9bc4eec908803548195ae8fd9150422789e5f98578ad01a303b5361f9ba42fe8faee27ac91e38328b7771e4ba42b296dfa70ecbbfc7d10436b6
kwvg
changed the title
`runner.os` is irrelevant since we run everything inside Docker containers, we should instead be relying on the file that defines the base image.
The order of elements that make a cache key should be in the order such that trimming them out allows for a nearest-available search to reach an alternate cache
`restore-keys` is only meant to specify fallback keys if the primary key doesn't result in a cache hit. re-specifying the primary key is redundant also, remove keys that are unlikely to exist
We cannot add it to the sources cache because we share the cache among all variants and as the caches are immutable, the first one to finish will decide the cache for all subsequent use. This will prevent the SDKs from being saved and will cause failures in the next step as it fetches the common cache, most likely saved by a Linux variant and finds no cache. Saving it in the source cache isn't a great option either because it will get frequently invalidated. Better to just give it its own cache.
|
WalkthroughThe changes update multiple GitHub Actions workflows to incorporate conditional steps and enhanced caching mechanisms, particularly for macOS builds (x86_64-apple-darwin). New job steps are introduced to cache SDK sources and restore them using keys based on file hashes, with cache management updated for both SDKs and ccache. The workflows now include jobs specifically targeting the macOS build, linking dependency and source builds through defined outputs. Additionally, build scripts are modified to handle environment variable cleanup for the Darwin host after the Qt build, and compiler command definitions have been simplified by removing unnecessary environment variable unsetting. The qt package configuration is also updated to include a new patch that removes an explicit math library link setting from the build configuration. All modifications are centered on streamlining build processes and ensuring consistency in caching and environment handling for macOS targets. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build-depends.yml (1)
80-85: Conditional SDK Setup and Shell Variable Quoting
The "Build depends" step now conditionally runs the SDK setup script for the Darwin host before building dependencies. However, the commandenv ${{ steps.setup.outputs.DEP_OPTS }} make -j$(nproc) -C dependstriggers a ShellCheck warning (SC2046) concerning unquoted variables, which might lead to unexpected word splitting if
DEP_OPTScontains spaces. Consider verifying whether quoting is needed or if an alternative approach (such as handlingDEP_OPTSas an array) would be more robust.🧰 Tools
🪛 actionlint (1.7.4)
80-80: shellcheck reported issue in this script: SC2046:warning:5:48: Quote this to prevent word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/build-depends.yml(1 hunks).github/workflows/build-src.yml(2 hunks).github/workflows/build.yml(2 hunks)contrib/guix/libexec/build.sh(2 hunks)depends/hosts/darwin.mk(1 hunks)depends/packages/qt.mk(2 hunks)depends/patches/qt/darwin_no_libm.patch(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build-depends.yml
80-80: shellcheck reported issue in this script: SC2046:warning:5:48: Quote this to prevent word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (10)
depends/patches/qt/darwin_no_libm.patch (1)
1-18: LGTM! Well-documented patch for macOS build.The patch correctly removes the redundant
-lmlink flag, as the math library is automatically linked when using GCC with libstdc++. The change is specific to macOS cross-build and is well-documented with a clear explanation.depends/hosts/darwin.mk (1)
53-60: LGTM! Simplified compiler command definitions.The changes streamline the compiler commands by removing unnecessary environment variable unsetting. The simplified definitions maintain all required flags and options while being more maintainable.
depends/packages/qt.mk (1)
28-28: LGTM! Correctly integrated macOS build patch.The
darwin_no_libm.patchis properly added to the Qt package's patch list and preprocessing sequence, ensuring it's applied during the build process.Also applies to: 262-262
.github/workflows/build-src.yml (2)
47-56: LGTM! Well-structured SDK cache restoration for macOS.The SDK cache restoration step is properly conditioned for macOS builds and uses a reliable cache key based on the darwin.mk hash. The fail-on-cache-miss flag ensures builds don't proceed without the required SDKs.
69-74: LGTM! Improved ccache key generation.The ccache key now includes hashes of both the Dockerfile and package files, ensuring proper cache invalidation when build environment or dependencies change.
.github/workflows/build-depends.yml (2)
56-66: Enhanced SDK Caching for macOS Builds
A new cache step ("Cache SDKs") has been added to conditionally cache SDK sources when the build target is mac. This is a good practice to speed up macOS builds, and the key is derived from the hash ofdepends/hosts/darwin.mk, ensuring proper invalidation when that file changes.
74-77: Improved Cache Key for Depends Restoration
The cache restoration key now incorporates the hash ofcontrib/containers/ci/Dockerfilealong with files fromdepends/packages/*and the build target. This change ensures that modifications to the CI Dockerfile trigger cache invalidation, reducing the chance of using outdated cache artifacts..github/workflows/build.yml (2)
52-59: Addition of macOS Depends Job
The newdepends-macjob has been introduced to build dependencies for the macOS target (x86_64-apple-darwin). It correctly uses the existing build-depends workflow with appropriate parameters. This aligns well with the broader goal of integrating macOS builds into the CI pipeline.
149-157: Addition of macOS Source Build Job
Thesrc-macjob is now in place to build the source for the macOS target. It properly depends on both the container job and the newdepends-macjob, and it passes the correct cache key to the build-src workflow. This is a clear and concise integration for macOS support.contrib/guix/libexec/build.sh (1)
183-190: Clearance of Environment Variables Post Qt Build on Darwin
The new case block for hosts matching*darwin*unsets critical environment variables (C_INCLUDE_PATH,CPLUS_INCLUDE_PATH, andLIBRARY_PATH) once the Qt build is completed. This helps prevent potential conflicts or residual settings in subsequent steps and aligns with the improved management of build environments for macOS.
Additional Information
Depends on ci: refactor GitHub Actions CI to use reusable workflows, respect
DEP_OPTS, build multiprocess depends with Clang #6400WINEPREFIXneeds to be overridden because of permissions issues with GitHub Actions (build)bitcoin#30451 will enable
ccacheto work with macOS cross-compilation builds (i.e. they will no longer be registered as uncacheable).After merging in dash#6564, existing caches resulted in across-the-board build failures (build, build, build) due to differing compiler versions (GCC 13 build on GCC 11 based cache) and differing glibc versions (2.39 build vs 2.35 cache) due to the change from
jammy(on which the caches were built) tonoble(the new base image for CI).To prevent this from occurring in the future, we will add CI's
Dockerfileto thehashFiles()used to generate the root of the cache key to ensure that the cache is discarded if it is modified.runner.osdoesn't help us as it tells us what platform the runner is on and is independent from what the base image in the container is using, we only care about the latter.The SDKs were cached separately because
The sources cache is shared between variants and the fastest runner (usually a Linux runner) will set the cache and as the download and extraction of the macOS SDK is only done when expected to target macOS, the cache associated with the shared key will not have the requisite SDKs.
Then when that cache key is used to pull the sources cache by the macOS build, it will succeed due to a cache hit and then fail building because the compiler cannot locate the SDK.
The build cache is expected to have a higher churn rate as it is tied to the platform, which will trigger more than necessary downloads of the SDK.
depends/hosts/darwin.mk. Meaning, that file needs to be included when computinghashFiles(). But this also would result in unnecessary cache misses for all the platforms that aren't macOS (because the structure of the build key is the same among all variants).Breaking Changes
None expected.
Checklist