2a746500fa ci: migrate some jobs to Debian Trixie, use GCC 14 (fanquake)
fb0e6edfe8 guix: Apply SSA generation patch to maintain determinism (Mara van der Laan)
34909799fe guix: use GCC 14.3.0 over 13.3.0 (fanquake)
47be9122a7 guix: disable gprofng in GCC (fanquake)
ea29329eb7 guix: build GCC with --enable-host-bind-now (fanquake)
6f54e267d0 guix: disable libquadmath in GCC (fanquake)
7735901ed2 guix: disable building libgomp in GCC (fanquake)
Pull request description:
Switching to using GCC 14.x for release builds has come up multiple times recently. It will eventually be needed for #25573, and could also be useful for #30210.
ACKs for top commit:
hebasto:
ACK 2a746500fa. I have reviewed the code and it looks OK. The new GCC patch looks reasonable.
theuni:
utACK 2a746500fa
sedited:
ACK 2a746500fa
Tree-SHA512: 56912bed19386f06d52fb94e0ef6d96f5415ab2de8b5e94890806d7cc0b937a3c4b11cc161aa2e06ca2fd3c392ef7501c91688e0897e1c1c51aafa963f3e50d9
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
fac5a1b10a test: Allow mempool_updatefromblock.py to run on 32-bit (MarcoFalke)
Pull request description:
The number of dropped parent transactions in the `test_max_disconnect_pool_bytes` test was hard-coded to `2`.
This happens to work fine on 64-bit for now. However, it seems to fail on 32-bit (https://github.com/bitcoin/bitcoin/issues/34108).
I don't think we care about the exact number, as long as it is at least `1`.
So hard-code `1` for an initial sanity check, and then calculate the exact value at runtime via `len(mempool) // 2`.
Also, enable the functional tests in 32-bit CI, to confirm the regression test.
Fixes https://github.com/bitcoin/bitcoin/issues/34108
ACKs for top commit:
bensig:
ACK fac5a1b10a
instagibbs:
ACK fac5a1b10a
Tree-SHA512: 8d468f306d95e52cbfac1803293e3b8e9575c9010200010c7833382112509e0d51827dc9681b0b68eeae742af2c14d12da5fd4cf0e1d871a02f91fc80e6720d1
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
This should avoid having to include interfaces/chain.h from a kernel
module. interfaces/chain.h in turn includes a bunch of non-kernel
headers, that break the desired library topology and might introduce
entanglement regressions.
Specifically gets rid of batchpriority, chainparams, script/sign.h and
system includes.
Also take the opportunity of cleaning up the headers for the effected
files and adding them to the iwyu-enforced set.
fa336053aa Move ci_exec to the Python script (MarcoFalke)
fa83555d16 ci: Require rsync to pass (MarcoFalke)
eeee02ea53 ci: Untangle CI_EXEC bash function (MarcoFalke)
fa21fd1dc2 ci: Move macos snippet under DANGER_RUN_CI_ON_HOST (MarcoFalke)
fa37559ac5 ci: Document the retry script in PATH (MarcoFalke)
666675e95f ci: Move folder creation and docker kill to Python script (MarcoFalke)
Pull request description:
The remaining `ci/test/02_run_container.sh` is fine, but has a bunch of shellcheck SC2086 word splitting violations.
This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces.
However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes.
ACKs for top commit:
frankomosh:
Code Review ACK [fa33605](fa336053aa)
m3dwards:
ACK fa336053aa
Tree-SHA512: 472decb13edca75566dffe49b9b3f554ab977fa60ec7902d5a060fe53381aee8606a10ff0c990a62ee2454dc6d9430cc064f58320b9043070b7bf08845413bf4
fad6118586 test: Fix "typo" in written invalid content (MarcoFalke)
fab085c15f contrib: Use text=True in subprocess over manual encoding handling (MarcoFalke)
fa71c15f86 scripted-diff: Bump copyright headers after encoding changes (MarcoFalke)
fae612424b contrib: Remove confusing and redundant encoding from IO (MarcoFalke)
fa7d72bd1b lint: Drop check to enforce encoding to be specified in Python scripts (MarcoFalke)
faf39d8539 test: Clarify that Python UTF-8 mode is the default today for most systems (MarcoFalke)
fa83e3a81d lint: Do not allow locale dependent shell scripts (MarcoFalke)
Pull request description:
Historically, there was an attempt via `test/lint/lint-python-utf8-encoding.py` to enforce explicit UTF8 in every Python IO statement (`open`, `subprocess`, ...). However, the lint check has many problems:
* The check is incomplete and many IO statements lack the explicit UTF8 specification.
* It was added at a time when some systems were not UTF8 by default.
* The check is brittle, as it depends on a fragile regex.
In theory, now that the minimum Python version is 3.10 (since commit 2123c94448), the check could be replaced by `PYTHONWARNDEFAULTENCODING=1` from https://docs.python.org/3/whatsnew/3.10.html#optional-encodingwarning-and-encoding-locale-option. However, this comes with many other problems:
* All our Python scripts already assume and require UTF8 to be set externally. On almost all modern systems, this is already the default. Some Windows versions do not have UTF8 by default and require `PYTHONUTF8=1` to be set for the tests to run already today (with or without the changes in this pull). Also, the CI and many other Bash scripts force UTF8 via `LC_ALL`. Finally, Python 3.15 will likely enable UTF8 on *all* systems by default, per https://peps.python.org/pep-0686/#abstract.
* So adding UTF8 to every single IO call is redundant, verbose, and confusing, given that it is the expected default.
So fix all issues, by:
* Removing the `test/lint/lint-python-utf8-encoding.py` check.
* Removing the encoding on the individual IO calls.
* Clarifying the existing docs around the existing UTF8 requirement and assumption.
Obviously, every IO call is still free to specify UTF8 or any other encoding explicitly, if there is a documented need for it in the future.
ACKs for top commit:
theStack:
re-ACK fad6118586
laanwj:
Re-ACK fad6118586
Tree-SHA512: 78025ea3508597d2299490347614f0ee3e4c66e3ba559ff50e498045a9c8bbd92f3a5ced18719d8fcebbd1e47bdbb56a0c85a5b73b425adb0ea4f02fe69c3149
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
Starting with Python 3.11, Pythons gzip might delegate to zlib.
Depending on the OS, i.e Ubuntu vs Fedora, the underlying zlib
implementation might differ, resulting in different output.
For now, or until a better solution exists, disable compression. This
results in the SDK increasing in size to ~157mb. Which is not
unreasonable, to regain determinism (and would be significantly worse
without the previous commit).
See: https://docs.python.org/3/library/gzip.html#gzip.compress
Co-authored-by: stickies-v <stickies-v@protonmail.com>
The encoding arg is confusing, because it is not applied consistently
for all IO.
Also, it is useless, as the majority of files are ASCII encoded, which
are fine to encode and decode with any mode.
Moreover, UTF-8 is already required for most scripts to work properly,
so setting the encoding twice is redundant.
So remove the encoding from most IO. It would be fine to remove from all
IO, however I kept it for two files:
* contrib/asmap/asmap-tool.py: This specifically looks for utf-8
encoding errors, so it makes sense to sepecify the utf-8 encoding
explicitly.
* test/functional/test_framework/test_node.py: Reading the debug log in
text mode specifically counts the utf-8 characters (not bytes), so it
makes sense to specify the utf-8 encoding explicitly.
The Bash script was acceptable, but CI_EXEC_CMD_PREFIX was a single
string, relying on brittle word splitting that the shellcheck SC2086
would warn about.
So just fix that by moving everything to the Python script and deleting
the Bash script.
This also removes the need to export the CI_CONTAINER_ID env var.
In theory one could run the CI without the rsync package installed, and
with DANGER_RUN_CI_ON_HOST=1. However, this seems to be an edge case.
Simply requiring rsync to be installed is less code and avoids brittle
edge cases around rsync failures.
It contains a large `bash -c` string, which is hard to parse. So pull
out components:
* CI_EXEC is only called with absolute folders as args, so the `cd` is
not needed in CI_EXEC. It is only needed to specify the working dir of
running the tests in 03_test_script.sh, so move it there.
* The PATH modification is only needed after commit
4756114e50 to check that depends does
work properly, even when the PATH contains a space.
* This allows to also drop the `bash -c` and use the proper and safer
"$@" to forward args without the risk of word splitting.
This move-only refactor clarifies that macos assumes and requires
DANGER_RUN_CI_ON_HOST.
So move the snippet under the condition for self-documenting code.
Can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The `retry` script is required for CI_RETRY_EXE and there are two ways
to put it into PATH:
* When running in a container engine, by copying it into /usr/bin
* When running without a container engine, by prepending its location to PATH
The option was fine, but now that there is a dedicated Alpine Linux
task, which uses BusyBox, it seems redundant.
(See: ci/test/00_setup_env_native_alpine_musl.sh)
So remove the USE_BUSY_BOX option, along with the BINS_SCRATCH_DIR env
var.
Also, enable pipefail in the ci/test/00_setup_env.sh script, while
touching it.
fae83611b8 ci: [refactor] Use --preset=dev-mode in mac_native task (MarcoFalke)
fadb67b4b4 ci: [refactor] Base nowallet task on --preset=dev-mode (MarcoFalke)
6666980e86 ci: Enable bitcoin-chainstate and test_bitcoin-qt in win64 task (MarcoFalke)
faff7b2312 ci: Enable experimental kernel stuff in i686 task (MarcoFalke)
fa1632eecf ci: Enable experimental kernel stuff in mac-cross tasks (MarcoFalke)
fad10ff7c9 ci: Enable experimental kernel stuff in armhf task (MarcoFalke)
fa9d67c13d ci: Enable experimental kernel stuff in Alpine task (MarcoFalke)
fab3fb8302 ci: Enable experimental kernel stuff in s390x task (MarcoFalke)
fa7da8a646 ci: Enable experimental kernel stuff in valgrind task (MarcoFalke)
fa9c2973d6 ci: Enable experimental kernel stuff in TSan task (MarcoFalke)
fad30d4395 ci: Enable experimental kernel stuff in MSan task (MarcoFalke)
Pull request description:
Most of the CI tasks have a long list of stuff that they enable. This makes it hard to see what each CI task is actually running.
Also, most of the CI tasks should probably mimic the `dev-mode` CMake preset and run on as much stuff as possible. Usually, changing the `dev-mode` comes with changing those CI tasks as well in the same commit, which is verbose.
Fix both issues, by basing most CI tasks on the `dev-mode`. In the future, this makes it easier to change the `dev-mode` in a single place. If CI tasks explicitly disable something, it will be listed explicitly in them.
As a side-effect this will enable the kernel stuff for some CI task that did not have it enabled, which seems desirable.
ACKs for top commit:
TheCharlatan:
Nice, ACK fae83611b8
janb84:
ACK fae83611b8
hebasto:
ACK fae83611b8, I have reviewed the code and it looks OK.
Tree-SHA512: 58d9d553437b57362e9ec0766bd202482435f263d3f4c6ee7020c5e1e5ba69f8c064630423424f9d754254a66981e670b964a5aee58ef87f30b7d775642255be
a3ac59a431 ci: Enable experimental kernel stuff in ASan task (MarcoFalke)
5b89956eeb kernel: Allow null arguments for serialized data (TheCharlatan)
Pull request description:
An empty span constructed from an empty vector may have a null data pointer depending on the implementation. Remove the BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead handle such null arguments in the implementation.
Also cherry-picked from #33845 to show that CI task passing now.
ACKs for top commit:
yuvicc:
Code review ACK a3ac59a431
maflcko:
review ACK a3ac59a431🥈
laanwj:
code review ACK a3ac59a431
Tree-SHA512: 629e463796f2f057df5be8e8981a45751c578ed0021be731c1d57fe849a539fe38b0a445914b0fc48f32f0408ad6d566984bd7f3a68797fcfdf1c6889e316a08
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
test_bitcoin-qt ..................... ON
IPC and USDT remain explicitly disabled.
fa9f29a4a7 doc: Recommend latest Debian stable or Ubuntu LTS (MarcoFalke)
fa1711ee0d doc: Add GCC-12 min release notes (MarcoFalke)
faa8be75c9 ci: Enable experimental kernel stuff in G++-12 task (previous releases) (MarcoFalke)
fabce97b30 test: Remove gccbug_90348 test case (MarcoFalke)
fa3854e432 test: Remove unused fs::create_directories test (MarcoFalke)
fa9dacdbde util: [refactor] Remove unused create_directories workaround (MarcoFalke)
fa807f78ae build: Bump g++ minimum supported version to 12 (MarcoFalke)
Pull request description:
All supported operating systems that previously came with at least g++-11, also come with at least g++-12, so bumping the minimum should be fine.
For reference:
* https://packages.ubuntu.com/jammy/g++-12
* https://packages.ubuntu.com/noble/g++ (g++-13)
* https://packages.debian.org/bookworm/g++ (g++-12)
* FreeBSD Ports ship a recent GCC
* RHEL-based 8, and 9 ship with g++-14 via appstream (`dnf install gcc-toolset-14` -> `/opt/rh/gcc-toolset-14/`)
* RHEL-based 10 ships with g++ (14 by default)
* OpenSuse Leap and Tumbleweed ship with g++ 15 https://software.opensuse.org/package/gcc15-c++
Obviously, downloading pre-compiled releases or compiling previous release branches is unaffected by this change.
ACKs for top commit:
janb84:
re-ACK fa9f29a4a7
TheCharlatan:
Re-ACK fa9f29a4a7
hebasto:
ACK fa9f29a4a7.
Tree-SHA512: ce14ecf78ccfe4f221dcbc9147dcfc00c0512b23a6fcda5ba71b62b4f5d39a5139f083d035113f189bfbd396d485e1ebc626a9a16b6fa0b74fd95aed2041c841
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
IPC remains explicitly disabled.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
USDT remains explicitly disabled.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
The GUI and USDT remain disabled explicitly.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
The GUI remains disabled explicitly.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
The GUI remains disabled explicitly.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Also, shorten the name, for a less cluttered web view.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
It does not make sense to use a pointer, when a reference is more
appropriate, especially given that nullptr has been ruled out.
This is also allows to remove the CI workaround to avoid warnings:
```
C++ compiler .......................... GNU 13.0.0, /bin/x86_64-w64-mingw32-g++-posix
...
/ci_container_base/src/test/blockmanager_tests.cpp: In member function ‘void blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::test_method()’:
/ci_container_base/src/test/blockmanager_tests.cpp:63:17: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
63 | const auto& chainman = Assert(m_node.chainman);
| ^~~~~~~~
In file included from /ci_container_base/src/streams.h:13,
from /ci_container_base/src/dbwrapper.h:11,
from /ci_container_base/src/node/blockstorage.h:10,
from /ci_container_base/src/test/blockmanager_tests.cpp:8:
/ci_container_base/src/util/check.h:116:49: note: the temporary was destroyed at the end of the full expression ‘inline_assertion_check<true, std::unique_ptr<ChainstateManager>&>(((blockmanager_tests::blockmanager_scan_unlink_already_pruned_files*)this)->blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::<anonymous>.TestChain100Setup::<anonymous>.TestingSetup::<anonymous>.ChainTestingSetup::<anonymous>.BasicTestingSetup::m_node.node::NodeContext::chainman, std::source_location{(& *.Lsrc_loc27)}, std::basic_string_view<char>(((const char*)"m_node.chainman")))’
116 | #define Assert(val) inline_assertion_check<true>(val, std::source_location::current(), #val)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/src/test/blockmanager_tests.cpp:63:28: note: in expansion of macro ‘Assert’
63 | const auto& chainman = Assert(m_node.chainman);
| ^~~~~~
cc1plus: all warnings being treated as errors
gmake[2]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake[2]: *** [src/test/CMakeFiles/test_bitcoin.dir/build.make:382: src/test/CMakeFiles/test_bitcoin.dir/blockmanager_tests.cpp.obj] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1810: src/test/CMakeFiles/test_bitcoin.dir/all] Error 2
gmake[1]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-w64-mingw32'
gmake: *** [Makefile:146: all] Error 2
```
This false-positive warning is also fixed in later GCC versions.
See also https://godbolt.org/z/fjc6be65M
fad6efd3be refactor: Use STR_INTERNAL_BUG macro where possible (MarcoFalke)
fada379589 doc: Remove unused bugprone-lambda-function-name suppression (MarcoFalke)
fae1d99651 refactor: Use const reference to std::source_location (MarcoFalke)
fa5fbcd615 util: Allow Assert() in contexts without __func__ (MarcoFalke)
Pull request description:
Without this, compile warnings could be hit about `__func__` being only valid inside functions.
```
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
```
Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
This also introduces a slight behaviour change, because `std::source_location::function_name` usually includes the entire function signature instead of just the name.
ACKs for top commit:
l0rinc:
Code review ACK fad6efd3be
stickies-v:
ACK fad6efd3be
hodlinator:
re-ACK fad6efd3be
Tree-SHA512: e78a2d812d5ae22e45c93db1661dafbcd22ef209b3d8d8d5f2ac514e92fd19a17c3f0a5db2ef5e7748aa2083b10c0465326eb36812e6a80e238972facd2c7e98
