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: merge bitcoin#27314, #28954, #29765, introduce dependency options in GitHub Actions builds, fix multiprocess builds #6400

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Nov 18, 2024

Additional Information

  • Depends on ci: use actions/cache to manage depends cache #6406

  • Dependency for ci: build GCC 14, build nowallet depends and source with it, stage built packages in /opt, allowlist LLVM libc++ #6387

  • The fuzz build will continue to use GCC-built depends instead of Clang-built depends as originally planned, as Clang-built depends are multiprocess-enabled and currently multiprocess and fuzz don't play nice together (see upstream build failure for example, source, corresponding with local failure, source).

  • Clang has been bumped to 18 in anticipation of bitcoin#30201, which will drop (native_llvm, formerly known as native_clang) and mandate the presence of Clang 18 or higher for cross-compilation.

  • As DEP_OPTS is a new parameter that could require rebuilding depends, it needs to be incorporated into the cache key. The simplest way to do it is to append the hash of the file that defines DEP_OPTS (build.yml) to the cache key. This comes with the drawback that any change to build.yml could result in a cache miss due to a changed hash.

    This has been mitigated by appending the hash of DEP_OPT's value instead. As GitHub doesn't make this easy, it has to be done in a prior "Determine params" step.

    • This was taken as an opportunity to separate the matrix configuration out to matrix.json and use jq to interpret it. We can use jq to query values (dep_opts) meant for one matrix (deps) using a value (depends_name) from the other matrix (src).
    • The above setup also removes having to mention host in both matrices as the value of host is taken by the second matrix (src) by fetching it from the array of the first matrix (deps) with matching depends_name.
    • head trims the output of sha256sum to 64 characters, the hash itself isn't being trimmed but everything after it is (trailing hyphen and newline). This is also why echo -n is used in some places, to avoid newline addition resulting in a different hash value.

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 changed the title ci: introduce dependency options in GitHub Actions builds, bump to Clang 18 and set defaults, fix multiprocess builds ci: merge bitcoin#27314, introduce dependency options in GitHub Actions builds, fix multiprocess builds Nov 18, 2024
@kwvg kwvg force-pushed the ci_stuff branch 2 times, most recently from 15cdb62 to 1d1ff41 Compare November 18, 2024 10:21
@kwvg kwvg changed the title ci: merge bitcoin#27314, introduce dependency options in GitHub Actions builds, fix multiprocess builds ci: merge bitcoin#27314, #28954, introduce dependency options in GitHub Actions builds, fix multiprocess builds, bump to Clang 18 Nov 18, 2024
@kwvg kwvg mentioned this pull request Nov 18, 2024
5 tasks
Copy link

This pull request has conflicts, please rebase.

Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg added this to the 22.1 milestone Nov 20, 2024
@kwvg kwvg force-pushed the ci_stuff branch 2 times, most recently from 3642091 to 3cc8c70 Compare November 20, 2024 14:41
@kwvg
Copy link
Collaborator Author

kwvg commented Nov 20, 2024

GitHub Actions run for 3cc8c70, https://github.com/kwvg/dash/actions/runs/11935641601.

  • No Qt regression (source).
  • DEP_OPTS propagating correctly to depends build step (source).
  • Hashing working correctly (source).
  • DEP_OPTS and HOST retrieved correctly (source).

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.

light utACK 3cc8c70

I have concerns about this PR; but I'm not sure how to communicate them, and don't have good actionable feedback.

I'm concerned the patch-set as is will make the GitHub CI / build.yml harder to understand and harder to improve; but I also cannot at this moment figure out an alternative way to accomplish what is needed to be accomplished. Some of the commits just "feel" wrong, but ultimately it's just for CI, so it's not that big of a deal, and I don't have ways imagined for how to improve the situation; so probably best path is to merge, and maybe I'll figure out something I like more later.

Comment on lines +37 to +38
echo "dep-matrix=$(jq -r '.dep' -c .github/workflows/matrix.json)" >> $GITHUB_OUTPUT
echo "src-matrix=$(jq -r '.src' -c .github/workflows/matrix.json)" >> $GITHUB_OUTPUT
Copy link
Member

Choose a reason for hiding this comment

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

I kinda hate using jq in the build.yml like this; but I guess it's ok

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alternative would be having to maintain two JSON files (they would have to be JSON if we want to share params between them without resorting to duplication), which wouldn't be the worst thing in the world but imo since src will be reading from dep for values and they are configuring the same actions run, just different jobs, consolidation would be better.

Plus we'd be using jq later on regardless to read through the matrix definition later on to find dep_opts and host from depends_name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

these 2 JSON seems doens't have any dependency on each other, maybe better to have 2 files?

Is there any case, when you need both of them at once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because they the second section (src) refers to the first section (dep) later on and they ultimately represent two halves of one workflow.

