-
-
Notifications
You must be signed in to change notification settings - Fork 872
Feat appimage pack #6839
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
base: dev
Are you sure you want to change the base?
Feat appimage pack #6839
Conversation
RubMaker
commented
Sep 21, 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 build system by adding robust support for AppImage packaging. It provides a streamlined workflow for creating self-contained Linux applications, handling everything from tool detection and dependency resolution to the generation of necessary AppImage metadata and the final executable. This feature simplifies application distribution on Linux by minimizing dependency issues and installation complexities. 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 introduces AppImage packaging support, a valuable addition. The implementation is well-structured, featuring a robust fallback mechanism for dependency collection. I've identified a couple of critical areas where the code could fail in certain environments, along with several opportunities to enhance error reporting and clarity. My comments focus on improving the robustness and maintainability of the new packaging logic.
local homedir = os.getenv("HOME") | ||
local paths = { | ||
"/usr/local/bin/linuxdeploy", | ||
"/usr/bin/linuxdeploy", | ||
"/opt/linuxdeploy/linuxdeploy", | ||
path.join(os.tmpdir(), "linuxdeploy"), | ||
path.join(homedir, "Downloads/linuxdeploy"), | ||
path.join(homedir, "downloads/linuxdeploy"), | ||
path.join(homedir, ".local/bin/linuxdeploy"), | ||
path.join(homedir, "bin/linuxdeploy"), | ||
"/snap/bin/linuxdeploy", | ||
"/var/lib/flatpak/exports/bin/linuxdeploy", | ||
path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeploy"), | ||
path.join(os.curdir(), "linuxdeploy"), | ||
path.join(os.curdir(), "tools/linuxdeploy"), | ||
path.join(os.curdir(), "bin/linuxdeploy") | ||
} |
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.
There's a potential crash here if the HOME
environment variable is not set. os.getenv("HOME")
would return nil
, and path.join(nil, ...)
will likely cause an error when constructing the paths
table. To prevent this, you should only attempt to construct paths using homedir
after verifying it's not nil
.
local homedir = os.getenv("HOME")
local paths = {
"/usr/local/bin/linuxdeploy",
"/usr/bin/linuxdeploy",
"/opt/linuxdeploy/linuxdeploy",
path.join(os.tmpdir(), "linuxdeploy"),
"/snap/bin/linuxdeploy",
"/var/lib/flatpak/exports/bin/linuxdeploy",
path.join(os.curdir(), "linuxdeploy"),
path.join(os.curdir(), "tools/linuxdeploy"),
path.join(os.curdir(), "bin/linuxdeploy")
}
if homedir then
table.insert(paths, path.join(homedir, "Downloads/linuxdeploy"))
table.insert(paths, path.join(homedir, "downloads/linuxdeploy"))
table.insert(paths, path.join(homedir, ".local/bin/linuxdeploy"))
table.insert(paths, path.join(homedir, "bin/linuxdeploy"))
table.insert(paths, path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeploy"))
end
local homedir = os.getenv("HOME") | ||
|
||
local paths = { | ||
"/usr/local/bin/linuxdeployqt", | ||
"/usr/bin/linuxdeployqt", | ||
"/opt/linuxdeployqt/linuxdeployqt", | ||
path.join(os.tmpdir(), "linuxdeployqt"), | ||
path.join(homedir, "Downloads/linuxdeployqt"), | ||
path.join(homedir, "downloads/linuxdeployqt"), | ||
path.join(homedir, ".local/bin/linuxdeployqt"), | ||
path.join(homedir, "bin/linuxdeployqt"), | ||
"/snap/bin/linuxdeployqt", | ||
"/var/lib/flatpak/exports/bin/linuxdeployqt", | ||
path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeployqt"), | ||
path.join(os.curdir(), "linuxdeployqt"), | ||
path.join(os.curdir(), "tools/linuxdeployqt"), | ||
path.join(os.curdir(), "bin/linuxdeployqt") | ||
} |
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.
Similar to find_linuxdeploy.lua
, this code is susceptible to a crash if the HOME
environment variable is not set. os.getenv("HOME")
will return nil
, causing path.join(nil, ...)
to fail. You should ensure homedir
is valid before using it to construct paths.
local homedir = os.getenv("HOME")
local paths = {
"/usr/local/bin/linuxdeployqt",
"/usr/bin/linuxdeployqt",
"/opt/linuxdeployqt/linuxdeployqt",
path.join(os.tmpdir(), "linuxdeployqt"),
"/snap/bin/linuxdeployqt",
"/var/lib/flatpak/exports/bin/linuxdeployqt",
path.join(os.curdir(), "linuxdeployqt"),
path.join(os.curdir(), "tools/linuxdeployqt"),
path.join(os.curdir(), "bin/linuxdeployqt")
}
if homedir then
table.insert(paths, path.join(homedir, "Downloads/linuxdeployqt"))
table.insert(paths, path.join(homedir, "downloads/linuxdeployqt"))
table.insert(paths, path.join(homedir, ".local/bin/linuxdeployqt"))
table.insert(paths, path.join(homedir, "bin/linuxdeployqt"))
table.insert(paths, path.join(homedir, ".local/share/flatpak/exports/bin/linuxdeployqt"))
end
.marscode/deviceInfo.json
Outdated
{ | ||
"deviceId": "1d865d85971033dbc6e2146fc84e4bab560676c51efb52db347df5c3d503bafb" | ||
} No newline at end of file |
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 file appears to contain a user-specific or machine-specific device ID. Such files should not be committed to the version control system as they can expose sensitive information and are not relevant to other developers. Please remove this file from the repository and add .marscode/
to your .gitignore
file to prevent it from being accidentally committed in the future.
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_appimagetool() | ||
local appimagetool = find_tool("appimagetool") | ||
if not appimagetool then | ||
assert(appimagetool, "appimagetool need to be downloaded!") |
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.
xmake/plugins/pack/appimage/main.lua
Outdated
if not ok then | ||
return false | ||
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.
if not ok then | ||
return false | ||
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.
xmake/plugins/pack/appimage/main.lua
Outdated
if linuxdeployqt then | ||
deps_collected = _collect_qt_deps_with_linuxdeployqt(package, appdir, linuxdeployqt) | ||
else | ||
print("linuxdeployqt not available, will try fallback methods") |
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.
xmake/plugins/pack/appimage/main.lua
Outdated
if linuxdeploy then | ||
deps_collected = _collect_deps_with_linuxdeploy(package, appdir, linuxdeploy) | ||
else | ||
print("linuxdeploy not available") |
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.
-- set nsis display icon | ||
"xpack.set_nsis_displayicon", | ||
-- set appimage icon name | ||
"xpack.set_iconname", |
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.
这里也是
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.
这里还没改
local program = find_program("linuxdeploy", opt) | ||
|
||
-- if not found, check common installation paths | ||
if not program then |
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.
这里都没改
-- You may obtain a copy of the License at | ||
-- | ||
-- http://www.apache.org/licenses/LICENSE-2.0 | ||
-- |
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.
qt相关的先删了
|
||
-- if not found, check common installation paths | ||
if not program then | ||
local paths = { |
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.
这里也没改,看下之前的 review 信息
xmake/plugins/pack/appimage/main.lua
Outdated
end | ||
|
||
-- collect Qt dependencies with linuxdeployqt (rewritten) | ||
function _collect_qt_deps_with_linuxdeployqt(package, appdir, linuxdeployqt) |
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.
这里都没删,qt 的
-- limitations under the License. | ||
-- | ||
-- Copyright (C) 2015-present, Xmake Open Source Community. | ||
-- |
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.
删了
end | ||
return appimagetool | ||
end | ||
-- get linuxdeploy tool |
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.
不要挤一起,适当换行下。。
-- get linuxdeploy tool | ||
function _get_linuxdeploy() | ||
local linuxdeploy = assert(find_tool("linuxdeploy"), "linuxdeploy not found!Pease install it in Downloads folder.") | ||
return linuxdeploy |
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.
删了
end | ||
-- execute install commands | ||
for _, cmd in ipairs(installcmds) do | ||
os.exec(cmd) |
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.
这里也没改,看下最早我的 review ,直接走 runcmds
1ef0c63
to
57bd2a6
Compare
57bd2a6
to
22ad6db
Compare
这里还有一些 review 没有处理 |
There are still some reviews that are not processed |