init nsis pack qt #6829
init nsis pack qt #6829
Conversation
RubMaker
commented
Sep 19, 2025
RubMaker
commented
- Before adding new features and new modules, please go to issues to submit the relevant feature description first.
- Write good commit messages and use the same coding conventions as the rest of the project.
- Please commit code to dev branch and we will merge into master branch in feature
- Ensure your edited codes with four spaces instead of TAB.
- 增加新特性和新模块之前，请先到issues提交相关特性说明，经过讨论评估确认后，再进行相应的代码提交，避免做无用工作。
- 编写友好可读的提交信息，并使用与工程代码相同的代码规范，代码请用4个空格字符代替tab缩进。
- 请提交代码到dev分支，如果通过，我们会在特定时间合并到master分支上。
- 为了规范化提交日志的格式，commit消息，不要用中文，请用英文描述。
Summary of ChangesHello @RubMaker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the NSIS packaging plugin by introducing comprehensive support for Qt application deployment. It automates the process of collecting and including Qt runtime dependencies, either by utilizing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for packaging Qt applications with NSIS. It introduces several new helper functions to detect Qt projects, find windeployqt, and deploy Qt dependencies. The changes are comprehensive, but there are several areas for improvement regarding code clarity, resource management, and correctness. I've found a critical bug related to premature cleanup of temporary files, some resource leaks, and inconsistencies in install/uninstall logic. I've also suggested several refactorings to improve code quality and maintainability.
| print("Generated", #nsis_commands, "NSIS commands for Qt deployment") | ||
|
|
||
| -- Clean up temporary deployment directory | ||
| _safe_rmdir(deploy_dir) |
There was a problem hiding this comment.
The temporary deployment directory deploy_dir is removed at the end of _deploy_qt_dependencies. However, the nsis_commands returned by this function contain absolute paths to files within deploy_dir. These files are needed by makensis later, but they will have been deleted by this point. This will cause the packaging to fail. The cleanup of deploy_dir should be deferred until after makensis has finished running.
| print("Removing existing temporary directory:", deploy_dir) | ||
| _safe_rmdir(deploy_dir) | ||
| -- Wait a bit to ensure directory is removed | ||
| os.sleep(100) |
There was a problem hiding this comment.
Using os.sleep() to wait for a directory to be removed is unreliable and can introduce race conditions. The _safe_rmdir function should be synchronous and guarantee the directory is removed before it returns. If there are still issues, the problem might be in _safe_rmdir or the underlying OS commands, and that should be investigated instead of using a sleep. Please remove this line.
| [Paths] | ||
| Plugins = . | ||
| ]] | ||
| local qt_conf_file = os.tmpfile() .. ".conf" |
There was a problem hiding this comment.
The temporary file qt_conf_file created with os.tmpfile() is not deleted, leading to a resource leak on the build machine. This file needs to exist when makensis runs, so it should be cleaned up afterwards. A similar issue exists in the manual deployment fallback on line 581. A possible solution is to return the temporary file path and clean it up in a higher-level function after packaging is complete.
| -- Add version-specific cleanup | ||
| if is_qt6 then | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt6Core.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt6Gui.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt6Widgets.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt6Network.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt6Sql.dll"') | ||
| else | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt5Core.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt5Gui.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt5Widgets.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt5Network.dll"') | ||
| table.insert(qt_cleanup_commands, '${unRMFileIfExists} "$InstDir\\bin\\Qt5Sql.dll"') | ||
| end |
There was a problem hiding this comment.
The list of Qt DLLs to be removed by the uninstaller is incomplete. The manual deployment logic in _get_qt_deployment_commands can deploy many other libraries (e.g., for XML, PrintSupport, Multimedia, OpenGL, SVG, QML/Quick) which are not cleaned up here. This will leave files behind after uninstallation. The cleanup logic should be made consistent with the deployment logic to ensure a clean uninstall.
| path.join("build", plat, "release", package:name() .. ".exe"), | ||
| path.join("build", "release", package:name() .. ".exe"), |
There was a problem hiding this comment.
There is a duplicated path in the possible_paths table. This is likely a copy-paste error and should be removed.
path.join("build", "release", package:name() .. ".exe"),
| local function _safe_rmdir(dir) | ||
| if not os.isdir(dir) then | ||
| return true | ||
| end | ||
|
|
||
| -- Try multiple methods to remove directory | ||
| local ok = false | ||
|
|
||
| -- Method 1: Use os.tryrm first | ||
| ok = os.tryrm(dir) | ||
| if ok then | ||
| print("Successfully removed directory using os.tryrm:", dir) | ||
| return true | ||
| end | ||
|
|
||
| -- Method 2: Try Windows rmdir command | ||
| if is_host("windows") then | ||
| print("Trying Windows rmdir command for:", dir) | ||
| local ok2 = try { function() | ||
| os.vrunv("cmd", {"/c", "rmdir", "/s", "/q", path.translate(dir)}) | ||
| return true | ||
| end } | ||
| if ok2 then | ||
| print("Successfully removed directory using rmdir:", dir) | ||
| return true | ||
| end | ||
| end | ||
|
|
||
| -- Method 3: Try PowerShell Remove-Item | ||
| if is_host("windows") then | ||
| print("Trying PowerShell Remove-Item for:", dir) | ||
| local ok3 = try { function() | ||
| os.vrunv("powershell", {"-Command", "Remove-Item -Recurse -Force '" .. dir .. "'"}) | ||
| return true | ||
| end } | ||
| if ok3 then | ||
| print("Successfully removed directory using PowerShell:", dir) | ||
| return true | ||
| end | ||
| end | ||
|
|
||
| return false | ||
| end |
There was a problem hiding this comment.
This function contains several print statements which seem to be for debugging. In xmake, it's better to use dprint for debug messages. Also, the ok variable is declared but its value is not used after being initialized, making it redundant. The logic can be simplified by removing it and returning directly after a successful removal attempt.
|
|
||
| -- Add Qt deployment commands if this is a Qt project | ||
| if _is_qt_project(package) then | ||
| print("Adding Qt deployment commands to installer") |
There was a problem hiding this comment.
This print statement should be cprint to follow xmake's logging conventions for user-facing information.
cprint("Adding Qt deployment commands to installer")
|
|
||
| -- Add Qt cleanup commands if this is a Qt project | ||
| if _is_qt_project(package) then | ||
| print("Adding Qt cleanup commands to uninstaller") |
There was a problem hiding this comment.
This print statement should be cprint to follow xmake's logging conventions for user-facing information.
cprint("Adding Qt cleanup commands to uninstaller")
| if links then | ||
| for _, link in ipairs(links) do | ||
| if link:lower():find("qt") then | ||
| print("Qt project detected via link:", link) |
There was a problem hiding this comment.
This print statement and others in this function (lines 187, 204, 211) seem to be for debugging. It's better to use dprint for debug messages in xmake, so they only appear in verbose mode.
dprint("Qt project detected via link: %s", link)
| -- check if this is a Qt project | ||
| local is_qt = _is_qt_project(package) | ||
| if is_qt then | ||
| cprint("Detected Qt project - using windeployqt for proper dependency deployment") | ||
|
|
||
| -- Check for windeployqt availability | ||
| local windeployqt = _get_windeployqt() | ||
| end |
There was a problem hiding this comment.
The call to _get_windeployqt() here is redundant. The returned windeployqt variable is not used, and _get_windeployqt() is already called inside _get_qt_deployment_commands when it's needed. This block of code can be removed to avoid the unnecessary check.
1 participant
