fae70ba00d ci: Better tidy errors (MarcoFalke)
Pull request description:
Currently tidy errors are not nice, because the user may have to scroll up to see them in a large block of text. See for example (before) https://github.com/bitcoin/bitcoin/runs/19670551485
Fix that by `tee`ing the output to a file and summarizing the errors in the end again. See for example (after): https://github.com/bitcoin/bitcoin/runs/22647850662
ACKs for top commit:
hebasto:
ACK fae70ba00d, logs with errors look cleaner.
TheCharlatan:
ACK fae70ba00d
Tree-SHA512: dcaea557fed40089409d16ce2cbaa8a9cfbf047f601d5daadfee0823b0eed7badc12d803addc0b7b6bb3f1eaf5c787fccb2488475d32c4efd80835f386f761dd
64722e4359 ci: Drop `--enable-c++20` option (Hennadii Stepanov)
Pull request description:
This option has ceased to exist since https://github.com/bitcoin/bitcoin/pull/28349.
ACKs for top commit:
maflcko:
ACK 64722e4359
Tree-SHA512: bd392c331f775605615e1b236682269b83a1e6363a4d3f09c4d8d54495cf3d22973a921ebf6b8a9f813ba6c024d3324761f3291aaf7f473995f5eaa4c195bc43
636c9862cf ci: Bump `TIDY_LLVM_V` (Hennadii Stepanov)
Pull request description:
This PR switches to the latest [IWYU 0.22](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.22), which is compatible with Clang 18.
ACKs for top commit:
fanquake:
ACK 636c9862cf
Tree-SHA512: 78ce89244c5e487dd1be8b4bd2ca6f06d19b04b78289ebc21985110574053545dcce5eb622edf2bede2cf7bb58360170e976d30a4484a127d34dd17b1c604e9c
acc06bc91f ci, macos: Use `--break-system-packages` with Homebrew's python (Hennadii Stepanov)
ae5f72027f ci: Add workaround for Homebrew's python link error (Hennadii Stepanov)
Pull request description:
Homebrew [promoted](https://github.com/Homebrew/homebrew-core/pull/150390) `python@3.12` to the default `python3`. Now, our "macOS native" CI job is facing the following issues:
1. Installing `qt@5` [requires](https://github.com/bitcoin/bitcoin/actions/runs/8216848118/job/22471875454#step:4:51) re-installing `python@3.12`:
```
==> Fetching dependencies for qt@5: readline, python@3.12 and gettext
```
2. Re-installing `python@3.12` [fails](https://github.com/bitcoin/bitcoin/actions/runs/8216848118/job/22471875454#step:4:127) due to symbolic link conflicts on macOS `x86_64`:
```
==> Pouring python@3.12--3.12.2_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
```
3. Homebrew's `python@3.12` is marked as externally managed (according to PEP 668), necessitating different approaches for installing Python packages.
This pull request resolves all the issues mentioned above.
ACKs for top commit:
m3dwards:
reACK acc06bc91f to get the CI passing again.
Tree-SHA512: 82cf72bff328f1e0725342431ac14ad4bae7a758186d97db6c7a558e4b661dcbf3fabe536978e26e709c5f6f7f5c11aac46642634c6685f1291592d8d825ad87
fa58ae74ea refactor: Add missing include for USE_BDB, USE_SQLITE to bench/wallet_ismine.cpp (MarcoFalke)
fa31908ea8 lint: Check for missing or redundant bitcoin-config.h includes (MarcoFalke)
fa63b0e351 lint: Make lint error easier to spot in output (MarcoFalke)
fa770fd368 doc: Add missing RUST_BACKTRACE=1 (MarcoFalke)
fa10051267 lint: Add get_subtrees() helper (MarcoFalke)
Pull request description:
Missing `bitcoin-config.h` includes are problematic, because the build could silently pass, but produce an unintended result. For example, a slower fallback algorithm could be picked, even though `bitcoin-config.h` indicates that a faster feature is available and should be used.
As the build succeeds silently, this problem is not possible to detect with iwyu.
Thus, fix this by using a linter based on grepping the source code.
ACKs for top commit:
theuni:
Weak ACK fa58ae74ea.
TheCharlatan:
ACK fa58ae74ea
hebasto:
ACK fa58ae74ea, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.
Tree-SHA512: cf4346f81ea5b8c215da6004cb2403d1aaf569589613c305d8ba00329b82b3841da94fe1a69815ce15f2edecbef9b031758ec9b6433564976190e3cf91ec8181
Drop `git diff` command so it is easier to run CI locally if git checkout is a
worktree. Currently it fails because the directory is not recognized as a git
repository.
The `git diff` command was added recently in #28359 commit
fa07ac48d8 and can be avoided just by teeing the
patch to stdout
540282905d docs: ci multi-arch requires qemu (Max Edwards)
Pull request description:
On a fresh Debian system qemu isn't installed and therefore the multi-architecture CI system doesn't run.
This documentation notes that qemu is required and how to install it.
ACKs for top commit:
maflcko:
lgtm ACK 540282905d
willcl-ark:
utACK 540282905d
Tree-SHA512: 903c12cf7b16f2146b99a952577c5550d60faf65f9e72b9f4d9479b52228118ab46349e5130de5281b39da05d3bc0b4ae8a8165601e62ce145647a98ef197131
fa91bf2559 ci: Skip git install if it is already installed (MarcoFalke)
c65fde4831 ci: vary /tmp/env (Sjors Provoost)
Pull request description:
* Currently, running separate CI tasks at the same time may intermittently fail, because they race to read/write `/tmp/env`. Fix this by adding `$CONTAINER_NAME` to the file name.
* Also, add `$USER`, while touching the line, to allow different users to run the same CI task at the same time.
* Also, skip the git install if there is no need.
Ref: https://github.com/bitcoin/bitcoin/pull/29274
ACKs for top commit:
Sjors:
ACK fa91bf2559
BrandonOdiwuor:
ACK fa91bf2559
hebasto:
ACK fa91bf2559.
Tree-SHA512: 9a8479255a2afb6618f9d0796488d9430ba95266b90ce39536a9817c1974ca4049beeaab5355a38b25171f76fc386dbec06b1919aaa079f08a5a0c0a146232c8
4756114e50 [depends] Allow PATH with spaces in directory names. (Mark Friedenbach)
Pull request description:
The goal of this PR is to help close https://github.com/bitcoin/bitcoin/pull/28733. I reverted the change on `depends/config.guess` based on the feedback provided in the previous PR. I've also incorporated the test mentioned by maflcko
ACKs for top commit:
maflcko:
lgtm ACK 4756114e50
hebasto:
ACK 4756114e50, successfully built depends on Ubuntu 22.04.
TheCharlatan:
ACK 4756114e50
Tree-SHA512: ee257f6efd235839156bc236384f08d77b91debc3c257168368a71e70742639f28a3289572b8693609c1109062dc9968e461103d1f4f5679906506e94b54e649
aaaace2fd1 fuzz: Assume presence of __builtin_*_overflow, without checks (MarcoFalke)
fa223ba5eb Revert "build: Fix undefined reference to __mulodi4" (MarcoFalke)
fa7c751bd9 build: Bump clang minimum supported version to 14 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-14 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang (`clang-14`)
* https://packages.ubuntu.com/jammy/clang (`clang-14`)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang17`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bullseye/g++ (g++-10)
* https://packages.ubuntu.com/focal/g++-10
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
ACKs for top commit:
fanquake:
ACK aaaace2fd1
Tree-SHA512: 81d066b14cc568d27312f1cc814b09540b038a10a0a8e9d71fc9745b024fb6c32a959af673e6819b817ea7cef98da4abfa63dff16cffb7821b40083016b0291f
This is already used in multiple CIs, and will soon become a requirement
for most CIs, i.e when we migrate depends packages to use CMake, for
example:
https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885576324.
Some of the CIs in 21778 are failing because CMake isn't available, so
just break this out and make CMake globally available.
The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to
speed up build and avoid timeout".
It is no longer the case for our CI infrastructure, which uses self-
hosted persistent workers and depends caching.
In the current circumstances, it does not seem worth porting this
feature to the upcoming CMake-based build system.
308aec3e56 build: disable external-signer for Windows (fanquake)
35537318a1 ci: remove --enable-external-signer from win64 job (fanquake)
Pull request description:
It's come to light that Boost ASIO (a Boost Process sub dep) has in some
instances, been quietly initialising our network stack on Windows (see
PR https://github.com/bitcoin/bitcoin/pull/28486 and discussion in https://github.com/bitcoin/bitcoin/issues/28940).
This has been shielding a bug in our own code, but the larger issue
is that Boost Process/ASIO is running code before main, and doing things
like setting up networking. This undermines our own assumptions about
how our binary works, happens before we run any sanity checks,
and before we call our own code to setup networking. Note that ASIO also
calls WSAStartup with version `2.0`, whereas we call with `2.2`.
It's also not clear why a feature like external signer would have a
dependency that would be doing anything network/socket related,
given it only exists to spawn a local process.
See also the discussion in https://github.com/bitcoin/bitcoin/issues/24907. Note that the maintaince of Boost Process in general,
has not really improved. For example, rather than fixing bugs like https://github.com/boostorg/process/issues/111,
i.e, https://github.com/boostorg/process/pull/317, the maintainer chooses to just wrap exception causing overflows
in try-catch blocks: 0c42a58eac. These changes get merged in large,
unreviewed PRs, i.e https://github.com/boostorg/process/pull/319.
This PR disables external-signer on Windows for now. If, in future, someone
changes how Boost Process works, or replaces it entirely with some
properly reviewed and maintained code, we could reenable this feature on
Windows.
ACKs for top commit:
hebasto:
re-ACK 308aec3e56.
TheCharlatan:
ACK 308aec3e56
Tree-SHA512: 7405f7fc9833eeaacd6836c4e5b1c1a7845a40c1fdd55c1060152f8d8189e4777464fde650e11eb1539556a75dddf49667105987078b1457493ee772945da66e
8ea45e626e build: use macOS 14 SDK (Xcode 15.0) (fanquake)
51c97ffb69 build: patch boost process for macOS 14 SDK (fanquake)
423949a13b depends: add -platform_version to macOS build flags (fanquake)
Pull request description:
This fixes: https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1748515277 (cross-compiling with C++20 for macOS). See https://developer.apple.com/xcode/cpp/#c++20 for C++20 support in Apples libc++, some features landed with Xcode 14.3, although many more landed with Xcode 15.0.
ACKs for top commit:
hebasto:
ACK 8ea45e626e.
TheCharlatan:
ACK 8ea45e626e
Tree-SHA512: 274ce2c9b9f8e4d755c07b8d0d4897a7f92708ac64e6afb7a3f75bdb485e863fc7f40badf3a88b129ce36f6cca72f768dc2ed7fba2bdf0bb6da2bf0c8fedee10
fad2392c58 ci: Use Ubuntu 24.04 Noble for asan (MarcoFalke)
fa83b65ef8 ci: Use Ubuntu 24.04 Noble for tsan,tidy,fuzz (MarcoFalke)
Pull request description:
23.10 will be EOL mid next year, so a bump is needed before then for the `master` branch (and possibly the `26.x` branch).
Doing the bump now is fine, because the clang version is pinned to 17 inside the CI tasks. So a default clang version change in the system image should not affect the tasks. Once clang-18 is available and the default in April next year (https://discourse.ubuntu.com/t/noble-numbat-release-schedule/35649#planned-and-potentially-disruptive-archive-wide-activities-2), the pinned version could be bumped (for CI tasks that require a pin, like tidy), or the pin can be removed (for CI tasks that usually do not require a pin, like fuzz or the sanitizers).
ACKs for top commit:
fanquake:
ACK fad2392c58
Tree-SHA512: c40aede4e2281a5d539d5f65d2c08a57bf92e4a00b4f45a4260b57b7443a63d1a0603115da4a3bbd100ac5f6ade3f2eda0916e4b565573741162a76294ec0ac5
It is confusing to treat commands as a single string. This change is
also required to support paths and strings with spaces in them in the
future.
This requires replacing TEST_RUNNER_ENV with a global export, because it
no longer works. See:
```bash
$ export ENV="A=1" && $ENV ls
bash: A=1: command not found...
```
Or in the CI task:
+ DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/
+ LD_LIBRARY_PATH=/ci_container_base/depends/i686-pc-linux-gnu/lib
+ BITCOIND=bitcoin-node make -j10 check VERBOSE=1
/ci_container_base/ci/test/03_test_script.sh: line 166: BITCOIND=bitcoin-node: command not found
https://github.com/bitcoin/bitcoin/pull/28954/checks?check_run_id=19096858944https://cirrus-ci.com/task/6718317604372480
fa01f884d3 ci: Add missing COPY for ./test/lint/test_runner (MarcoFalke)
faff3e3b46 lint: Report all lint errors instead of early exit (MarcoFalke)
Pull request description:
`all-lint.py` currently collects all failures. However, the `06_script.sh` does not, since July this year (https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806).
Fix this by printing all failures before exiting.
Can be tested by modifying (for example) two subtrees in the same commit and then running the linters.
ACKs for top commit:
kevkevinpal:
ACK [fa01f88](fa01f884d3)
TheCharlatan:
lgtm ACK fa01f884d3
Tree-SHA512: c0f3110f2907d87e29c755e3b77a67dfae1f8a25833fe6ef8f2f2c58cfecf1aa46f1a20881576b62252b04930140a9e416c78b4edba0780d3c4fa7aaebabba81
43de4d3630 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
bbbbdb0cd5 ci: Add filesystem lint check (MarcoFalke)
fada2f9110 refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)
Pull request description:
Using `std::filesystem` is problematic:
* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.
Fix all issues by removing use of it and adding a linter to avoid using it again in the future.
ACKs for top commit:
TheCharlatan:
ACK bbbbdb0cd5
fanquake:
ACK bbbbdb0cd5🦀
Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
fca0a8938e ci: remove "--exclude banman" for fuzzing in mac (brunoerg)
f9b286353f fuzz: call lookup functions before calling `Ban` (brunoerg)
Pull request description:
Fixes#27924
To not have any discrepancy, it's required to call lookup functions before calling `Ban`. If we don't do it, the assertion `assert(banmap == banmap_read);` may fail because `BanMapFromJson` will call `LookupSubNet` and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (#27924).
Also, calling lookup functions before banning is what RPC `setban` does.
ACKs for top commit:
maflcko:
lgtm ACK fca0a8938e
dergoegge:
ACK fca0a8938e
Tree-SHA512: a3d635088a556df4507e65542157f10b41d4f87dce42927b58c3b812f262f4544b6b57f3384eef1097ffdd7c32b8dd1556aae201254960cbfbf48d45551200f7
