💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2327952563)
review ACK dee36a6746f18a3db748ef7bf52bfebddb546307

CI output:

* Before: https://cirrus-ci.com/task/6415951824945152?logs=ci#L1057

```
C++ compiler flags .................... -Wno-error=documentation -O0 -ftrapv -O1 -g3 -g3 -std=c++20 -fPIC -fdebug-prefix-map=/ci_container_base=. -fmacro-prefix-map=/ci_container_base=. -Werror -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wcond
...

💬 hebasto commented on pull request "build: Fix / improve coverage scripts":
(https://github.com/bitcoin/bitcoin/pull/30772#discussion_r1743093952)
Thanks! Added.

💬 jadijadi commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2327990907)
> review ACK [8289373](https://github.com/bitcoin/bitcoin/commit/8289373ebf089d0719a8edecb8bfbd52c244f4ee)
>
> Please adjust the pull request description, now that the approach has changed: [#30788 (comment)](https://github.com/bitcoin/bitcoin/pull/30788#issue-2500949706)

Done. The PR description (https://github.com/bitcoin/bitcoin/pull/30788#issue-2500949706) is updated to reflect the changes in the commit message.

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2327996238)
Please update the pull request description properly. There is no need to include incorrect stuff (`echo success` isn't translated), or outdated stuff of a previous commit. Please focus on the important and correct details only.

👍 hebasto approved a pull request: "test: fixing failing system_tests/run_command under some Locales"
(https://github.com/bitcoin/bitcoin/pull/30788#pullrequestreview-2279109392)
ACK ae48a22a3df086fb59843b7b814619ed5df7557b, tested on Ubuntu 24.04 by switching locale.

https://github.com/bitcoin/bitcoin/pull/30788#issue-2500949706

> the run_command test under system_tests fails if the locale is anything other than US/UK/C (English ones) because it checks for things like `result.find_value("success");`
>
> To prevent this, a `setenv("LC_ALL", "C", 1);` is added to make sure that the tests are being run under C locale even if something else is set in the terminal.
...

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2328028855)
The commit description looks fine, so why not copy-paste it and replace the pull request description?

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2328029615)
review ACK ae48a22a3df086fb59843b7b814619ed5df7557b

💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328044971)
Could include https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742931121 to avoid having to open another pull for that?

📝 hebasto opened a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)
Closes https://github.com/bitcoin/bitcoin/issues/30799:

```
$ ./build/src/bitcoind -logsourcelocations -asmap=/tmp/no_file 2>&1 | head -1
2024-09-04T07:01:03Z [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v28.99.0-b94fe85fbe2f (release build)
```

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743189780)
Fixed in https://github.com/bitcoin/bitcoin/pull/30803.

💬 hebasto commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328087131)
> Could include [#30454 (comment)](https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742931121) to avoid having to open another pull for that?

Sure! Included.

💬 maflcko commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#discussion_r1743191660)
This isn't the only reason. The other is ccache, see commit 7abac98d3e3c1bc8ad66cb5c05184b9c5cc674d5. It would be good to mention this.

Ref: https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-ffile-prefix-map

💬 jadijadi commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2328091025)
> The commit description looks fine, so why not copy-paste it and replace the pull request description?

just did. thanks.

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743195595)
> I'd rather not include Wine installation instructions in this build doc. It is not a trivial process and could create an additional maintenance burden in the future.

Not sure I agree. Can you elaborate on this "burden"? Why is that better to avoid than running the tests?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743199107)
@fanquake I am not sure how useful it is to run the tests in the build environment on Linux on Wine, when the binaries itself are meant to be run on a completely different platform (Windows). It would be better to run the tests on the actual platform that will later run the binaries instead. If the docs don't mention it, they can be adjusted to say so.

💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2328105897)
lgtm ACK cc9d65dc05f802aaaa108a2edcf30c3758c3f459

Let's wait for the outcome of the discussions in https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726684296 and https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1723106474 as well?

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743211010)
> Can you elaborate on this "burden"?

Wine config creation? Need of the `wine-binfmt` package? Peoples request to fix their Wine installations?

💬 hebasto commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#discussion_r1743220926)
Sure! The comment has been extended.

💬 maflcko commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2328142791)
According to https://reproducible-builds.org/docs/build-path/ "-ffile-prefix-map=OLD=NEW is an alias for both -fdebug-prefix-map and -fmacro-prefix-map. (available since GCC 8 and Clang 10)". Ref https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-ffile-prefix-map and https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffile-prefix-map

review ACK 93a0c2da34b45aaf0aab23c5f1b66cb32df06507

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2328178587)
`08c9a8254a...ed7040a9da`: rebase due to conflicts