Skip to content

Conversation

@RubMaker
Copy link

  • 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消息,不要用中文,请用英文描述。

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @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 packaging capabilities by adding comprehensive support for AppImage creation. It provides a robust mechanism for bundling applications, including those built with Qt, into a single executable format for Linux. The changes streamline the process of preparing applications for distribution by automating dependency collection and AppDir structure generation.

Highlights

  • AppImage Packaging Support: Introduced a new plugin for creating AppImage packages, enabling easier distribution of Linux applications.
  • Qt Project Integration: Added specialized logic to detect and correctly package Qt-based projects into AppImages, leveraging linuxdeployqt for dependency collection.
  • Flexible Dependency Management: Implemented a multi-tiered approach for collecting dependencies, utilizing linuxdeployqt for Qt projects, linuxdeploy for general cases, and a manual ldd-based fallback.
  • New xpack APIs: Expanded the xpack API with new functions to configure AppImage properties (like icon name and tool) and prepare for future DMG packaging options.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 for xmake, a valuable addition for Linux application distribution. The implementation is comprehensive, covering dependency collection for both generic and Qt-based projects. My review focuses on enhancing the robustness, user experience, and code quality of the new AppImage plugin. Key suggestions include automating the download of required tools, using a more persistent caching mechanism, improving error handling, and general code cleanup for better maintainability.

Comment on lines +32 to +38
function _get_appimagetool()
local appimagetool = find_tool("appimagetool")
if not appimagetool then
assert(appimagetool, "appimagetool need to be downloaded!")
end
return appimagetool
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

For a better user experience and consistency with _get_linuxdeploy and _get_linuxdeployqt, appimagetool should be downloaded automatically if it's not found in the system PATH. Currently, the script just fails with an assertion, which is not user-friendly.

function _get_appimagetool()
    local appimagetool = find_tool("appimagetool")
    if not appimagetool then
        -- TODO: check os.arch() to get the correct url for non-x86_64 architectures
        local appimagetool_url = "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage"
        local appimagetool_path = path.join(os.tmpdir(), "appimagetool")
        if not os.isfile(appimagetool_path) then
            -- For robustness, it's better to use xmake's built-in download utilities
            -- instead of relying on an external tool like wget.
            os.runv("wget", {"-O", appimagetool_path, appimagetool_url})
            os.runv("chmod", {"+x", appimagetool_path})
        end
        appimagetool = {program = appimagetool_path}
    end
    return appimagetool
end

Comment on lines 44 to 49
local linuxdeploy_url = "https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage"
local linuxdeploy_path = path.join(os.tmpdir(), "linuxdeploy")
if not os.isfile(linuxdeploy_path) then
os.runv("wget", {"-O", linuxdeploy_path, linuxdeploy_url})
os.runv("chmod", {"+x", linuxdeploy_path})
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using os.runv("wget", ...) is not robust as wget might not be installed. It's recommended to use xmake's built-in download utilities (e.g., os.download()) for better portability.

Additionally, os.tmpdir() is not a persistent location and its contents can be cleared by the OS. For caching downloaded tools, a directory inside os.cachedir() would be more appropriate to avoid repeated downloads. This feedback also applies to _get_linuxdeployqt.

local lib_dir = path.join(appdir, "usr/lib")
os.mkdir(lib_dir)

local copied_count = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The local variable copied_count is initialized but its value is never used. It should be removed, along with its increment on line 565.

local dst_path = path.join(lib_dir, lib_name)
if not os.isfile(dst_path) then
os.cp(lib_path, dst_path)
copied_count = copied_count + 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line increments the unused variable copied_count. It should be removed.


-- get linuxdeploy tool
function _get_linuxdeploy()
local linuxdeploy = find_tool("linuxdeploy")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同样的,每个 find_tool 都需要有个 find_xxx.lua 实现对应,才能精准查找。

local linuxdeploy_url = "https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage"
local linuxdeploy_path = path.join(os.tmpdir(), "linuxdeploy")
if not os.isfile(linuxdeploy_path) then
os.runv("wget", {"-O", linuxdeploy_path, linuxdeploy_url})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不要这么搞,下载包,你应该到 https://github.com/xmake-io/xmake-repo 仓库,提交一个 linuxdeploy 包。

然后走包安装,参考下 nsis 里面的实现。

table.join2(packages, install_packages("nsis"))

end

-- get linuxqtdeploy tool
function _get_linuxdeployqt()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也是

@waruqi
Copy link
Member

waruqi commented Sep 18, 2025

appimage 和 dmg 拆成两个独立 pr ,重新提交下。方便 review 和 merge,也便于ospp那边审核

