-
-
Notifications
You must be signed in to change notification settings - Fork 882
deb pack qt #6828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deb pack qt #6828
Conversation
RubMaker
commented
Sep 19, 2025
- 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 Xmake Debian packaging plugin by introducing robust support for Qt projects. It implements intelligent detection of Qt applications, automatically determines and includes the correct Qt5 or Qt6 runtime and build dependencies, and integrates a mechanism to remove RPATH from binaries. These changes streamline the packaging of Qt-based software for Debian, improving compatibility and maintainability, while also standardizing maintainer metadata. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds functionality for packaging Qt applications into Debian (.deb) packages. The changes include detecting Qt projects, determining Qt dependencies, and handling RPATH for binaries.
My review focuses on improving code quality and performance. I've identified a few areas for improvement:
- Removal of a debug
printstatement. - Refactoring duplicated code for determining Qt dependencies to improve maintainability.
- Removing a redundant and inefficient implementation for RPATH removal, favoring a more standard approach already present in the
debian/rulesfile.
Overall, this is a great addition. Addressing these points will make the implementation more robust and maintainable.
| -- Add RPATH removal commands | ||
| local rpath_cmds = _remove_rpath_from_binaries(package) | ||
| for _, rpath_cmd in ipairs(rpath_cmds) do | ||
| table.insert(installcmds, rpath_cmd) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to remove RPATH is duplicated. It's implemented here in Lua (by calling _remove_rpath_from_binaries) and also directly in xmake/scripts/xpack/deb/debian/rules using find commands. The find commands in the rules file are more efficient and idiomatic for Debian packaging. This Lua implementation is redundant and less performant, as it iterates through all files and calls file and chrpath for each one.
To avoid redundancy and improve performance, this block should be removed. The _remove_rpath_from_binaries function (lines 209-236) and _get_chrpath function (lines 39-42) also become unnecessary and can be removed.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function _get_qt_dependencies(package) | ||
| local qt = find_qt() | ||
| local qt_deps = {} | ||
| local qt_build_deps = {} | ||
| if not qt then | ||
| -- Fallback generic dependencies | ||
| table.insert(qt_deps, "libqt5core5a") | ||
| table.insert(qt_deps, "libqt5gui5") | ||
| table.insert(qt_build_deps, "qtbase5-dev") | ||
| return qt_deps, qt_build_deps | ||
| end | ||
| local qt_version = qt.sdkver or "5.15.3" | ||
| local qt_major = qt_version:match("^(%d+)") | ||
| if qt_major == "6" then | ||
| -- Qt6 dependencies | ||
| table.insert(qt_deps, "libqt6core6") | ||
| table.insert(qt_deps, "libqt6gui6") | ||
| table.insert(qt_deps, "libqt6widgets6") | ||
|
|
||
| -- Qt6 build dependencies | ||
| table.insert(qt_build_deps, "qt6-base-dev") | ||
| table.insert(qt_build_deps, "qt6-tools-dev") | ||
| table.insert(qt_build_deps, "qt6-tools-dev-tools") | ||
| -- Check for additional Qt6 modules based on links | ||
| local links = package:get("links") or {} | ||
| for _, link in ipairs(links) do | ||
| local lower_link = link:lower() | ||
| if lower_link:find("qt6network") or lower_link:find("qtnetwork") then | ||
| table.insert(qt_deps, "libqt6network6") | ||
| elseif lower_link:find("qt6sql") or lower_link:find("qtsql") then | ||
| table.insert(qt_deps, "libqt6sql6") | ||
| elseif lower_link:find("qt6xml") or lower_link:find("qtxml") then | ||
| table.insert(qt_deps, "libqt6xml6") | ||
| elseif lower_link:find("qt6opengl") or lower_link:find("qtopengl") then | ||
| table.insert(qt_deps, "libqt6opengl6") | ||
| table.insert(qt_build_deps, "qt6-opengl-dev") | ||
| elseif lower_link:find("qt6multimedia") or lower_link:find("qtmultimedia") then | ||
| table.insert(qt_deps, "libqt6multimedia6") | ||
| table.insert(qt_build_deps, "qt6-multimedia-dev") | ||
| elseif lower_link:find("qt6webengine") or lower_link:find("qtwebengine") then | ||
| table.insert(qt_deps, "libqt6webenginewidgets6") | ||
| table.insert(qt_build_deps, "qt6-webengine-dev") | ||
| elseif lower_link:find("qt6qml") or lower_link:find("qtqml") then | ||
| table.insert(qt_deps, "libqt6qml6") | ||
| table.insert(qt_build_deps, "qt6-declarative-dev") | ||
| end | ||
| end | ||
|
|
||
| else | ||
| -- Qt5 dependencies (default) | ||
| table.insert(qt_deps, "libqt5core5a") | ||
| table.insert(qt_deps, "libqt5gui5") | ||
| table.insert(qt_deps, "libqt5widgets5") | ||
|
|
||
| -- Qt5 build dependencies | ||
| table.insert(qt_build_deps, "qtbase5-dev") | ||
| table.insert(qt_build_deps, "qt5-qmake") | ||
| table.insert(qt_build_deps, "qttools5-dev") | ||
| table.insert(qt_build_deps, "qttools5-dev-tools") | ||
|
|
||
| -- Check for additional Qt5 modules based on links | ||
| local links = package:get("links") or {} | ||
| for _, link in ipairs(links) do | ||
| local lower_link = link:lower() | ||
| if lower_link:find("qt5network") or lower_link:find("qtnetwork") then | ||
| table.insert(qt_deps, "libqt5network5") | ||
| elseif lower_link:find("qt5sql") or lower_link:find("qtsql") then | ||
| table.insert(qt_deps, "libqt5sql5") | ||
| elseif lower_link:find("qt5xml") or lower_link:find("qtxml") then | ||
| table.insert(qt_deps, "libqt5xml5") | ||
| elseif lower_link:find("qt5opengl") or lower_link:find("qtopengl") then | ||
| table.insert(qt_deps, "libqt5opengl5") | ||
| table.insert(qt_build_deps, "libqt5opengl5-dev") | ||
| elseif lower_link:find("qt5multimedia") or lower_link:find("qtmultimedia") then | ||
| table.insert(qt_deps, "libqt5multimedia5") | ||
| table.insert(qt_build_deps, "qtmultimedia5-dev") | ||
| elseif lower_link:find("qt5webengine") or lower_link:find("qtwebengine") then | ||
| table.insert(qt_deps, "libqt5webenginewidgets5") | ||
| table.insert(qt_build_deps, "qtwebengine5-dev") | ||
| elseif lower_link:find("qt5qml") or lower_link:find("qtqml") then | ||
| table.insert(qt_deps, "libqt5qml5") | ||
| table.insert(qt_build_deps, "qtdeclarative5-dev") | ||
| elseif lower_link:find("qt5svg") or lower_link:find("qtsvg") then | ||
| table.insert(qt_deps, "libqt5svg5") | ||
| table.insert(qt_build_deps, "libqt5svg5-dev") | ||
| end | ||
| end | ||
| end | ||
| return qt_deps, qt_build_deps | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has a lot of duplicated logic for handling Qt5 and Qt6 dependencies.
- The
package:get("links")call is duplicated. It can be moved to the top of the function. - The large
if/elseifblocks for detecting modules are almost identical for Qt5 and Qt6. This makes the code hard to read and maintain.
This can be refactored using a data-driven approach. For example, you could define maps for Qt5 and Qt6 modules and their corresponding packages:
local qt6_module_map = {
network = { dep = "libqt6network6" },
sql = { dep = "libqt6sql6" },
opengl = { dep = "libqt6opengl6", build_dep = "qt6-opengl-dev" },
-- etc.
}
-- in the loop over links
for mod_name, deps in pairs(qt6_module_map) do
if lower_link:find("qt6" .. mod_name) or lower_link:find("qt" .. mod_name) then
table.insert(qt_deps, deps.dep)
if deps.build_dep then
table.insert(qt_build_deps, deps.build_dep)
end
end
endA similar map can be created for Qt5. This would make the code much cleaner and easier to extend.
|
同样,都放到 qt rules 里面去做。 |
Similarly, put them in qt rules to do it. |