* Run git config earlier and only once
* Run git merge in the yaml, before calling the bash script
* Run git reset in the yaml as well, for symmetry
* Replace "git merge --abort" with "git reset --hard", because it does
not fail when already up to date and no merge was started.
faa807bdf8 ci: Merge master in test-each-commit task (MarcoFalke)
Pull request description:
The `test-each-commit` task will often fail, when the CI config yaml is updated along with code changes.
This is because, GitHub seems to be merging the CI config on a fresh pull with the current target branch (`master`). However, the code changes are not.
A tedious workaround would be for every developer to rebase on every intermittent (https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740911853) and non-intermittent CI issue.
However, fix this instead by merging with `master`.
ACKs for top commit:
laanwj:
ACK faa807bdf8
hebasto:
ACK faa807bdf8.
Tree-SHA512: 4849bd558dc6cdc7d86b95164ccee32ab7c08c9b7d31cf8ec5c8e9a2251fc819630f8fa9b929ed39e8e033c67bb006f0beb33e0de216e1224680be88c5fa0161
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This change:
1. Collects build artifacts in dedicated locations.
2. Allows running bitcoin-chainstate.exe with bitcoinkernel.dll directly
from the build tree on Windows.
8888ee4403 ci: Allow build dir on CI host (MarcoFalke)
Pull request description:
This is required to pass cross builds on to a different machine after the build.
See for example https://github.com/bitcoin/bitcoin/pull/31176, but this pull will also allow someone to implement it outside this repo.
ACKs for top commit:
davidgumberg:
lgtm ACK 8888ee4403
hebasto:
re-ACK 8888ee4403.
Tree-SHA512: a1e2c32bc1b95efbd0b48287ac5b49e0e1bacbf5a5800845be5352bbdd3e17fa478e90348b2e94e95cf3ae863cdf75ab444089376588f6f8eec438f73a4b5b97
According to the description for pkg-config, "pkgconf is a
replacement for pkg-config, providing additional functionality
while also maintaining compatibility. This package only provides
a dependency link to the pkgconf package to help with package
upgrades. It can be safely removed."
Thus several scripts and markdown files are updated.
b031b7910d [ci] Split out native fuzz jobs for macOS and windows (dergoegge)
Pull request description:
Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.
In both jobs the fuzz binary is built with `-DBUILD_FOR_FUZZING` to enable `Assume` assertions as well as `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.
ACKs for top commit:
maflcko:
re-lgtm ACK b031b7910d
achow101:
ACK b031b7910d
hebasto:
ACK b031b7910d.
Tree-SHA512: 7d0dc5a9cb299f6f4596dd9a5526b6aaf82efc6eba308bdc9d8b0a45f79dea87204fb6cd4b2ea2a1bd952466b2e958d64021999296d110d7a83c1667f4de51fe
fafbf8acf4 Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to execute a fuzz target (MarcoFalke)
fae3cf0ffa ci: Temporarily disable macOS/Windows fuzz step (MarcoFalke)
Pull request description:
`g_fuzzing` is used inside `Assume` at runtime, causing significant overhead in hot paths. See https://github.com/bitcoin/bitcoin/issues/31178
One could simply remove the `g_fuzzing` check from the `Assume`, but this would make fuzzing a bit less useful. Also, it would be unclear if `g_fuzzing` adds a runtime overhead in other code paths today or in the future.
Fix all issues by making `G_FUZZING` equal to the build option `BUILD_FOR_FUZZING`, and for consistency in fuzzing, require it to be set when executing any fuzz target.
Fixes https://github.com/bitcoin/bitcoin/issues/31178
Temporarily this drops fuzzing from two CI tasks, but they can be re-added in a follow-up with something like https://github.com/bitcoin/bitcoin/pull/31073
ACKs for top commit:
marcofleon:
Tested ACK fafbf8acf4
davidgumberg:
I still ACK fafbf8acf4 for fixing the regression measured in #31178.
ryanofsky:
Code review ACK fafbf8acf4 but approach -0, because this approach means libraries built for fuzz testing do not function correctly if used in a release, and libraries built for releases are mostly useless for fuzz testing. So I would like to at least consider other solutions to this problem even if we go with this one.
dergoegge:
utACK fafbf8acf4
Tree-SHA512: 124fc2e8b35e0c4df414436556a7a0a36cd1bec4b3000b40dcf2ab8c85f32e0610bf7f70d2fd79223d62f3c3665b6c09da21241654c7b9859461b8ca340d5421
The ci "test-each-commit" job fetches the PR branch being tested with a depth of (# of commits in PR + 2), and then tries to run tests on commits after the most recent merge commit.
When a PR is opened against a bitcoin core branch, a merge commit is always guaranteed to exist within the fetch depth, because bitcoin core branches always point at merge commits.
However, in fork repositories, pull requests can be opened based on other branches that don't contain recent merge commits, and this will currently cause git rev-list to fail with fatal: bad revision '^^@'.
Work around this problem by not requiring a recent merge commit, and just testing on all fetched commits if a merge commit can't be found.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
DANGER_CI_ON_HOST_CACHE_FOLDERS if set will mount caches in directories on the host rather than in docker volumes. Supports saving and restoring caches on Github Actions.
9eea51d905 ci: move Asan / LSan / USDT job to Github Actions (Max Edwards)
4b527fa93b ci: add IPV6 network to ci container (Max Edwards)
Pull request description:
PR for moving the ASAN + LSAN + USDT + friends job to github actions from Cirrus.
The motivation for this PR is that this task needs a full VM (or bare metal) to function, because of the tracepoints. It can not run in a container on an arbitrary Linux, because the outside machine must exactly match the specification of the distro used in the CI task config. This requires more maintenance for the persistent worker, and I think moving to GHA will reduce the maintenance burden, or at least make it possible for anyone to work on.
Also, it makes it easier to run the task on forks (bitcoin-inquisition, bitcoin-knots, devel forks, ...) without having to set-up a real machine.
ACKs for top commit:
maflcko:
review ACK 9eea51d905
achow101:
ACK 9eea51d905
hebasto:
ACK 9eea51d905.
Tree-SHA512: 1111c1c9e3a11e725dff1344643fff3c91fb9b4d7c1cc9a7d507a8f146f5223316a00272030b41ae37ecb59d044f2e90e1cd907450049b25f094f0b60643d4c7
GHA Windows images previously had multiple VC Build Tools installed,
which required specifying the `VCPKG_PLATFORM_TOOLSET_VERSION`
explicitly to avoid linker errors. This issue has been resolved as per
https://github.com/actions/runner-images/issues/9701.
115c283516 ci: add print of powershell version to win64 job (Max Edwards)
Pull request description:
Extraction of just printing powershell version from closed PR: https://github.com/bitcoin/bitcoin/pull/29581
See https://github.com/bitcoin/bitcoin/pull/29581#issuecomment-1984212990 for the cause of a CI failure which was a powershell update.
This PR will make it easier to notice in the future that PS has changed.
ACKs for top commit:
hebasto:
ACK 115c283516. We still use PowerShell in some steps of the "Win64 native" CI job.
Tree-SHA512: 4c7ba9df4f0a98491120326f05e877a995f43a387fe9bbd193549b32f5a4488f85f83e472c9277db457110a7deda04f08832fe6e8129aff4b0b7278be23d4e35
Homebrew attempts to check for outdated dependents or those with broken
linkage. Such behavior might lead to failures when Homebrew updates them
on old macOS images.
This change prevents such behavior.
The GHA VS installation includes its own vcpkg package manager, which is
available since VS 17.6. This change avoids any ambiguity about which
copy of vcpkg we run.
88c8e3a0e4 github actions: Fix test-one-commit when parent of head is merge commit (Ryan Ofsky)
Pull request description:
Instead of figuring out the commit *after* the last merge and rebasing on that with a ~1 suffix, just figure out the last merge commit directly and rebase on it. This way, if HEAD happens to be a merge commit, the rebase just succeeds immediately without blank variables or errors.
Explanation of the problem from https://github.com/bitcoin/bitcoin/pull/28497#issuecomment-1743430631:
> The problem is that the PR only contains a one commit after the last merge, so the job _should_ be skipped, but the `pull_request.commits != 1` check is not smart enough to skip it because the PR is based on another PR and has merge ancestor commits. So specifically what happens is that after HEAD~ is checked out, the new HEAD is a merge commit, so the range `$(git log --merges -1 --format=%H)..HEAD` is equivalent to HEAD..HEAD, which is empty, so the `COMMIT_AFTER_LAST_MERGE` variable is empty and the rebase command fails.
Note: In the current version of this PR, the "test each commit" job is skipped, because this PR only contains a single commit. But I manually verified the code works in earlier versions of the PR that included dummy commits.
ACKs for top commit:
maflcko:
lgtmrecr ACK 88c8e3a0e4
RandyMcMillan:
utACK 88c8e3a
Tree-SHA512: a6865b5c8b96eb0b622b3255971a3cf050dd0f5a356cdfcf7f0cbb659e4a363612e8e62b3ae4fd6b5d9a40bc29176891bc4690659b026c5ef8feea25c8e263cc
Instead of figuring out the commit *after* the last merge and rebasing on that
with a ~1 suffix, just figure out the last merge commit directly and rebase on
it. This way, if HEAD happens to be a merge commit, the rebase just succeeds
immediately without blank variables or errors.
From https://github.com/bitcoin/bitcoin/pull/28497#issuecomment-1743430631:
The problem is that the PR only contains a one commit after the last merge,
so the job _should_ be skipped, but the `pull_request.commits != 1` check
is not smart enough to skip it because the PR is based on another PR and
has merge ancestor commits. So specifically what happens is that after
HEAD~ is checked out, the new HEAD is a merge commit, so the range `$(git
log --merges -1 --format=%H)..HEAD` is equivalent to HEAD..HEAD, which is
empty, so the `COMMIT_AFTER_LAST_MERGE` variable is empty and the rebase
command fails.
fa5356cd49 ci: Limit test-each-commit to --max-count=6 (MarcoFalke)
fafcd2e9ef ci: Add test-each-commit task (MarcoFalke)
Pull request description:
Currently, if a pull request has more than one commit, previous commits may fail to compile, or may fail the tests. This is problematic, because it breaks git-bisect, or worse.
Fix this by adding a CI task for this.
ACKs for top commit:
jonatack:
ACK fa5356cd49
dergoegge:
utACK fa5356cd49
hebasto:
ACK fa5356cd49
Tree-SHA512: 304eff5545501ee499b881f0b0a0c1fe32a7c9f00d96b45bba731af08ac5ca917cef223f5c3d346e30c11a3e6e12e1da297929d3caea9644f3ec1de25dd97c37
fa3b816240 doc: Fill in the required skills in the good_first_issue template (MarcoFalke)
Pull request description:
Compiling and running the tests is always required, so fill it in to avoid having to type it manually every time.
ACKs for top commit:
willcl-ark:
ACK fa3b816
Tree-SHA512: 1bcb93aaff235dd62513cda05547db90d12ad7638c050ee125845d20df1e1bc457bf4ec590677a0875fae8729dcc58842398e637e517997b35e3b3adffc34a72
Lőrinc
Hennadii Stepanov
fanquake
MarcoFalke
Sjors Provoost
Ryan Ofsky
espi3
Adlai Chandrasekhar
Ava Chow
dergoegge
furszy
Sergi Delgado Segura
Antoine Poinsot
laanwj
Max Edwards
Max Edwards
Fabian Jahr