💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2326784390)
CI failure looks unrelated and can be ignored: https://github.com/bitcoin/bitcoin/issues/30798

💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#issuecomment-2326785530)
review ACK 6e5254cf687578ebb686628cf89f2509d415f04f

💬 maflcko commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#discussion_r1742252760)
Why not set the parallel tasks, like in the script above?

💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742256231)
I think I understand why this needs to pass BUILD_TYPE=Debug so the debug flags from the depends system will be applied. But I don't understand why FLAGS_DEBUG variables need to be set to empty stings. How does setting those to empty strings cause MSAN_FLAGS to remain unaltered? What code is altering MSAN_FLAGS if FLAGS_DEBUG variables are unset?

💬 fanquake commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742257436)
So given the Build type change, this line, and the equivalent flag in the other CI config, are redundant and should have been removed. However I don't actually think they should be removed, because it's instead CMake that should be fixed, to remove the assumption that builds using the Debug build type, will always have no optimisations, as this isn't actually the case, for exmaple, in oss-fuzz, or doing a depends build with DEBUG=1. See further discussion in https://github.com/google/oss-fuzz/pu
...

⚠️ maflcko opened an issue: "Unify -logsourcelocations format"
(https://github.com/bitcoin/bitcoin/issues/30799)
Now that cmake requires out-of-tree builds, it would be nice to unify the `-logsourcelocations` output format.

Currently, with cmake, it includes the `src`, which seems redundant and doesn't match the guix builds.

Out-of-tree builds were always broken in this regard, but now would probably be a good time to unify it to drop the `src` prefix in `logsourcelocations`, so that it matches: (1) legacy-autotools-in-tree behavior (2) legacy- and cmake-guix build behavior.

Ref:

```
$ ./bld-c
...

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742262723)
Wouldn't
```C++
BOOST_CHECK_EQUAL(from_hex.has_value(), parse_hash_v.has_value());
if (!from_hex.has_value()) continue;
BOOST_CHECK_EQUAL(from_hex.value(), parse_hash_v.value());
```
be simpler after this change as just directly comparing the two?
```C++
BOOST_CHECK_EQUAL(from_hex, parse_hash_v);
```
?

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742265812)
was specifically requested to avoid repetition and since they're not mission-critical code: https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1739923996

💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742268255)
Perhaps I used the word 'unaltered' incorrectly. It would be more accurate to say that "flags set in MSAN_FLAGS won't be overridden". The `CMAKE_CXX_FLAGS_DEBUG` follow `CMAKE_CXX_FLAGS` in the compiler invocation string and competing flags may be overridden.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742269284)
I don't really see the value in doing that for such short-scoped test variables - but you can convince me with good arguments, it just seems like noise to me in these cases

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742272526)
those were testing the same input, this one is testing the parsing of `result->ToString()`

🤔 maflcko reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2245305890)
Submitting my review comments, now that this has been merged.

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1725182221)
d2fda82b4954f4af7e7d340cf42b9cb34d96cde1: 4.2.2 Looks ancient, so maybe it can be dropped already?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1724779168)
nit in 3118e40c6157c8ab9e264518d1065d2b0fc07795: The comment seems wrong? This is an internal lib, neither stable nor "compatible"

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1723106474)
nit in commit https://github.com/bitcoin/bitcoin/commit/4a0af29697b62d32af6f60d3ec70cd2ed4d7243c ("cmake: Add TryAppendCXXFlags module"):

could it make sense to mention that `IF_CHECK_FAILED` is never applied to the linker flags, whereas `IF_CHECK_PASSED` seems to be? (`IF_CHECK_FAILED` is unused dead code, so nothing blocking)

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726684296)
3d853795707c5a1828dcd09c1f68bb07dee472cd: Why does this need special handling for msvc? What problem is this trying to solve?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726761710)
57a6e2ef4abbfd2b12ee6489366bc6609bead263, two questions:

* Looks like Network was added in 4af4672525f698c7b491023fcd36a17a4e982070, but not guarded by the wallet
* Why is this needed by the wallet?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1721657604)
nit in https://github.com/bitcoin/bitcoin/commit/a2317e27b7fb86df4e32cd1674c06e09cb808248: Maybe add a comment that this can be removed after `cmake_minimum_required(VERSION 3.25)`?

Alternatively, given that "The default installation of the latest version of MSVC 17.10.4 includes both CMake 3.28.3 and the vcpkg package manager", it may be easier to just require cmake >= 3.25 on MSVC and enforce the policy this way?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1727632112)
You can't run exe on WSL without wine, can you?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1736110362)
in commit a2317e27b7fb86df4e32cd1674c06e09cb808248: This will only print the compiler name, not the flags.

E.g. `-DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-m32'` will print the flags nowhere. (This is used in the `ci/test/00_setup_env_i686_multiprocess.sh` task)