💬 maflcko commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282116087)
```suggestion
# We need to temporarily replace that in order to keep strings intact.
```

(nit from the llm)

💬 maflcko commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282115116)
Can be dropped, given the global requirement in the root `CMakeLists.txt`?

💬 maflcko commented on pull request "cmake: Do not require Python to build GUI":
(https://github.com/bitcoin/bitcoin/pull/33156#issuecomment-3196255679)
I see. No strong opinion on which direction to take, but this one need rebase, if still relevant.

💬 purpleKarrot commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282132313)
No. This is a script that is executed as a separate process. It does not inherit any settings from the buildsystem generator invocation.

💬 purpleKarrot commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282133378)
Thanks.

💬 maflcko commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282164552)
Just a nit, but I mentioned it, because we don't add it to any other script (cmake, python, bash, ...). They are simply documented in `doc/dependencies.md` and bumping them is done with a minimal patch, not touching all possible scripts that could be called as an entrypoint. Also, this protects against an edge case, that doesn't exist in reality: It requires a developer that uses an ancient cmake to call this entrypoint manually. But just a nit.

💬 maflcko commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3196344455)
It is already possible to run clang tidy on a diff. See `git grep clang-tidy-diff`. However, it is not integrated into CI. In theory it could be integrated, enforcing a stronger set of checks than the "default" ones. Though, that's also going to mess with move-only commits, so I am not sure if it will be useful to have it enabled and lead to failing CI. I'd say there are two options:

* Enable it globally and by default in a "breaking" CI change that may lead to silent or explicit merge confli
...

💬 maflcko commented on pull request "test: cover invalid codesep positions for signature in taproot":
(https://github.com/bitcoin/bitcoin/pull/32301#issuecomment-3196369804)
CI failure:

```
[08:01:28.655] �[0;36m OverflowError: can't convert negative int to unsigned�[0m

💬 purpleKarrot commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282215658)
This also sets the [policy version](https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html#policy-version). No matter what version of CMake you use, it will behave as if you used 3.22.

🤔 janb84 reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128106813)
Concept ACK d3679017c64f336f94339373e563bd4967cd143c

PR moves functionality from python based script to cmake, which imho better aligns with the place from where the script is called (from a build) and it removes an external dependency.

- build ✅
- followed translation instruction that calls the script ✅

💬 janb84 commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#discussion_r2282214659)
Nit, Both define lines are a change in the `#ifdef` style (also from the previous generated file). Personally I do not care which style is followed. Just noting that this is a derivation.

```suggestion
#define UNUSED __attribute__((unused))
```

🤔 hebasto reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128119729)
Concept ACK.

> Translate the `share/qt/extract_strings_qt.py` script to CMake. This removes the python dependency from the `translate` target.

I [support](https://github.com/bitcoin/bitcoin/pull/33156#issue-3303948221) this approach:
> An alternative approach would be to convert the `translate` build target into a CMake script.

🤔 hebasto reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128124464)
CI linter complains:
```
[11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
```

✅ hebasto closed a pull request: "cmake: Do not require Python to build GUI"
(https://github.com/bitcoin/bitcoin/pull/33156)

💬 hebasto commented on pull request "cmake: Do not require Python to build GUI":
(https://github.com/bitcoin/bitcoin/pull/33156#issuecomment-3196425727)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/33209.

💬 purpleKarrot commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#issuecomment-3196426884)
> CI linter complains:
>
> ```
> [11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
> ```

What would you prefer:

a) Set executable bit, or
b) Remove shebang.

💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3196441400)
@Eunovo Nice work on addressing the issues identified in previous tests. All of those are working as expected now.

I am including a patch that adds 2 more failing tests when adding silent-payments address in the transaction output:
self.test_sendrawtransaction()
self.test_mixed_output_types()

[test_sp_receive.patch](https://github.com/user-attachments/files/21834964/test_sp_receive.patch)

💬 hebasto commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#issuecomment-3196449095)
> > CI linter complains:
> > ```
> > [11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
> > ```
>
> What would you prefer:
>
> a) Set executable bit, or
> b) Remove shebang.

(b), for consistency with other CMake scripts in the repository,

📝 brunoerg opened a pull request: "fuzz: enhance wallet_fees by mocking mempool stuff"
(https://github.com/bitcoin/bitcoin/pull/33210)
Some functions in `wallet/fees.cpp` (fuzzed by the wallet_fees target) depends on some mempool stuff - e.g. relay current min fee, smart fee and max blocks estimation, relay dust fee and other ones. For better fuzzing of it, it would be great to have these values/interactions. That said, this PR enhances the `wallet_fees` target by:

- Setting mempool options - `min_relay_feerate`, `dust_relay_feerate` and `incremental_relay_feerate` - when creating the `CTxMemPool`.
- Creates a `ConsumeMemp
...

🤔 janb84 reviewed a pull request: "Release: Prepare "Translation string freeze" step"
(https://github.com/bitcoin/bitcoin/pull/33193#pullrequestreview-3128184088)
ACK d32fcd3df22671b633cae605ed06c6d0a353e2bf

PR updates translation strings, for 30.0 freeze.

Ran the commands against this PR; got 0 changes (reproduced the results) ✅
Ran the commands against Master; got the changes as provided in this PR ✅

🤔 hebasto reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128236494)
It seems no need to track `in_msgstr`:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -34,23 +34,20 @@ function(extract_strings output)
set(messages)
set(msgid)
set(in_msgid OFF)
- set(in_msgstr OFF)

file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
- if(in_msgstr AND NOT msgid STREQUAL "\"\"")
+ set(in_msgid ON)
+ set(msgid "${CMAKE_MATCH_1}")
+
...