- name: Determine params
id: det-params
run: |
dep_name="${{ matrix.depends_name }}"
dep_opts="$(jq -r ".dep.include[] | select(.depends_name == \"${{ matrix.depends_name }}\") | .dep_opts" -c .github/workflows/matrix.json)"
dep_hash="$(echo -n ${dep_opts} | sha256sum | head -c 64)"
echo "\"${dep_name}\" has DEP_OPTS \"${dep_opts}\" with hash \"${dep_hash}\""
echo "dep_hash=${dep_hash}" >> $GITHUB_OUTPUT
dep_host="$(jq -r ".dep.include[] | select(.depends_name == \"${{ matrix.depends_name }}\") | .host" -c .github/workflows/matrix.json)"
echo "\"${dep_name}\" has HOST \"${dep_host}\""
echo "dep_host=${dep_host}" >> $GITHUB_OUTPUT

Examples of these are explained in the PR description.

- build_target: linux64_nowallet
host: x86_64-pc-linux-gnu
depends_on: linux64
matrix: ${{ fromJson(needs.build-image.outputs.src-matrix) }}
Copy link
Member

Choose a reason for hiding this comment

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

I fear this overcomplicates build.yml and will make it harder to improve / debug / logic on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Outputs have to be generated in the previous job in order to be used in the current job and creating a new job just to read the matrix seems a bit wasteful, so the build image step is being used to also read, generate and export as output, the matrices needed for the next two jobs.

We can probably try reusable workflows (source) in the future, it'll be more GitLab-like but would invite duplication and a different kind of convoluted syntax. Depends on how reliable this setup proves to be.

.github/workflows/build.yml Show resolved Hide resolved
@UdjinM6
Copy link

UdjinM6 commented Nov 23, 2024

I think I found a way to do this with no jq/json and no duplication either (and bring two CI-s a bit closer to each other while at it), pls check UdjinM6@c40a595 (CI: Github + Gitlab)

@kwvg
Copy link
Collaborator Author

kwvg commented Nov 24, 2024

@UdjinM6, I like the idea of going back to honoring the DEP_OPTS in the matrix definitions but the proposed approach requires ensuring that every build target has the same DEP_OPTS expected by the depends build to ensure a cache hit (i.e. DEP_OPTS don't trickle down, we have to make sure that when it's re-evaluated, the same value is returned).

One of the arrangements are to use the multiprocess depends for multiprocess and tsan (and DEP_OPTS were adjusted in multiprocess with the proposed changes to make sure they build correctly) but tsan uses different DEP_OPTS (here), which is reflected in the "Determine params" step (here) (it also uses the debug depends but that changing that isn't an issue).

Another example is the SQLite build, which has a different DEP_OPTS (source) but falls back to the correct debug depends' key (source). This works so long as there aren't competing DEP_OPTS (otherwise fallback may not behave deterministically) but the reason we are introducing a DEP_OPTS key is due to the anticipation of there being competing variants (say, in between CI runs where the compiler is bumped).

This can be trivially fixed by ensuring all the DEP_OPTS match but also means that if there is a misalignment of expected DEP_OPTS, unless they're incompatible (in which case we'll get linker errors), it will continue to happen silently.

So the proposed approach does deduplicate between GitHub and GitLab configurations but requires duplication in every matrix definition script. If this is an acceptable tradeoff, I'll cherry-pick the changes. Thoughts?

@UdjinM6
Copy link

UdjinM6 commented Nov 25, 2024

Interesting... So we were using wrong depends for these build targets in CI all this time. Smth like 94e37ef (CI: https://github.com/UdjinM6/dash/actions/runs/11999294004 + https://gitlab.com/UdjinM6/dash/-/pipelines/1558281977) on top of my previous commit should fix it (NOTE: -stdlib=libc++ was skipped for BITCOIN_CONFIG in 09fe21b, shouldn't be in DEP_OPTS either to make it work).

@UdjinM6
Copy link

UdjinM6 commented Nov 25, 2024

Also, I wonder why we bump clang to 18 manually via 77795a5 instead of doing it via corresponding backports. Can you clarify this pls?

Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg changed the title ci: merge bitcoin#27314, #28954, introduce dependency options in GitHub Actions builds, fix multiprocess builds, bump to Clang 18 ci: merge bitcoin#27314, #28954, #29765, introduce dependency options in GitHub Actions builds, fix multiprocess builds, bump to Clang 18 Nov 27, 2024
@kwvg kwvg changed the title ci: merge bitcoin#27314, #28954, #29765, introduce dependency options in GitHub Actions builds, fix multiprocess builds, bump to Clang 18 ci: merge bitcoin#27314, #28954, #29765, introduce dependency options in GitHub Actions builds, fix multiprocess builds Nov 27, 2024
@kwvg
Copy link
Collaborator Author

kwvg commented Nov 27, 2024

GitHub Actions run for f7035c4, https://github.com/kwvg/dash/actions/runs/12047954759.

@kwvg kwvg requested a review from PastaPastaPasta November 27, 2024 12:03
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg marked this pull request as draft December 15, 2024 11:34
@kwvg kwvg mentioned this pull request Dec 15, 2024
5 tasks
PastaPastaPasta added a commit that referenced this pull request Dec 17, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants