init wix pack qt #6830
init wix pack qt #6830
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 WiX packaging plugin by adding comprehensive support for Qt applications. It introduces intelligent detection of Qt projects and their dependencies, leveraging 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 introduces support for packaging Qt applications using WiX. The changes include detecting Qt projects, using windeployqt to gather dependencies, and generating the necessary WiX configuration. My review identifies a critical bug in argument handling for windeployqt and other issues related to portability, code redundancy, and maintainability. I've provided suggestions to fix these issues and improve the overall quality of the implementation.
| -- Check if this is a QML project | ||
| local uses_qml = _check_qml_usage(package) | ||
| if uses_qml then | ||
| table.insert(args, "--qmldir") | ||
| local qml_dir = _find_project_qml_dir() | ||
| if qml_dir then | ||
| table.insert(args, qml_dir) | ||
| else | ||
| -- Use Qt's QML directory if available | ||
| if qt.qmldir and os.isdir(qt.qmldir) then | ||
| table.insert(args, qt.qmldir) | ||
| else | ||
| -- Remove the --qmldir flag if no valid directory found | ||
| table.remove(args) -- remove qml_dir placeholder | ||
| table.remove(args) -- remove --qmldir | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
There is a critical bug in the logic for handling the --qmldir argument. When no QML directory is found, you call table.remove(args) twice. The first call correctly removes the "--qmldir" flag, but the second call incorrectly removes the preceding argument, which is the deployment directory path. This will cause windeployqt to fail.
The logic should be refactored to only add the --qmldir argument and its value if a valid directory is found.
-- Check if this is a QML project
local uses_qml = _check_qml_usage(package)
if uses_qml then
local qml_dir_to_use = _find_project_qml_dir()
if not qml_dir_to_use and qt.qmldir and os.isdir(qt.qmldir) then
qml_dir_to_use = qt.qmldir
end
if qml_dir_to_use then
table.insert(args, "--qmldir")
table.insert(args, qml_dir_to_use)
end
end
| -- Create a temporary deployment directory | ||
| local deploy_dir = path.join(os.tmpdir(), package:name() .. "_qt_deploy") | ||
| if os.isdir(deploy_dir) then | ||
| os.vrunv("rm", {"-r", "-fo", deploy_dir}) |
There was a problem hiding this comment.
Using os.vrunv("rm", ...) is not portable and will likely fail on Windows systems that do not have an rm executable in the system's PATH. For portability, you should use xmake's built-in os.rm() function, which can recursively remove directories.
os.rm(deploy_dir)
| -- Create components for each directory | ||
| for dir, files in pairs(file_groups) do | ||
| local component_id = _get_id("QtRuntime" .. dir) | ||
| local subdir = (dir ~= "bin" and dir ~= ".") and dir or nil |
There was a problem hiding this comment.
The logic for determining the component's subdirectory is incorrect. When dir is "bin", subdir becomes nil. This will cause the files in this component to be installed into INSTALLFOLDER instead of the intended INSTALLFOLDER\bin directory. To fix this, the subdir should be set to dir as long as dir is not ..
local subdir = (dir ~= ".") and dir or nil
| local mode = is_mode("debug") and "debug" or "release" | ||
| local possible_paths = { | ||
| path.join("build", plat, arch, mode, package:name() .. ".exe"), | ||
| path.join("build", plat, arch, "release", package:name() .. ".exe"), |
There was a problem hiding this comment.
This path check is redundant. When is_mode("debug") is false, the mode variable becomes "release", making the path checked on line 151 identical to this one. You can safely remove this line to avoid the duplicate check.
| for _, link in ipairs(links) do | ||
| local link_lower = link:lower() | ||
| if is_qt6 then | ||
| if link_lower:find("qt6network") or link_lower:find("qtnetwork") then | ||
| table.insert(additional_dlls, "Qt6Network.dll") | ||
| end | ||
| if link_lower:find("qt6multimedia") or link_lower:find("qtmultimedia") then | ||
| table.insert(additional_dlls, "Qt6Multimedia.dll") | ||
| end | ||
| if link_lower:find("qt6opengl") or link_lower:find("qtopengl") then | ||
| table.insert(additional_dlls, "Qt6OpenGL.dll") | ||
| end | ||
| if link_lower:find("qt6svg") or link_lower:find("qtsvg") then | ||
| table.insert(additional_dlls, "Qt6Svg.dll") | ||
| end | ||
| if link_lower:find("qt6xml") or link_lower:find("qtxml") then | ||
| table.insert(additional_dlls, "Qt6Xml.dll") | ||
| end | ||
| if link_lower:find("qt6quick") or link_lower:find("qtquick") then | ||
| table.insert(additional_dlls, "Qt6Quick.dll") | ||
| table.insert(additional_dlls, "Qt6Qml.dll") | ||
| end | ||
| else | ||
| if link_lower:find("qt5network") or link_lower:find("qtnetwork") then | ||
| table.insert(additional_dlls, "Qt5Network.dll") | ||
| end | ||
| if link_lower:find("qt5multimedia") or link_lower:find("qtmultimedia") then | ||
| table.insert(additional_dlls, "Qt5Multimedia.dll") | ||
| end | ||
| if link_lower:find("qt5opengl") or link_lower:find("qtopengl") then | ||
| table.insert(additional_dlls, "Qt5OpenGL.dll") | ||
| end | ||
| if link_lower:find("qt5svg") or link_lower:find("qtsvg") then | ||
| table.insert(additional_dlls, "Qt5Svg.dll") | ||
| end | ||
| if link_lower:find("qt5xml") or link_lower:find("qtxml") then | ||
| table.insert(additional_dlls, "Qt5Xml.dll") | ||
| end | ||
| if link_lower:find("qt5quick") or link_lower:find("qtquick") then | ||
| table.insert(additional_dlls, "Qt5Quick.dll") | ||
| table.insert(additional_dlls, "Qt5Qml.dll") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This block of code for detecting required DLLs is highly repetitive for Qt5 and Qt6, which makes it difficult to read and maintain. You can refactor this by using a configuration table for modules and dynamically generating the DLL names based on the Qt version. This will greatly reduce code duplication and improve maintainability.
for _, link in ipairs(links) do
local link_lower = link:lower()
local modules = {
network = "Network",
multimedia = "Multimedia",
opengl = "OpenGL",
svg = "Svg",
xml = "Xml",
quick = {"Quick", "Qml"}
}
local qt_prefix = is_qt6 and "Qt6" or "Qt5"
local qt_version_prefix = is_qt6 and "qt6" or "qt5"
for name, suffixes in pairs(modules) do
if link_lower:find(qt_version_prefix .. name) or link_lower:find("qt" .. name) then
for _, suffix in ipairs(table.wrap(suffixes)) do
table.insert(additional_dlls, qt_prefix .. suffix .. ".dll")
end
end
end
end
| -- Add additional DLLs if they exist | ||
| for _, dll_name in ipairs(additional_dlls) do | ||
| local dll_path = path.join(qt.bindir, dll_name) | ||
| if os.isfile(dll_path) then | ||
| local already_added = false | ||
| for _, existing in ipairs(qt_dlls) do | ||
| if existing[2] == dll_name then | ||
| already_added = true | ||
| break | ||
| end | ||
| end | ||
| if not already_added then | ||
| table.insert(qt_dlls, {dll_path, dll_name}) | ||
| print("Found additional Qt DLL:", dll_path) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Checking for already_added by iterating through qt_dlls for each additional DLL is inefficient, with a time complexity of O(N*M). This can be optimized by using a Lua table as a hash set to track added DLL names, allowing for O(1) lookups.
-- Add additional DLLs if they exist
local added_dll_names = {}
for _, existing in ipairs(qt_dlls) do
added_dll_names[existing[2]] = true
end
for _, dll_name in ipairs(table.unique(additional_dlls)) do
local dll_path = path.join(qt.bindir, dll_name)
if os.isfile(dll_path) and not added_dll_names[dll_name] then
table.insert(qt_dlls, {dll_path, dll_name})
added_dll_names[dll_name] = true
print("Found additional Qt DLL:", dll_path)
end
end
| local qt = find_qt() | ||
| if qt then | ||
| specvars.PACKAGE_QT_VERSION = qt.sdkver or "unknown" | ||
| end |
There was a problem hiding this comment.
The find_qt() function is called here again, but it has already been called within _collect_qt_dlls. To avoid this redundant call and improve code clarity, you could modify _collect_qt_dlls to return the qt object it finds, and then reuse that object here. Although find_qt is cached, this refactoring would improve the code's structure and reduce coupling.
| -- Check if this is a Qt project and inform the user | ||
| local is_qt = _is_qt_project(package) | ||
| if is_qt then | ||
| local windeployqt = _get_windeployqt() | ||
| end |
There was a problem hiding this comment.
This block of code is redundant and has no effect. The local variables is_qt and windeployqt are declared but never used. The functions _is_qt_project and _get_windeployqt are already called within _get_specvars later in the execution flow. This block should be removed.
|
不应该每个格式加这种 qt支持，按 #6826 (comment) 这里的说明，通过 qt rule + on_installcmd 来实现，所有格式可以全部生效 qt 支持 另外，不要一下子提交这么多，先把一个格式的 qt 实现做出来 merge ，后面扩展微调就省事了，一下子这么多 pr 我也 review 不过来 |
This kind of qt support should not be added to each format. According to #6826 (comment), the instructions here are implemented through qt rule + on_installcmd. All formats can take effect. qt support In addition, don't submit so many at once. First, make a merge implementation of a format. The expansion and fine-tuning will save trouble. I'll review it too. |
| end | ||
|
|
||
| -- detect if this is a Qt project | ||
| function _is_qt_project(package) |
4 participants
