💬 maflcko commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556799506)
lgtm ACK d38ade7bc409207bf425e05ee10b633b0aef4c36
In theory, the shebang could be removed from the mocks files, to better enforce this, but this could make one of the linters sad, so could be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556799506)
lgtm ACK d38ade7bc409207bf425e05ee10b633b0aef4c36
In theory, the shebang could be removed from the mocks files, to better enforce this, but this could make one of the linters sad, so could be done in a follow-up.
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820716)
I want to keep the changes minimal. Happy to review a follow-up doing this, but I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820716)
I want to keep the changes minimal. Happy to review a follow-up doing this, but I'll leave this as-is for now.
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820757)
> we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
Where? Functional-style casts are recommended by the dev notes, so if you want to change that, it would be good to change the dev notes first.
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820757)
> we're getting `Functional-style cast is used instead of a C++ cast` warning now, what if we used character literals instead:
Where? Functional-style casts are recommended by the dev notes, so if you want to change that, it would be good to change the dev notes first.
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820831)
Happy to review a follow-up doing this, but I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820831)
Happy to review a follow-up doing this, but I'll leave this as-is for now.
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820913)
> does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
Yes, the purpose is to follow the clang-format sorting. If it was changed, clang-format will undo the change the next time.
> maybe even adding a 0 prefix for <10
Happy to review a follow-up doing this, but I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893820913)
> does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
Yes, the purpose is to follow the clang-format sorting. If it was changed, clang-format will undo the change the next time.
> maybe even adding a 0 prefix for <10
Happy to review a follow-up doing this, but I'll leave this as-is for now.
💬 hebasto commented on pull request "guix: latest 2.31 glibc":
(https://github.com/bitcoin/bitcoin/pull/31529#issuecomment-2556813270)
My Guix build:
```
aarch64
8f50480a91024a77c45233e33b8151e213a08834c77493446191a6bc78d82a1f guix-build-b8710201fbd0/output/aarch64-linux-gnu/SHA256SUMS.part
6fd527ebfa0842cd3f3c743dc616d75cdcd0aea637d04e4e047f29e87c43d383 guix-build-b8710201fbd0/output/aarch64-linux-gnu/bitcoin-b8710201fbd0-aarch64-linux-gnu-debug.tar.gz
a40ec71782cc8018ffb9b8b73d99c55aece77c5779e752f1b4209894efd3c97b guix-build-b8710201fbd0/output/aarch64-linux-gnu/bitcoin-b8710201fbd0-aarch64-linux-gnu.tar.gz
74fb98d9
...
(https://github.com/bitcoin/bitcoin/pull/31529#issuecomment-2556813270)
My Guix build:
```
aarch64
8f50480a91024a77c45233e33b8151e213a08834c77493446191a6bc78d82a1f guix-build-b8710201fbd0/output/aarch64-linux-gnu/SHA256SUMS.part
6fd527ebfa0842cd3f3c743dc616d75cdcd0aea637d04e4e047f29e87c43d383 guix-build-b8710201fbd0/output/aarch64-linux-gnu/bitcoin-b8710201fbd0-aarch64-linux-gnu-debug.tar.gz
a40ec71782cc8018ffb9b8b73d99c55aece77c5779e752f1b4209894efd3c97b guix-build-b8710201fbd0/output/aarch64-linux-gnu/bitcoin-b8710201fbd0-aarch64-linux-gnu.tar.gz
74fb98d9
...
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893823673)
It requires the caller to define it, which is intentional. Should I add a comment explaining it?
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893823673)
It requires the caller to define it, which is intentional. Should I add a comment explaining it?
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893825061)
Not sure what you mean, for me it is already expanded/evaluated, see https://github.com/bitcoin/bitcoin/pull/31428/checks#step:6:11
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893825061)
Not sure what you mean, for me it is already expanded/evaluated, see https://github.com/bitcoin/bitcoin/pull/31428/checks#step:6:11
👍 hebasto approved a pull request: "guix: latest 2.31 glibc"
(https://github.com/bitcoin/bitcoin/pull/31529#pullrequestreview-2517356688)
ACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af.
(https://github.com/bitcoin/bitcoin/pull/31529#pullrequestreview-2517356688)
ACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893827538)
The following works on FreeBSD. In case `dtrace` works on macOS:
```sh
dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c 'bitcoin -m gui'
```
Maybe how to enable `dtrace` on macOS: https://github.com/suolapeikko/dtrace?tab=readme-ov-file#sample-dtrace-scripts-for-macos
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893827538)
The following works on FreeBSD. In case `dtrace` works on macOS:
```sh
dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c 'bitcoin -m gui'
```
Maybe how to enable `dtrace` on macOS: https://github.com/suolapeikko/dtrace?tab=readme-ov-file#sample-dtrace-scripts-for-macos
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893834634)
I mean, expanded in a log for the current step, like [this](https://github.com/hebasto/bitcoin/actions/runs/12430191280/job/34705149833#step:3:2):
```
Run echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "BASE_ROOT_DIR=/home/runner/work/_temp" >> "$GITHUB_ENV"
echo "DEPENDS_DIR=/home/runner/work/_temp/depends" >> "$GITHUB_ENV"
echo "BASE_BUILD_DIR=/home/runner/work/_temp/build" >> "$GITH
...
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893834634)
I mean, expanded in a log for the current step, like [this](https://github.com/hebasto/bitcoin/actions/runs/12430191280/job/34705149833#step:3:2):
```
Run echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
echo "BASE_ROOT_DIR=/home/runner/work/_temp" >> "$GITHUB_ENV"
echo "DEPENDS_DIR=/home/runner/work/_temp/depends" >> "$GITHUB_ENV"
echo "BASE_BUILD_DIR=/home/runner/work/_temp/build" >> "$GITH
...
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893837484)
> It requires the caller to define it, which is intentional.
Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
> Should I add a comment explaining it?
If it helps :)
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893837484)
> It requires the caller to define it, which is intentional.
Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
> Should I add a comment explaining it?
If it helps :)
💬 maflcko commented on pull request "fuzz: Faster leak check":
(https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1893840379)
Also, the message is hidden anyway by default in the fuzz runner, unless you specified `-l DEBUG`.
(https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1893840379)
Also, the message is hidden anyway by default in the fuzz runner, unless you specified `-l DEBUG`.
💬 maflcko commented on pull request "fuzz: Faster leak check":
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556840485)
> ### Performance increase
>
> Changed `-max_total_time=60` beneath the line this PR is modifying and ran with & without `-detect_leaks=0`. The performance improvement is VERY stark. The number of fuzz inputs generated where either around 67 (2 tests) without, and with, it was a staggering 1937 (average from 2 runs). **Around 28x faster.**
Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long run
...
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556840485)
> ### Performance increase
>
> Changed `-max_total_time=60` beneath the line this PR is modifying and ran with & without `-detect_leaks=0`. The performance improvement is VERY stark. The number of fuzz inputs generated where either around 67 (2 tests) without, and with, it was a staggering 1937 (average from 2 runs). **Around 28x faster.**
Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long run
...
💬 maflcko commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893844905)
> > It requires the caller to define it, which is intentional.
>
> Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
My understanding is that it would fail otherwise. I suspect the error message will be:
```
root@7b4d00222340:/# mkdir -p "${BASE_BUILD_DIR}"
mkdir: cannot create directory '': No such file or directory
root@7b4d00222340:/# mkdir -p ""
mkdir: cannot create directory '': No such file or directory
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893844905)
> > It requires the caller to define it, which is intentional.
>
> Why? What happens if the default value will be used? (especially, if a CI job doesn't care about it)
My understanding is that it would fail otherwise. I suspect the error message will be:
```
root@7b4d00222340:/# mkdir -p "${BASE_BUILD_DIR}"
mkdir: cannot create directory '': No such file or directory
root@7b4d00222340:/# mkdir -p ""
mkdir: cannot create directory '': No such file or directory
📝 hebasto converted_to_draft a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176)
This PR enables on the CI tests of cross-compiled Windows binaries on Windows.
It is important to have such tests in CI because the release binaries for Windows are also cross-compiled.
Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as https://github.com/bitcoin/bitcoin/pull/31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs.
Resolves https://github.com
...
(https://github.com/bitcoin/bitcoin/pull/31176)
This PR enables on the CI tests of cross-compiled Windows binaries on Windows.
It is important to have such tests in CI because the release binaries for Windows are also cross-compiled.
Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as https://github.com/bitcoin/bitcoin/pull/31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs.
Resolves https://github.com
...
💬 hebasto commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556851076)
Friendly ping Python connoisseur @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2556851076)
Friendly ping Python connoisseur @stickies-v :)
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2556857324)
Friendly ping a Python connoisseur @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2556857324)
Friendly ping a Python connoisseur @stickies-v :)
💬 hebasto commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893852851)
Right. That's why I suggest to move `BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build-$HOST}` before.
(https://github.com/bitcoin/bitcoin/pull/31428#discussion_r1893852851)
Right. That's why I suggest to move `BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build-$HOST}` before.
💬 fanquake commented on issue "depends: `capnp` package fails to build on NetBSD 10.0":
(https://github.com/bitcoin/bitcoin/issues/31499#issuecomment-2556860449)
Nothing for us to do here, so could be reopened/linked from a PR once a proper issue is filed upstream.
(https://github.com/bitcoin/bitcoin/issues/31499#issuecomment-2556860449)
Nothing for us to do here, so could be reopened/linked from a PR once a proper issue is filed upstream.