Bitcoin Core Github
44 subscribers
121K links
Download Telegram
📝 hebasto opened a pull request: "depends: Fix cross-compiling `qt` package from macOS to Windows"
(https://github.com/bitcoin/bitcoin/pull/32357)
Native packages cannot be used during cross-compiling. However, Qt still unconditionally tries to find them, which causes issues in some cases, such as when [cross-compiling from macOS to Windows](https://github.com/bitcoin/bitcoin/issues/32346).

This PR explicitly disables this unnecessary Qt behaviour.

Fixes https://github.com/bitcoin/bitcoin/issues/32346.

Here is a full workflow on my macOS Sequoia 15.4.1 (Intel):
```
% brew install make cmake ninja mingw-w64 nsis
% gmake -C depen
...
💬 hebasto commented on pull request "cmake: Do not override flags set by the user when replacing them":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2832504310)
The commit message has been amended.
💬 hebasto commented on issue "test: different error message fails rpc_signer.py":
(https://github.com/bitcoin/bitcoin/issues/31506#issuecomment-2832568646)
I'm struggling to reproduce the 'execve failed: Not a directory (20)' error message on Ubuntu 24.04:

```
$ bitcoind -daemon -signet -signer=fake.py
Bitcoin Core starting
$ bitcoin-cli -signet enumeratesigners
error code: -1
error message:
execve failed: No such file or directory (2)
```
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2061570932)
Thanks! Took a variant of that, while still maintaining support for shortened forms of `--tmpdir`. The Python `ArgumentParser` supports doing that and I've personally used `--time=2` in place of `--timeout-factor=2` since it's shorter and I don't remember if it's `--timeout_factor`.
💬 hebasto commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2832590273)
Quoting from https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2801696317:

> ... a recommended debugging step is to discard any cached results from an existing build directory...

And in this particular case, I don't think that CMake cache is worth reusing.
💬 hebasto commented on pull request "depends: Fix cross-compiling `qt` package from macOS to Windows":
(https://github.com/bitcoin/bitcoin/pull/32357#issuecomment-2832596900)
My Guix build:
```
aarch64
8ba76371ec4b297da8d29674c7ec31a01eead8131a8e0bbf9bc7e66fb4e1adff guix-build-35e57fbe336c/output/aarch64-linux-gnu/SHA256SUMS.part
d16137ebd3ca5eabb9614a6d4c322e05c43727621d1b8eca98ef4321c5964f21 guix-build-35e57fbe336c/output/aarch64-linux-gnu/bitcoin-35e57fbe336c-aarch64-linux-gnu-debug.tar.gz
3f0078d7df0a54497a52541f2252a52d5b50737b96b83f88d8f2aecd76b54cca guix-build-35e57fbe336c/output/aarch64-linux-gnu/bitcoin-35e57fbe336c-aarch64-linux-gnu.tar.gz
778792dc
...
💬 abdallh12345 commented on pull request "depends: Fix cross-compiling `qt` package from macOS to Windows":
(https://github.com/bitcoin/bitcoin/pull/32357#issuecomment-2832602105)
.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2061611643)
But this doesn't just match prefixes, it makes every parameter optional, so the middle one can be missing - which would be weird.

Maybe you meant `--tm(?:p(?:d(?:ir?)?)?)?=` which would only match the prefixes
* `--tmpdir=`
* `--tmpdi=`
* `--tmpd=`
* `--tmp=`
* `--tm=`

but the intermediary letters aren't optional, so
* `--tmr=`
won't be matched (and the groups are non-capturing so you can still use `m[1]` for the value)
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2832616772)
ACK b19439b1eee57041fc37dcf509d5ee9f248263bc
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2061620107)
Switched to use `os.linesep` which I think is more idiomatic... But I'm not entirely happy with how I ended up having to escape:
https://github.com/bitcoin/bitcoin/blob/b19439b1eee57041fc37dcf509d5ee9f248263bc/test/functional/feature_framework_startup_failures.py#L76-L79

It's a side-effect of using `repr(e)` in 28e282ef9ae94ede4aace6b97ff18c66cb72a001 which gives the escaped form.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2061626467)
Thanks for catching that. I would have leaned towards `--tm(p(d(i(r?)?)?)?)?=` but I like the non-capturing groups, don't recall coming across them before. Taken.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2832642819)
ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c
👍 hodlinator approved a pull request: "test: avoid stack overflow in `FindChallenges` via manual iteration"
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2796453342)
ACK 2e533cf86a42b3416c1164d04089e80ffe622dd6

Prefer this alternative of switching to an iterative algorithm (instead of bumping the stack space #32349).

On master we do a depth-first gathering of challenges, the new stack based version does breadth-first, but result is put in a `std::set` so traversal order does not matter.
💬 hodlinator commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#discussion_r2061627575)
In bd89f7eff0dc6c44c6a0b96883c3816fe0919f95, why not just:
```C++
BOOST_CHECK(challenges_recursive == challenges);
```
?
https://en.cppreference.com/w/cpp/container/set/operator_cmp claims the complexity is linear with the size.
💬 hodlinator commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2832650664)
Was curious how this issue was introduced. Seems like it's existed for some time but was found by #32339 momentarily switching some Windows CI to use Debug configs. Would it be worth adding to the description instead of just "ref: #32339"?
💬 hebasto commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2832694789)
Concept ACK.
💬 maflcko commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2833289993)
The erroneous format specs still remain: https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827238571
💬 maflcko commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2833334943)
The error seems to be intermittent, because now it looks like the centos task passed. (For reference, the failure three days ago was https://github.com/bitcoin/bitcoin/runs/41112451543)
👍 hebasto approved a pull request: "test: avoid stack overflow in `FindChallenges` via manual iteration"
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2797575186)
ACK 2e533cf86a42b3416c1164d04089e80ffe622dd6, I have reviewed the code and it looks OK.

If we put refactoring aside, the changes in this PR could be much smaller and easier to review with `git diff --ignore-all-space`:
```diff
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -297,9 +297,13 @@ using miniscript::operator""_mst;
using Node = miniscript::Node<CPubKey>;

/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
...