cmake: Introduce translate.cmake script for translate target #33200

pull purpleKarrot wants to merge 2 commits into bitcoin:master from purpleKarrot:250815-tr-headers changing 2 files +90 −58
  1. purpleKarrot commented at 4:12 pm on August 15, 2025: contributor

    Using file(GLOB) in the generates step is discouraged because the globbing result may be out of date when the target is built. Performing the globbing in a script that is executed as the build target means the result is always reproducable and the overhead of globbing is only paid when used.

    As a follow up, the dependency on sed may be removed by performing the replacement with cmake. Also, the logic from extract_strings_qt.py can be migrated to cmake.

  2. DrahtBot added the label Build system on Aug 15, 2025
  3. DrahtBot commented at 4:12 pm on August 15, 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/33200.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto
    Concept ACK TheCharlatan

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33193 (Release: Prepare “Translation string freeze” step by hebasto)
    • #33156 (cmake: Do not require Python to build GUI by hebasto)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. hebasto commented at 4:15 pm on August 15, 2025: member

    Concept ACK.

    Thank you!

  5. purpleKarrot force-pushed on Aug 15, 2025
  6. TheCharlatan commented at 4:23 pm on August 15, 2025: contributor
    Concept ACK
  7. in share/qt/translate.cmake:1 in ee2c5bc81c outdated
    0@@ -0,0 +1,82 @@
    1+# Copyright (c) 2012-2025 The Bitcoin Core developers
    


    hebasto commented at 11:14 am on August 16, 2025:
    nit: Why this year range? This script seems like brand-new code to me.
  8. in share/qt/translate.cmake:12 in ee2c5bc81c outdated
     7+  COPYRIGHT_HOLDERS
     8+  LCONVERT_EXECUTABLE
     9+  LUPDATE_EXECUTABLE
    10+  PYTHON_EXECUTABLE
    11+  XGETTEXT_EXECUTABLE
    12+  )
    


    hebasto commented at 11:16 am on August 16, 2025:

    style-nit: Everywhere else, closing parentheses are aligned with the beginning of the command:

    0)
    

    purpleKarrot commented at 7:08 am on August 18, 2025:
    Define Everywhere? Indenting the closing paren is the original CMake style! It probably dates back to when CMake was using Whitesmiths indentation style for the C++ codebase. I am OK with dedenting the closing paren if that is preferred here.

    hebasto commented at 8:44 am on August 18, 2025:

    Define Everywhere?

    This project codebase.


    hebasto commented at 8:46 am on August 18, 2025:
    I’m not arguing which style is better, but asking for the sake of consistency.
  9. in share/qt/translate.cmake:45 in ee2c5bc81c outdated
    40+    EXCLUDE REGEX "${PROJECT_SOURCE_DIR}/src/${directory}/.*"
    41+    )
    42+endforeach()
    43+
    44+set(ENV{XGETTEXT} "${XGETTEXT_EXECUTABLE}")
    45+set(ENV{COPYRIGHT_HOLDERS} "${COPYRIGHT_HOLDERS}")
    


    hebasto commented at 11:20 am on August 16, 2025:
    The scope of this environment variables could be limited to extract_strings_qt.py only.
  10. in share/qt/translate.cmake:62 in ee2c5bc81c outdated
    57+    -I ${PROJECT_SOURCE_DIR}/src
    58+    -locations relative ${PROJECT_SOURCE_DIR}/src/qt/bitcoinstrings.cpp
    59+    ${ui_files}
    60+    ${qt_translatable_sources}
    61+    -ts ${PROJECT_SOURCE_DIR}/src/qt/locale/bitcoin_en.ts
    62+  WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/src
    


    hebasto commented at 11:21 am on August 16, 2025:
    Is the WORKING_DIRECTORY option needed here and below?
  11. hebasto approved
  12. hebasto commented at 11:22 am on August 16, 2025: member

    ACK ee2c5bc81c64d68057278f951c37c8b52e21ff92, I have reviewed the code and tested it on Ubuntu 24.04. The output files match those in #33194.

    This PR effectively fixes the same issue as #33194 and is superior to it.

  13. DrahtBot requested review from TheCharlatan on Aug 16, 2025
  14. hebasto added this to the milestone 30.0 on Aug 16, 2025
  15. purpleKarrot force-pushed on Aug 18, 2025
  16. cmake: Introduce translate.cmake script for translate target
    Using `file(GLOB)` in the generates step is discouraged because the
    globbing result may be out of date when the target is built.
    Performing the globbing in a script that is executed as the build
    target means the result is always reproducable and the overhead
    of globbing is only paid when used.
    
    As a follow up, the dependency on `sed` may be removed by performing
    the replacement with cmake. Also, the logic from extract_strings_qt.py
    can be migrated to cmake.
    d5054beca5
  17. cmake: Drop dependency on sed for translate target 05255d5d1e
  18. purpleKarrot force-pushed on Aug 18, 2025
  19. hebasto approved
  20. hebasto commented at 10:51 am on August 18, 2025: member
    ACK 05255d5d1ec1852d8d8d7683ccbf28351f57b89e.
  21. hebasto merged this on Aug 18, 2025
  22. hebasto closed this on Aug 18, 2025

  23. fanquake commented at 10:57 am on August 18, 2025: member
    Would have been good to update the PR description before merge. It says “As a follow up, the dependency on sed may be removed …” but it looks like that was done here?

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-21 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me