cmake: Drop python dependency for translate

purpleKarrot commented at 11:12 am on August 18, 2025: contributor

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

Resolves #33146

DrahtBot added the label Build system on Aug 18, 2025

DrahtBot commented at 11:12 am on August 18, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33209.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, janb84
Stale ACK	achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in share/qt/translate.cmake:5 in b98729d564 outdated

1@@ -2,12 +2,13 @@
2 # Distributed under the MIT software license, see the accompanying
3 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+cmake_minimum_required(VERSION 3.22)

maflcko commented at 11:21 am on August 18, 2025:

Can be dropped, given the global requirement in the root CMakeLists.txt?

purpleKarrot commented at 11:29 am on August 18, 2025:

No. This is a script that is executed as a separate process. It does not inherit any settings from the buildsystem generator invocation.

I added a shebang to clarify that this may be used as an entrypoint.

maflcko commented at 11:44 am on August 18, 2025:

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.

purpleKarrot commented at 12:09 pm on August 18, 2025:

This also sets the policy version. No matter what version of CMake you use, it will behave as if you used 3.22.

in share/qt/translate.cmake:44 in b98729d564 outdated

39+  file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
40+  foreach(line IN LISTS text)
41+    if(line MATCHES "^msgid (.*)$")
42+      if(in_msgstr AND NOT msgid STREQUAL "\"\"")
43+        # CMake uses ';' as a list separator.
44+        # We need to temporarily replace that in orde to keep strings intact.

maflcko commented at 11:22 am on August 18, 2025:

0        # We need to temporarily replace that in order to keep strings intact.

(nit from the llm)

purpleKarrot commented at 11:30 am on August 18, 2025:

Thanks.

purpleKarrot force-pushed on Aug 18, 2025

in share/qt/translate.cmake:72 in d3679017c6 outdated

