Conversation
Summary of ChangesHello @waruqi, 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 Xmake's packaging capabilities by integrating native support for the AppImage format. It provides a complete solution for creating portable Linux applications, including the necessary tooling detection, a dedicated packaging plugin that handles AppDir structure, desktop file generation, AppRun script creation, and specific icon requirements. This allows Xmake users to effortlessly produce universal Linux binaries, simplifying application distribution. 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.
Code Review
This pull request introduces support for packaging applications as AppImages. The implementation is well-structured, covering the creation of the AppDir, desktop file, and AppRun script. The code is generally clean and adheres to xmake's conventions. I have a couple of suggestions to enhance maintainability and flexibility.
| function _get_main_executable(package, usrdir) | ||
| local main_executable = nil | ||
| local main_executable_path = nil | ||
|
|
||
| -- try to find from targets first | ||
| for _, target in ipairs(package:targets()) do | ||
| if target:is_binary() then | ||
| main_executable = target:basename() | ||
| -- check if file exists in usr/bin | ||
| local exec_path = path.join(usrdir, "bin", main_executable) | ||
| if os.isfile(exec_path) then | ||
| main_executable_path = exec_path | ||
| break | ||
| end | ||
| end | ||
| end | ||
|
|
||
| -- fallback: find in bindir | ||
| if not main_executable_path then | ||
| local bindir = package:bindir() | ||
| if bindir and os.isdir(bindir) then | ||
| -- find executable files in bindir using os.files callback | ||
| os.files(path.join(bindir, "*"), function (file, isdir) | ||
| if not isdir and os.isfile(file) and not os.islink(file) and os.isexec(file) then | ||
| main_executable = path.filename(file) | ||
| main_executable_path = path.join(usrdir, "bin", main_executable) | ||
| if os.isfile(main_executable_path) then | ||
| return false | ||
| end | ||
| end | ||
| return true | ||
| end) | ||
| end | ||
| end | ||
|
|
||
| return main_executable, main_executable_path | ||
| end |
There was a problem hiding this comment.
The logic to find the main executable can be simplified. The current implementation is a bit complex and indirect. You can directly search for an executable file in the usr/bin directory of the AppDir. This would make the code more readable and maintainable.
function _get_main_executable(package, usrdir)
local main_executable = nil
local main_executable_path = nil
-- try to find from targets first
for _, target in ipairs(package:targets()) do
if target:is_binary() then
main_executable = target:basename()
-- check if file exists in usr/bin
local exec_path = path.join(usrdir, "bin", main_executable)
if os.isfile(exec_path) and os.isexec(exec_path) then
main_executable_path = exec_path
break
end
end
end
-- fallback: find in usr/bin
if not main_executable_path then
local bindir = path.join(usrdir, "bin")
if os.isdir(bindir) then
os.files(path.join(bindir, "*"), function (file, isdir)
if not isdir and not os.islink(file) and os.isexec(file) then
main_executable = path.filename(file)
main_executable_path = file
return false -- stop on first found
end
return true
end)
end
end
return main_executable, main_executable_path
end
| local appdescription = package:description() or "" | ||
|
|
||
| -- handle icon file | ||
| local iconname = _handle_icon(package, appdir, appname) | ||
|
|
||
| -- create desktop file | ||
| _create_desktop_file(package, appdir, appname, apptitle, appdescription, main_executable, iconname) |
There was a problem hiding this comment.
The Categories field in the generated .desktop file is hardcoded to Utility. It would be better to make this configurable through the xpack configuration, so users can specify the appropriate category for their application.
You can retrieve the categories from the package configuration here and pass it to _create_desktop_file. You will also need to update _create_desktop_file to accept and use this new parameter.
For example, in _create_desktop_file:
function _create_desktop_file(package, appdir, appname, apptitle, appdescription, main_executable, iconname, appcategories)
...
local desktop_content = string.format([[
...
Categories=%s;
]], ..., appcategories)
...
end local appdescription = package:description() or ""
local appcategories = package:get("categories") or "Utility"
-- handle icon file
local iconname = _handle_icon(package, appdir, appname)
-- create desktop file
_create_desktop_file(package, appdir, appname, apptitle, appdescription, main_executable, iconname, appcategories)
#6839