fa36adeb71 ci: [refactor] Drop last use of pwsh (MarcoFalke)
fae31b1e2f ci: [refactor] Move github_import_vs_env to python script (MarcoFalke)
Pull request description:
The use of pwsh was a bit confusing and inconsistent around the exit code. See also https://github.com/bitcoin/bitcoin/pull/32672
I think it is fine to drop it and purely use Bash/Python.
This also moves a bit of code from the github yaml to the python script.
ACKs for top commit:
m3dwards:
Looks good! re-ACK fa36adeb71
hodlinator:
re-ACK fa36adeb71
Tree-SHA512: 78edffc60c58c476b0acca5224150169d154b0b818114844a04295af9ba19b7cdf1fb2afb738f6cafd6172f0f477d546018ebf95061eb5bd8bbb35e065a129d4
The standard and fuzz matrix jobs share the same github.job value
(windows-native-dll), so both try to save the vcpkg tools cache with the
same key.
Since the tools are identical across build types, let them share a
single cache entry by restricting the save to the standard job only.
eb510f8678 ci: fail fast in test-each-commit script (Lőrinc)
04c4d71008 ci: remove commit count limit from `test-each-commit` (Lőrinc)
Pull request description:
### Problem
`test-each-commit` currently tests only a limited number of ancestor commits in a PR, so failures introduced deeper in the commit stack might be missed.
### Fix
Remove the max-count limit so `test-each-commit` runs the full build + unit + functional test flow on every non-head PR commit, while keeping the PR tip excluded because it is already covered by the normal CI jobs.
It will also stop after the first failure to surface the root cause sooner and keep logs readable when testing ancestor commits.
### Examples
* Example failure 10 commits deep: https://github.com/l0rinc/bitcoin/actions/runs/21390976651/job/61577575033?pr=105 in https://github.com/l0rinc/bitcoin/pull/105
* Example pass with >7 dummy commits: https://github.com/l0rinc/bitcoin/actions/runs/21392557521/job/61595159841?pr=106 in https://github.com/l0rinc/bitcoin/pull/106
---------
Note: this PR has gone through a few iterations, the latest one just extends the existing job.
ACKs for top commit:
maflcko:
lgtm ACK eb510f8678🕓
hebasto:
re-ACK eb510f8678.
willcl-ark:
ACK eb510f8678
Tree-SHA512: 5aadafd32daad610ce882277802c390642dc34f7d5bfa71d4b503ee007942d1ebafce2a3430ea5fd2af6673c83f9aee42450043be4722d7c02407d90920f8bce
fa13b13239 ci: [refactor] Use pathlib over os.path (MarcoFalke)
fa2719ab1b ci: [refactor] Move run_unit_tests to ci-windows-cross.py (MarcoFalke)
fa99ba5f14 ci: Set PREVIOUS_RELEASES_DIR env var in ci-windows-cross.py (MarcoFalke)
fa4a1cab6c ci: Move run_functional_tests into ci-windows-cross.py (MarcoFalke)
1111108685 ci: [refactor] Move pyzmq install and get_previous_releases into ci-windows-cross.py (MarcoFalke)
fac9c7bd66 ci: [refactor] Move config.ini rewrite to ci-windows-cross.py (MarcoFalke)
faf7389466 ci: Move check_manifests step to ci-windows-cross.py (MarcoFalke)
fa674d55df ci: [refactor] Move print_version step into ci-windows-cross.py helper (MarcoFalke)
Pull request description:
Currently the ci yaml has a mix of Bash and Pwsh snippets, which is problematic:
* The `shellcheck` tool does not review the Bash
* The ci yaml is not merged with master on re-runs, but the code is, leading to possibly confusing CI errors on re-runs
* The Pwsh isn't reviewed at all by any tool
* It is tedious to run the CI commands locally on Windows
Fix all issues by extracting them into a step-based Python script.
ACKs for top commit:
janb84:
re ACK fa13b13239
hebasto:
ACK fa13b13239, I have reviewed the code and it looks OK.
Tree-SHA512: 23d21d3bfb07e102fe1cc15ba5749d553d9766ae6c4a7648bd77df0705469bd138c76a9a2fdeb4d91d3f889a425b7caf25878ecb2e68b604faf9665f8df4eb6d
The vcpkg tools cache was using the combined actions/cache action,
which saves on every run regardless of branch. Split it into the
restore/save pattern used by the other caches, so that saves only
happen on default branch pushes.
fa90277d22 ci: Use ubuntu-slim for [meta] runners (MarcoFalke)
fa9627af9f ci: Rely on cmake --preset toolchain file (MarcoFalke)
fa3f89acaa ci: Add check_manifests to ci-windows.py (MarcoFalke)
1111079a16 ci: Add run_tests step to ci-windows.py (MarcoFalke)
fa561682ce ci: [refactor] Add .github/ci-windows.py prepare_tests step (MarcoFalke)
fa3e607c6d ci: Print verbose Windows CI build failure (MarcoFalke)
4444808dd3 ci: [refactor] Add .github/ci-windows.py build step (MarcoFalke)
fabdd4e823 ci: Refactor Windows CI into script (MarcoFalke)
Pull request description:
Just like all the other CI configs, the Windows one should print a single and readable build failure at the end.
Also, includes a bunch of Windows CI refactors.
ACKs for top commit:
m3dwards:
ACK fa90277d22
hebasto:
ACK fa90277d22.
willcl-ark:
utACK fa90277d22
Tree-SHA512: 00443289e3d8b6d60d1347934d9d4b16098e8c36b6325467e5804b1869714201c4f7e932da3be44392c73e4713a1f52cd8af481894d36c6a281ba7238d43434e
This is the standard approach and avoids relying on
VCPKG_INSTALLATION_ROOT and -DCMAKE_TOOLCHAIN_FILE= in the ci-windows.py
script.
This makes it easier to run locally.
This is mostly a refactor, except for using the runner workspace (cwd) for:
* fuzz qa-assets
* functional temp prefix
This makes it easier to run the ci-windows.py locally.
faa4ab113c ci: Drop valgrind fuzz from GHA matrix (MarcoFalke)
Pull request description:
The valgrind fuzz task is problematic, because:
* It is redundant with the msan fuzz task, which has std lib hardening enabled, so often UB is diagnosed before it even happens in the valgrind task.
* All issues so far found by the valgrind fuzz task were also found by the hardened msan fuzz task.
* All other issues were false-positives, which are hard to debug, and confusing and tedious to work around.
I don't think there is any value in asking pull request authors to debug valgrind false-positives that they triggered by accident. So remove the task for now.
I know that there are some devs, who like to keep the task, but if the task is kept, it should come with clear instructions on how to deal with false-postives in pull requests.
I am not proposing to remove the config itself, and I am happy to continue maintaining it, like it was done before. However, as of now, running it in the GHA matrix is of negative or questionable benefit.
ACKs for top commit:
l0rinc:
ACK faa4ab113c
fanquake:
ACK faa4ab113c - hopefully we can revisit re-adding soon. To be clear, I don't agree with the rationale from #34304, or the initial changes there. The case here, and the fact that it is causing disruption in this repo, is more pressing.
Tree-SHA512: 59272f4b5b01c3b8ee6078ea635441f17776d4d8923f1adacdabdbb00bd2eb0234b30dc5b27938e29f8e79b3c3bebed5f339ae36c2c8fb17ea9d3a2884bee986
This makes it easier to:
* Run the exact command of any CI type and step locally
* Re-Run older CI tasks on GHA and using the latest merged config.
(.github/ci-windows.py is merged with master on re-runs, but
.github/workflows/ci.yml is NOT)
Also, writing it in Python has benefits:
* Any developer (even non-Windows ones) can read and modify the script.
* Python is already required for tests, so no new dependency is needed.
-Werror is added to the previous releases job, given it runs on Ubuntu
22.04, which uses an older CMake.
`--compile-no-warning-as-error` can be used, if needed, in future, to
suppress the `CMAKE_COMPILE_WARNING_AS_ERROR` behaviour from a CI
config.
CMAKE_COMPILE_WARNING_AS_ERROR was added to CMake in 3.24.
See https://cmake.org/cmake/help/latest/prop_tgt/COMPILE_WARNING_AS_ERROR.html.
Co-authored-by: willcl-ark <will8clark@gmail.com>
Extend `test-each-commit` to run on every non-head pull request commit.
The PR tip is excluded because it is already covered by other CI jobs.
Runner was changed to a performant cirrus runner.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
ddae1b4efa ci: remove gnu-getopt usage (fanquake)
Pull request description:
This is used for argument parsing in the `retry` script, however we don't use the script with any arguments. So remove the unused code, and the dependency on `gnu-getopt`.
This came up in the context of adding new CI jobs, where gnu-getopt might not be available, or working properly. It seemed easier to just remove the unused code, than look for more workarounds.
ACKs for top commit:
maflcko:
review ACK ddae1b4efa🔀
sedited:
ACK ddae1b4efa
Tree-SHA512: a73cf61fe0965127f87f1725b3a25a305ebfd354c318f5f44ecfa20da02ba72fef42dca656dae07f6e1ece956b9d7c58e99edb124d968a4bffb2ce6ac8fc018b
This is used for argument parsing in the retry script, however we don't
use the script with any arguments. So remove the unused code, and the
dependency on gnu-getopt.
This came up in the context of adding new CI jobs, where gnu-getopt
might not be available, or working properly. It seemed easier to just
remove the unused code, than look for more workarounds.
Add a ci/lint.py script to run the linter both locally or inside the CI
(replacing .github/ci-lint-exec.py) which supports running from a
worktree.
Determines whether we are in a worktree, and mounts the real `.git`
directory as a read-only volume if we are.
fa2959e16d test: Fail on self-check warnings in test_runner.py (MarcoFalke)
Pull request description:
I don't see a reason to start running the tests, if the test_runner detects warnings during the self-check.
Usually, this will just lead to a possibly confusing test failure after some wasted time anyway.
So just fail fast before even running any tests.
If there was a reason to ignore the warnings, a new option could trivially be added:
```py
parser.add_argument("--ignore-self-check-warnings", dest="ignore_warnings", default=False, action="store_true",
help="Ignore test runner warnings about self-checks before running the tests")
```
However, I don't see the need.
ACKs for top commit:
hodlinator:
utACK fa2959e16d
Tree-SHA512: 3f2e1af9ba06cd805c69e8c40149214a8a13af286ba6315c346e23d0f1ddae8e850103b6967d2d9799a095f6a8ec8802c3c773af8d7123598e8887f56c764fb5
c5825d4b7f qa: Require `--exclude` for each excluded test (Hennadii Stepanov)
Pull request description:
This PR allows a long `--exclude ...` argument in the `test/functional/test_runner.py` invocation to be split across multiple lines, with optional per-line explanatory comments. I found this useful for the CI scripts in https://github.com/hebasto/bitcoin-core-nightly.
ACKs for top commit:
l0rinc:
tested ACK c5825d4b7f
maflcko:
review ACK c5825d4b7f🛄
achow101:
ACK c5825d4b7f
rkrux:
ACK c5825d4b7f
Tree-SHA512: bcf42848516197978b65df8a8bc68e036a62c9afc6158274eac74a325dc01991eb063a042f940c53ea15a7feb18d4bdfc45d8c71f0ef20c76140b12e07ba3ac5
fa65bc0e79 test: Run bench sanity checks in parallel with functional tests (MarcoFalke)
fa9fdbce79 test: Pass bench exe into test framework utils (MarcoFalke)
Pull request description:
The ctest target `bench_sanity_check` has many issues:
* With sanitizers enabled, it is one of the slowest targets, often taking several minutes. See https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066.
* There is no insight from ctest into how long each individual sanity check takes.
* On a timeout, or OOM issue, there is no insight into which sub-bench failed. The failure will generally just look like `75/153 Test #9: bench_sanity_check ...................***Failed 770.84 sec out of memory`
* Places that can't use ctest (like the Windows-cross CI task) have to explicitly run it, or risk forgetting to run it.
* All benchmarks are run sequentially, when they could run in parallel instead.
Both issues can lead to CI timeouts and leave CPU unused during testing.
Fix all issues by running it as part of the functional tests instead. This is similar to the rpcauth tests (https://github.com/bitcoin/bitcoin/pull/32881) and util tests [bitcoin-tx, and bitcoin-util] (https://github.com/bitcoin/bitcoin/pull/32697).
ACKs for top commit:
achow101:
ACK fa65bc0e79
l0rinc:
Tested ACK fa65bc0e79
janb84:
tACK fa65bc0e79
willcl-ark:
ACK fa65bc0e79
Tree-SHA512: d27e363b7896a7543a4ee8df41a56e58b74f07d4f296e2e5ee293fc91817d0be310e26905755fb94d44417d94fa29ad4cc5d4aa19e78d25d41bc2d9e0948c034
56750c4f87 iwyu, clang-format: Sort includes (Hennadii Stepanov)
2c78814e0e ci: Add IWYU job (Hennadii Stepanov)
94e4f04d7c cmake: Fix target name (Hennadii Stepanov)
0f81e00519 cmake: Make `codegen` target dependent on `generate_build_info` (Hennadii Stepanov)
73f7844cdb iwyu: Add patch to prefer C++ headers over C counterparts (Hennadii Stepanov)
7a65437e23 iwyu: Add patch to prefer angled brackets over quotes for includes (Hennadii Stepanov)
Pull request description:
This PR separates the IWYU checks into its own CI job to provide faster feedback to developers. No other changes are made to the treatment of IWYU warnings. The existing “tidy” CI job will no longer run IWYU.
See also the discussion of https://github.com/bitcoin/bitcoin/pull/33779, specifically this [comment](https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3491515263):
> Maybe a better approach would be to run the enforced sections in a separate, faster job? Some of the linters are already a bit annoying to invoke locally, so I usually just run the lint job. Doing the same for the includes seems fine to me.
Based on ideas from https://github.com/bitcoin/bitcoin/pull/32953.
ACKs for top commit:
maflcko:
review ACK 56750c4f87🌄
sedited:
ACK 56750c4f87
Tree-SHA512: af15326b6d0c5d1e11346ac64939644936c65eb9466cd1a17ab5da347d39aef10f7ab33b39fbca31ad291b0b4b54639b147b24410f4f86197e4a776049882694
b5a7a685bb ci: Make the max number of commits tested explicit (Hodlinator)
Pull request description:
Gives less of a false sense of security.
ACKs for top commit:
maflcko:
lgtm ACK b5a7a685bb
rkrux:
crACK b5a7a685bb
janb84:
ACK b5a7a685bb
glozow:
lgtm ACK b5a7a685bb
Tree-SHA512: 9f50a86f440d6a551a0c1ff547e61b61b829e98cd0cd2d5ca65966af0b48d40582f698bcb039a7467c4b71166920413c334eac0e9e4f0141c3e02cd68555865b
The "zeromq" feature is already enabled by default in `vcpkg.json`, and
there appears to be no reason to omit this configuration option when
building on Windows.
2e27bd9c3a ci: Add Windows + UCRT jobs for cross-compiling and native testing (Hennadii Stepanov)
bd130db994 ci: Rename items specific to Windows + MSVCRT (Hennadii Stepanov)
Pull request description:
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593
MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
ACKs for top commit:
maflcko:
review ACK 2e27bd9c3a 🖊
fanquake:
ACK 2e27bd9c3a
Tree-SHA512: 222ca5e54646bcce9db6e20191d5891e988274e18b2f30085de6435a3b288a9d0fc414e8f76342e275ae58ee6603f751933d1faa8bdff446edf2695091f8ca4c
Clear out space on the centos job be deleteing unnecessary files.
Raised by #33293 which pointed to a solution like b7f04d7822
Only runs when cache provider (runner) is `gha`, and on the CentOS job.
