💬 hebasto commented on issue "build: status code 503 for freetype":
(https://github.com/bitcoin/bitcoin/issues/32085#issuecomment-2729316596)
See: https://github.com/Microsoft/vcpkg/issues/44429
(https://github.com/bitcoin/bitcoin/issues/32085#issuecomment-2729316596)
See: https://github.com/Microsoft/vcpkg/issues/44429
💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2729322992)
Thank you for the review and checking the binary!
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2729322992)
Thank you for the review and checking the binary!
📝 Sjors opened a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086)
macOS ships with GNU Make 3.81 from 2006. This has caused
difficult to debug issues, e.g. #32070 and #30978.
Tell users / developers who use the depends system to install a modern version of `make`.
Although Homebrew allows overriding the system `make`, we instead just instruct users to build with `gmake`. This way there should be no impact on other projects they wish to compile.
To increase the likeliness of anyone actually seeing and following this instruction, the first commit moves
...
(https://github.com/bitcoin/bitcoin/pull/32086)
macOS ships with GNU Make 3.81 from 2006. This has caused
difficult to debug issues, e.g. #32070 and #30978.
Tell users / developers who use the depends system to install a modern version of `make`.
Although Homebrew allows overriding the system `make`, we instead just instruct users to build with `gmake`. This way there should be no impact on other projects they wish to compile.
To increase the likeliness of anyone actually seeing and following this instruction, the first commit moves
...
💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729339929)
Proposed in #32086.
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729339929)
Proposed in #32086.
💬 Sjors commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2729341917)
"fixing" this with #32086...
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2729341917)
"fixing" this with #32086...
🤔 janb84 reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2690222695)
While re-testing this PR, I found that adding the `-g` flag (debug symbols) impacts code coverage reports. With `-g`, more source lines are mapped, lowering the coverage percentage by revealing untested code that’s hidden without it. I tested this with and without `-DCMAKE_BUILD_TYPE=Debug`, and both setups give a different result.
I agree with the rest of the PR changes. But for the most accurate coverage of the source code as written, I recommend sticking with `Debug` and adding `-g`.
(I
...
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2690222695)
While re-testing this PR, I found that adding the `-g` flag (debug symbols) impacts code coverage reports. With `-g`, more source lines are mapped, lowering the coverage percentage by revealing untested code that’s hidden without it. I tested this with and without `-DCMAKE_BUILD_TYPE=Debug`, and both setups give a different result.
I agree with the rest of the PR changes. But for the most accurate coverage of the source code as written, I recommend sticking with `Debug` and adding `-g`.
(I
...
💬 janb84 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1998592459)
```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
```
NIT: In Debug builds, the compiler usually disables optimizations, but without explicit -g, some optimizations might still occur. Given that the code coverage results are indeed different when running without `-g` and with `-g`, I would say some optimization still occurs interfering with the code coverage results.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1998592459)
```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
```
NIT: In Debug builds, the compiler usually disables optimizations, but without explicit -g, some optimizations might still occur. Given that the code coverage results are indeed different when running without `-g` and with `-g`, I would say some optimization still occurs interfering with the code coverage results.
💬 brunoerg commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729352010)
> "Changing the preset to avoid the other sanitizers seems to solve the issue."
It would be good to understand why it doesn't work anymore first.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729352010)
> "Changing the preset to avoid the other sanitizers seems to solve the issue."
It would be good to understand why it doesn't work anymore first.
💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1998654566)
> For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
Are you certain? I checked this and I couldn't verify this claim. If the redeem script is not available in the provider, `provider.GetCScript(scriptid, subscript)` should evaluate to `false` and the expected `addr()` desc will be returned. I tested this by adding the following test
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1998654566)
> For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
Are you certain? I checked this and I couldn't verify this claim. If the redeem script is not available in the provider, `provider.GetCScript(scriptid, subscript)` should evaluate to `false` and the expected `addr()` desc will be returned. I tested this by adding the following test
...
👍 rkrux approved a pull request: "test: avoid disk space warning for non-regtest"
(https://github.com/bitcoin/bitcoin/pull/32057#pullrequestreview-2690371123)
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
The last statement in the PR description seems to be contradicting with the title & the code comment.
> This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
(https://github.com/bitcoin/bitcoin/pull/32057#pullrequestreview-2690371123)
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
The last statement in the PR description seems to be contradicting with the title & the code comment.
> This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2729392547)
Rebased it against latest [master](https://github.com/bitcoin/bitcoin/commit/db2c57ae9eebdb75c58cd165ac929919969c19a9), ran all tests locally, searched for remaining `\bSpan\b` and compared to previously acked fab51b26d68b37f5914314ad29eeb930538ea7ae the only diff was @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1980199782).
<details>
<summary>Details</summary>
```diff
---
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
--- a/src/test/uti
...
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2729392547)
Rebased it against latest [master](https://github.com/bitcoin/bitcoin/commit/db2c57ae9eebdb75c58cd165ac929919969c19a9), ran all tests locally, searched for remaining `\bSpan\b` and compared to previously acked fab51b26d68b37f5914314ad29eeb930538ea7ae the only diff was @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1980199782).
<details>
<summary>Details</summary>
```diff
---
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
--- a/src/test/uti
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998673375)
> @hebasto should we unset `CMAKE_RUNTIME_OUTPUT_DIRECTORY` here to avoid `mpgen` and `mpgen` landing in the `bin/` directory? Maybe it would be better to move the `libmultiprocess` subtree up one level?
I like the idea of moving the add_subdirectory call up a level so to avoid this problem so will experiment with this.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998673375)
> @hebasto should we unset `CMAKE_RUNTIME_OUTPUT_DIRECTORY` here to avoid `mpgen` and `mpgen` landing in the `bin/` directory? Maybe it would be better to move the `libmultiprocess` subtree up one level?
I like the idea of moving the add_subdirectory call up a level so to avoid this problem so will experiment with this.
💬 Sjors commented on pull request "test: avoid disk space warning for non-regtest":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2729404900)
@rkrux updated PR description
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2729404900)
@rkrux updated PR description
🤔 hebasto reviewed a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2690444929)
Approach ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb.
The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.
For more details, see:
- https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2585683053
- https://github.com/hebasto/bitcoin-core-nightly/pull/30
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2690444929)
Approach ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb.
The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.
For more details, see:
- https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2585683053
- https://github.com/hebasto/bitcoin-core-nightly/pull/30
✅ l0rinc closed a pull request: "doc: use `libfuzzer-nosan` for macOS"
(https://github.com/bitcoin/bitcoin/pull/32084)
(https://github.com/bitcoin/bitcoin/pull/32084)
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729481654)
Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results.
Clearing all build caches however fixed it for master as well:
```bash
ccache -C && git clean -fxd && git reset --hard
```
Guess we can't rely on cache invalidation here.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729481654)
Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results.
Clearing all build caches however fixed it for master as well:
```bash
ccache -C && git clean -fxd && git reset --hard
```
Guess we can't rely on cache invalidation here.
💬 hebasto commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2729483431)
> Tell users / developers who use the depends system to install a modern version of `make`.
Concept ACK on that.
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2729483431)
> Tell users / developers who use the depends system to install a modern version of `make`.
Concept ACK on that.
💬 rkrux commented on pull request "docs: remove CI passing requirement for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998727102)
IMO, this section should be kept by rewording it a bit as suggested in this comment https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998121846. Though it is common sense now to ensure a passing CI, there could be new contributors who might not be aware of it and this section is something that can be shared with them if needed. Also, I've seen few instances of CI (beyond this repo) where the CI checks are not required to be passing for the PR to be merged, having this section would set
...
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998727102)
IMO, this section should be kept by rewording it a bit as suggested in this comment https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998121846. Though it is common sense now to ensure a passing CI, there could be new contributors who might not be aware of it and this section is something that can be shared with them if needed. Also, I've seen few instances of CI (beyond this repo) where the CI checks are not required to be passing for the PR to be merged, having this section would set
...
💬 hebasto commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729530235)
> Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results. Clearing all build caches however fixed it for master as well:
>
> ```shell
> ccache -C && git clean -fxd && git reset --hard
> ```
>
> Guess we can't rely on cache invalidation here.
I'd appreciate it if you could provide steps to reproduce the scenario where Ccache cache causes a build issue.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729530235)
> Thanks for the hint @brunoerg, I did a bisect to see if it's the commits or the update, which gave me contradictory results. Clearing all build caches however fixed it for master as well:
>
> ```shell
> ccache -C && git clean -fxd && git reset --hard
> ```
>
> Guess we can't rely on cache invalidation here.
I'd appreciate it if you could provide steps to reproduce the scenario where Ccache cache causes a build issue.
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729550053)
I can't, I deleted everything as the above steps show.
But it was likely the local build folder and not ccache (based on how git bisect behaved, i.e. after the first build passed, everything did).
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729550053)
I can't, I deleted everything as the above steps show.
But it was likely the local build folder and not ccache (based on how git bisect behaved, i.e. after the first build passed, everything did).