67+  set(content [[// Automatically generated by translate.cmake
68+
69+#include <QtGlobal>
70+
71+#ifdef __GNUC__
72+#  define UNUSED __attribute__((unused))

janb84 commented at 12:09 pm on August 18, 2025:

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.

0#define UNUSED __attribute__((unused))

janb84 commented at 12:12 pm on August 18, 2025: contributor

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 ✅

hebasto commented at 12:12 pm on August 18, 2025: member

Concept ACK.

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

I support this PR approach:

An alternative approach would be to convert the translate build target into a CMake script.

hebasto commented at 12:13 pm on August 18, 2025: member

CI linter complains:

0[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).

purpleKarrot commented at 12:15 pm on August 18, 2025: contributor

CI linter complains:

0[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.

hebasto added this to the milestone 30.0 on Aug 18, 2025

hebasto commented at 12:18 pm on August 18, 2025: member

CI linter complains:

0[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,

purpleKarrot force-pushed on Aug 18, 2025

DrahtBot added the label CI failed on Aug 18, 2025

DrahtBot commented at 12:27 pm on August 18, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48299489270 LLM reason (✨ experimental): The CI failure is caused by incorrect file permissions on a script with a shebang line.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hebasto commented at 12:47 pm on August 18, 2025: member

It seems no need to track in_msgstr:

 0--- a/share/qt/translate.cmake
 1+++ b/share/qt/translate.cmake
 2@@ -34,23 +34,20 @@ function(extract_strings output)
 3   set(messages)
 4   set(msgid)
 5   set(in_msgid OFF)
 6-  set(in_msgstr OFF)
 7 
 8   file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
 9   foreach(line IN LISTS text)
10     if(line MATCHES "^msgid (.*)$")
11-      if(in_msgstr AND NOT msgid STREQUAL "\"\"")
12+      set(in_msgid ON)
13+      set(msgid "${CMAKE_MATCH_1}")
14+    elseif(line MATCHES "^msgstr (.*)$")
15+      set(in_msgid OFF)
16+      if(NOT msgid STREQUAL "\"\"")
17         # CMake uses ';' as a list separator.
18         # We need to temporarily replace that in order to keep strings intact.
19         string(REPLACE ";" "<cmake-semicolon>" msgid "${msgid}")
20         list(APPEND messages "${msgid}")
21       endif()
22-      set(in_msgid ON)
23-      set(in_msgstr OFF)
24-      set(msgid "${CMAKE_MATCH_1}")
25-    elseif(line MATCHES "^msgstr (.*)$")
26-      set(in_msgid OFF)
27-      set(in_msgstr ON)
28     elseif(line MATCHES "^\"")
29       if(in_msgid)
30         string(APPEND msgid "\n${line}")
31@@ -58,11 +55,6 @@ function(extract_strings output)
32     endif()
33   endforeach()
34 
35-  if(in_msgstr AND NOT msgid STREQUAL "\"\"")
36-    string(REPLACE ";" "<cmake-semicolon>" msgid "${msgid}")
37-    list(APPEND messages "${msgid}")
38-  endif()
39-
40   set(content [[// Automatically generated by translate.cmake
41 
42 #include <QtGlobal>

purpleKarrot force-pushed on Aug 18, 2025

hebasto approved

hebasto commented at 2:37 pm on August 18, 2025: member

ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86.

Another line can be saved:

 0--- a/share/qt/translate.cmake
 1+++ b/share/qt/translate.cmake
 2@@ -33,7 +33,7 @@ function(extract_strings output)
 3 
 4   file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
 5 
 6-  set(messages)
 7+  set(messages "\"${COPYRIGHT_HOLDERS}\"")
 8   foreach(line IN LISTS text)
 9     if(line MATCHES "^msgid (.*)$")
10       set(msgid "${CMAKE_MATCH_1}")
11@@ -63,8 +63,6 @@ static const char UNUSED *bitcoin_strings[] = {
12   set(prefix "QT_TRANSLATE_NOOP(\"bitcoin-core\", ")
13   set(suffix "),\n")
14 
15-  string(APPEND content "${prefix}\"${COPYRIGHT_HOLDERS}\"${suffix}")
16-
17   list(SORT messages)
18   list(JOIN messages "${suffix}${prefix}" messages_str)
19   string(APPEND content "${prefix}${messages_str}${suffix}")

DrahtBot requested review from janb84 on Aug 18, 2025

janb84 commented at 2:52 pm on August 18, 2025: contributor

re ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86

changes since last ACK:

dropped shebang
script optimizations
ifdef style change (thanks for including my nit)

DrahtBot removed the label CI failed on Aug 18, 2025

purpleKarrot force-pushed on Aug 18, 2025

purpleKarrot commented at 4:44 pm on August 18, 2025: contributor

Changes:

xgettext is instructed to sort the output by file
the messages are no longer sorted by cmake
multiline strings are joined

It was tested on both arch and ubuntu 24.04 to produce the same output (Older versions of xgettext split the strings at different positions, joining the multiline strings results in the same output).

achow101 commented at 10:27 pm on August 18, 2025: member

ACK d395abac85a2acf0717859f05ccfccf8a3c71a46

DrahtBot requested review from janb84 on Aug 18, 2025

DrahtBot requested review from hebasto on Aug 18, 2025

hebasto commented at 7:37 am on August 19, 2025: member

I have reviewed d395abac85a2acf0717859f05ccfccf8a3c71a46.

Changes:

xgettext is instructed to sort the output by file

the messages are no longer sorted by cmake

Could we revert these changes? All strings in src/qt/bitcoinstrings.cpp share the same context, namely “bitcoin-core”. Keeping the content stable by sorting within the context feels more natural. Otherwise, the string order depends on the order in which source files are processed by xgettext.

multiline strings are joined

It was tested on both arch and ubuntu 24.04 to produce the same output (Older versions of xgettext split the strings at different positions, joining the multiline strings results in the same output).

This is neat. It avoids reproducibility issues such as #33152#pullrequestreview-3101116986.

DrahtBot requested review from hebasto on Aug 19, 2025

hebasto commented at 7:54 am on August 19, 2025: member

@purpleKarrot

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

Mind updating the PR description with something like “Resolves #33146”?

purpleKarrot commented at 7:54 am on August 19, 2025: contributor

Otherwise, the string order depends on the order in which source files are processed by xgettext. @hebasto, I just created two files a.c and b.c with some gettext content and compared the outputs of xgettext a.c b.c and xgettext b.c a.c both with --sort-by-file and without. I can confirm that without the --sort-by-file option the order of processing changes the output, but it is stable when that option is used.

cmake: Drop python dependency for translate

Resolves #33146

3c4a109aa8

purpleKarrot force-pushed on Aug 19, 2025

hebasto commented at 8:06 am on August 19, 2025: member

Otherwise, the string order depends on the order in which source files are processed by xgettext.

@hebasto, I just created two files a.c and b.c with some gettext content and compared the outputs of xgettext a.c b.c and xgettext b.c a.c both with --sort-by-file and without. I can confirm that without the --sort-by-file option the order of processing changes the output, but it is stable when that option is used.

Right my assumption was not correct.

However, there is another drawback to using the --sort-by-file option. The resulting order depends on the actual file names, so renaming a source file will cause unnecessary churn.

hebasto approved

hebasto commented at 8:11 am on August 19, 2025: member

ACK 3c4a109aa821cbf1e46a67275b4f456673ed13d8.

DrahtBot requested review from achow101 on Aug 19, 2025

hebasto commented at 8:22 am on August 19, 2025: member

multiline strings are joined

~~FWIW, the same effect can be achieved with the --no-wrap option. However, I haven’t tested this with older versions of xgettext.~~

EDIT: Apparently, its behaviour is a bit complicated.

purpleKarrot force-pushed on Aug 19, 2025

purpleKarrot commented at 8:42 am on August 19, 2025: contributor

FWIW, the same effect can be achieved with the --no-wrap option. However, I haven’t tested this with older versions of xgettext.

I have tested it. It simplifies the loop a lot.

purpleKarrot force-pushed on Aug 19, 2025

hebasto approved

hebasto commented at 8:56 am on August 19, 2025: member

re-ACK 3c4a109aa821cbf1e46a67275b4f456673ed13d8.

purpleKarrot commented at 8:57 am on August 19, 2025: contributor

Apparently, its behaviour is a bit complicated.

For reference, the problem is that strings that are wrapped in C++ are not joined with --no-wrap.

janb84 commented at 11:09 am on August 19, 2025: contributor

re ACK 3c4a109aa821cbf1e46a67275b4f456673ed13d8

changes since last ACK:

multilines are now joint
changes in sorting

Manually checked diff of bitcoinstrings.cpp because of change of sorting, all the lines are included. This PR introduces 2 new lines in bitcoinstrings.cpp:

0 QT_TRANSLATE_NOOP("bitcoin-core", "The %s path uses exFAT, which is known to have intermittent corruption problems on macOS. Move this directory to a different filesystem to avoid data loss."),
1 QT_TRANSLATE_NOOP("bitcoin-core", "default wallet"),

hebasto commented at 11:11 am on August 19, 2025: member

Manually checked diff of bitcoinstrings.cpp because of change of sorting, all the lines are included. This PR introduces 2 new lines in bitcoinstrings.cpp:
0 QT_TRANSLATE_NOOP("bitcoin-core", "The %s path uses exFAT, which is known to have intermittent corruption problems on macOS. Move this directory to a different filesystem to avoid data loss."),
1 QT_TRANSLATE_NOOP("bitcoin-core", "default wallet"),

This is expected. See: #33193.

fanquake merged this on Aug 19, 2025

fanquake closed this on Aug 19, 2025

cmake: Drop python dependency for translate #33209

Code Coverage & Benchmarks

Reviews