@@ -0,0 +1,3 @@
{
"deviceId": "1d865d85971033dbc6e2146fc84e4bab560676c51efb52db347df5c3d503bafb"
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这是啥?删了

local paths = {
"/usr/local/bin/appimagetool",
"/usr/bin/appimagetool",
"/opt/appimagetool/appimagetool"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里判断平台,这些都是 unix 路径,windows 下不会有

if not is_host("windows") then
    table.insert(paths, ..)
end

for _, p in ipairs(paths) do
if os.isfile(p) and os.isexec(p) then
program = p
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里不对,find_program 支持 paths 参数传入,进行查找的,没必要额外自己实现。改下上面的 opt.paths 就行了

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

参考

opt.paths = table.wrap(opt.paths)

}

for _, p in ipairs(paths) do
if os.isfile(p) and os.isexec(p) then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也是,挪到 find_program opt.paths里面

}

for _, p in ipairs(paths) do
if os.isfile(p) and os.isexec(p) then
Copy link
Member

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!")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里后期 xmake-repo 添加对应的包,来安装,不过可以等这里 pr 全部处理完后再弄。

-- get linuxdeploy tool
function _get_linuxdeploy()
local linuxdeploy = assert(find_tool("linuxdeploy"), "linuxdeploy not found!")
-- if not linuxdeploy then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里也是,不过这里注释全删了。

-- if not os.isfile(linuxdeployqt_path) then
-- os.runv("wget", {"-O", linuxdeployqt_path, linuxdeployqt_url})
-- os.runv("chmod", {"+x", linuxdeployqt_path})
-- end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

还有这 删了


-- create temporary AppDir
local appdir_name = package:name() .. ".AppDir"
local appdir = path.join(os.tmpdir(), appdir_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不要用临时目录,放到当前项目 build 目录下

tmpdir -> package:builddir()

if linuxdeployqt then
deps_collected = _collect_qt_deps_with_linuxdeployqt(package, appdir, linuxdeployqt)
else
print("linuxdeployqt not available, will try fallback methods")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

用 raise

if linuxdeploy then
deps_collected = _collect_deps_with_linuxdeploy(package, appdir, linuxdeploy)
else
print("linuxdeploy not available")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise

os.tryrm(appimage_file)

-- set architecture environment variable
local arch = package:get("arch") or "x86_64"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package:arch()

end

-- get dependencies using ldd
local ldd_output = os.iorunv("ldd", {main_executable})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

之前说了,直接用 utils.binary.deplibs 模块

里面会用 ldd 去取依赖库,甚至还会尝试用其他工具取。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

参考下

function _get_target_package_libfiles(target, opt)

local iconname = package:get("iconname") or package:name()

if iconfile and os.isfile(iconfile) then
-- 复制图标到usr/share/icons/hicolor目录
Copy link
Member

@waruqi waruqi Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

中文注释去了


local apprun_file = path.join(appdir, "AppRun")
io.writefile(apprun_file, apprun_content)
os.runv("chmod", {"+x", apprun_file})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没必要执行额外的 chmod 命令,可以提前创建好 .sh 的模版文件,改好 +x 权限,然后直接替换变量就行了

参考

os.cp(path.join(os.programdir(), "scripts", "xpack", "runself", "setup.sh"), setupfile)

local root_icon = path.join(appdir, iconname .. path.extension(iconfile))
os.cp(iconfile, root_icon)

return icon_dst
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

返回值也没用到,删了

end

-- create files required for AppImage
_create_desktop_file(package, appdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这几个的返回值都没用到,删了


-- execute install commands
for _, cmd in ipairs(installcmds) do
os.exec(cmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里不对,为啥要这么搞。

既然要提前执行里面的 cmds,直接 batchcmds.get_installcmds(package):runcmds() 就可以运行了。

参考下

batchcmds.get_installcmds(package):runcmds()

根本没必要 用 _get_installcmds 先转一遍,再执行,_get_installcmds 可以整个删了

@waruqi
Copy link
Member

waruqi commented Sep 19, 2025

现在这个 patch 还是太大,把 qt 相关的先删了,先做最基础的 appimage 支持,方便 merge 。

而且现在的 qt 支持有点问题。所有的 xpack 实现都应该是通用逻辑,不应该跟 qt 这种特定需求的逻辑绑定。

qt 相关的实现应该在 rule 层实现,而不是直接塞到 xpack 实现里。

如果要调用 deployqt 的命令,也应该全部在 qt rules 里面做。

当前 rules 是可以支持自定义 on_installcmd, before_installcmd, after_installcmd, on_uninstallcmd, ... 脚本配置的,

参考下

target:script("installcmd", _on_target_installcmd),

然后在这下面添加 on_installcmd 来扩展 deployqt 执行去打包。

after_build("android", "deploy.android")
after_build("macosx", "deploy.macosx")
-- install application for android
on_install("android", "install.android")
after_install("windows", "install.windows")
after_install("mingw", "install.mingw")

@RubMaker RubMaker closed this Sep 21, 2025
@RubMaker RubMaker deleted the feat-pr branch September 21, 2025 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants