fix:(cmake) Improve Qt lrelease detection for translation generation #409
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hillwoodroc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @hillwoodroc. Thanks for your PR. 😃 |
|
Hi @hillwoodroc. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts file permission logging for Qt 6.10+ compatibility and improves detection of the Qt lrelease tool in the translation generation CMake script. Flow diagram for QFileInfo permission logging with Qt version checkflowchart TD
Start[Start main logging block] --> CreateFileInfo[Create QFileInfo from path]
CreateFileInfo --> CheckQtVersion{QT_VERSION >= 6.10.0?}
CheckQtVersion -- Yes --> LogUintPerm[Log file info with permissions cast to uint]
CheckQtVersion -- No --> LogPerm[Log file info with raw permissions]
LogUintPerm --> End[Continue program]
LogPerm --> End
Flow diagram for improved Qt lrelease detection in translation generationflowchart TD
Start[Start TRANSLATION_GENERATE function] --> FindLinguistTools[find_package QtLinguistTools]
FindLinguistTools --> FindLrelease[find_program Qt_LRELEASE_EXECUTABLE in standard Qt bin paths]
FindLrelease --> CheckFound{Qt_LRELEASE_EXECUTABLE found?}
CheckFound -- Yes --> UseFound[Use found Qt_LRELEASE_EXECUTABLE]
CheckFound -- No --> SetFallback[Set QT_LRELEASE to /lib/qt$QT_VERSION_MAJOR/bin/lrelease]
SetFallback --> LogMessage[message STATUS about fallback QT_LRELEASE]
UseFound --> End[End TRANSLATION_GENERATE]
LogMessage --> End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- For the QFileInfo::permissions logging, consider using the same representation across all Qt versions (e.g., always
static_cast<uint>(fileInfo.permissions())) so log format stays consistent regardless of the Qt version compiled against. - In
translation-generate.cmake,find_program(Qt${QT_VERSION_MAJOR}_LRELEASE_EXECUTABLE ...)never populatesQT_LRELEASEwhen it succeeds, so you may want to assignQT_LRELEASEfromQt${QT_VERSION_MAJOR}_LRELEASE_EXECUTABLEto ensure the found binary is actually used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the QFileInfo::permissions logging, consider using the same representation across all Qt versions (e.g., always `static_cast<uint>(fileInfo.permissions())`) so log format stays consistent regardless of the Qt version compiled against.
- In `translation-generate.cmake`, `find_program(Qt${QT_VERSION_MAJOR}_LRELEASE_EXECUTABLE ...)` never populates `QT_LRELEASE` when it succeeds, so you may want to assign `QT_LRELEASE` from `Qt${QT_VERSION_MAJOR}_LRELEASE_EXECUTABLE` to ensure the found binary is actually used.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
hillwoodroc
force-pushed
the
master
branch
3 times, most recently
from
d1ef13b to
faf93f7
Compare
|
We need to resolve the conflicts, and also note that our system supports Qt5 compilation, which requires dual authentication. |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Qt lrelease detection for translation generation in CMake. However, the changes introduce critical bugs that break the translation generation functionality.
Changes:
- Modified find_package to use REQUIRED and COMPONENTS syntax for Qt LinguistTools
- Replaced fallback logic with version-specific lrelease detection for Qt6 vs Qt5
- Removed input validation for the translation directory parameter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return() | ||
| set(QT_LRELEASE "lupdate") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
The validation check that ensures ARGN (the translation directory path) is provided has been removed. Without this check, if the function is called without a directory path, it will silently fail or produce unexpected behavior when trying to find .ts files on line 13. The original error message provided clear feedback about incorrect usage.
| # Ensure a translation directory path is provided | |
| if (NOT ARGN) | |
| message(FATAL_ERROR "TRANSLATION_GENERATE requires a translation directory path argument (e.g. translations).") | |
| endif() |
cmake/translation-generate.cmake
Outdated
| if(NOT ARGN) | ||
| message(SEND_ERROR "Error: TRANSLATION_GENERATE() called without any .ts path") | ||
| return() | ||
| set(QT_LRELEASE "lupdate") |
There was a problem hiding this comment.
For Qt5, the code should retrieve the lrelease executable path similar to Qt6. The current hardcoded string "lrelease" may not work if the lrelease binary is not in the system PATH. Consider using get_target_property with Qt5::lrelease or the Qt5_LRELEASE_EXECUTABLE variable that was available from find_package.
| set(QT_LRELEASE "lupdate") | |
| if (TARGET Qt5::lrelease) | |
| get_target_property(QT_LRELEASE Qt5::lrelease LOCATION) | |
| elseif(DEFINED Qt5_LRELEASE_EXECUTABLE) | |
| set(QT_LRELEASE "${Qt5_LRELEASE_EXECUTABLE}") | |
| else() | |
| # Fallback: rely on lrelease being in PATH | |
| set(QT_LRELEASE "lrelease") | |
| endif() |
hillwoodroc
force-pushed
the
master
branch
2 times, most recently
from
4df096c to
d726f70
Compare
hillwoodroc
changed the title
Use Qt6::lrelease imported target from Qt6 LinguistTools instead of manually guessing the lrelease path, fixing build failures when the command cannot be found.
4 participants
fix:(cmake) Improve Qt lrelease detection for translation generation