[WIP]feat:LLGo’s distribution module for Python(bundle) #1307
Conversation
PR Review for Python Bundling FeatureTodo Checklist:
Code Review SummaryThis PR implements a comprehensive Python bundling feature for LLGo that enables packaging executable artifacts with embedded Python runtime. The implementation addresses the requirements outlined in issue #1306 well, but there are several areas for improvement. 🎯 Overall Architecture AssessmentStrengths:
Areas for Improvement:
🐛 Critical Issues1. Linux Support Not Implemented (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @1351914167, 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 LLGo's distribution capabilities by introducing a new bundle command. This command allows developers to package their LLGo applications, along with their Python runtime dependencies, into either a self-contained directory or a single executable file. The changes streamline the process of setting up the Python environment, managing Python package installations, and ensuring that the Python interpreter is correctly initialized at runtime, thereby making LLGo applications more portable and easier to deploy.
Highlights
- New
llgo bundleCommand: Introduces a newllgo bundlecommand to create self-contained distributions of LLGo applications with embedded Python runtimes. - Dual Bundling Modes: Supports two primary bundling modes:
onedirfor a directory-based distribution andonefilefor a single self-extracting executable. - Automated Python Environment Management: Includes functionality to automatically download, extract, and configure the necessary Python runtime environment, ensuring portability.
- Integrated Python Package Installation: Enables automatic installation of required Python packages via
pipduring the build process, simplifying dependency management. - Dynamic Python Runtime Initialization: Embeds C code to dynamically initialize the Python interpreter at runtime, correctly handling
PYTHONHOMEandlibpythonpaths relative to the executable. - Archiving Options: Provides options to archive
onedirbundles intozip,tar.gz, orrarformats for easier distribution.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new bundle command to llgo for packaging executables with an embedded Python runtime. This is a significant feature addition, implemented through a new bundle command package and a comprehensive pyenv internal package that handles Python environment fetching, building, and bundling. The changes are extensive and well-structured. My review focuses on improving robustness, configuration, and maintainability. I've identified a few areas for improvement, including handling of environment variables for configuration, making Python version strings dynamic, improving error handling, and clarifying some implementation details. Overall, this is a great step towards better Python integration.
| func PythonHome() string { | ||
| // if pyHome := os.Getenv("PYTHONHOME"); pyHome != "" { | ||
| // return pyHome | ||
| // } | ||
| return filepath.Join(env.LLGoCacheDir(), "python_env", "python") | ||
| } |
There was a problem hiding this comment.
The implementation of PythonHome is hardcoded and does not respect the LLPYG_PYHOME environment variable, which is mentioned in comments elsewhere in the codebase (e.g., internal/build/build.go). This prevents users from configuring a custom Python installation. The commented-out code should be implemented to allow for this configuration.
| func PythonHome() string { | |
| // if pyHome := os.Getenv("PYTHONHOME"); pyHome != "" { | |
| // return pyHome | |
| // } | |
| return filepath.Join(env.LLGoCacheDir(), "python_env", "python") | |
| } | |
| func PythonHome() string { | |
| if pyHome := os.Getenv("LLPYG_PYHOME"); pyHome != "" { | |
| return pyHome | |
| } | |
| return filepath.Join(env.LLGoCacheDir(), "python_env", "python") | |
| } |
| // // Optional marker to indicate successful initialization | ||
| // _ = os.WriteFile(filepath.Join(tmp, ".init_ok"), []byte(time.Now().Format(time.RFC3339)), 0o644) | ||
| // if err := os.Rename(tmp, dir); err != nil { | ||
| // _ = os.RemoveAll(tmp) | ||
| // // If another process won the race, treat as success | ||
| // if st, err2 := os.Stat(dir); err2 == nil && st.IsDir() { | ||
| // return nil | ||
| // } | ||
| // return fmt.Errorf("rename temp to final: %w", err) | ||
| // } |
There was a problem hiding this comment.
The core logic of ensureDirAtomic, which performs an atomic rename to create the directory, is commented out. As it stands, the function's name is misleading because it only creates a temporary directory and doesn't ensure the final directory exists atomically. This can lead to race conditions and confusing behavior in EnsureWithFetch. Please uncomment the rename logic to fix the function's behavior.
| // // Optional marker to indicate successful initialization | |
| // _ = os.WriteFile(filepath.Join(tmp, ".init_ok"), []byte(time.Now().Format(time.RFC3339)), 0o644) | |
| // if err := os.Rename(tmp, dir); err != nil { | |
| // _ = os.RemoveAll(tmp) | |
| // // If another process won the race, treat as success | |
| // if st, err2 := os.Stat(dir); err2 == nil && st.IsDir() { | |
| // return nil | |
| // } | |
| // return fmt.Errorf("rename temp to final: %w", err) | |
| // } | |
| // Optional marker to indicate successful initialization | |
| // _ = os.WriteFile(filepath.Join(tmp, ".init_ok"), []byte(time.Now().Format(time.RFC3339)), 0o644) | |
| if err := os.Rename(tmp, dir); err != nil { | |
| _ = os.RemoveAll(tmp) | |
| // If another process won the race, treat as success | |
| if st, err2 := os.Stat(dir); err2 == nil && st.IsDir() { | |
| return nil | |
| } | |
| return fmt.Errorf("rename temp to final: %w", err) | |
| } |
cmd/internal/bundle/bundle.go
Outdated
|
|
||
| // llgo bundle | ||
| var Cmd = &base.Command{ | ||
| UsageLine: "llgo bundle [-mode onedir|onefile] [-out output] [-archive zip|rar|tar] [-archiveOut file] [packages]", |
There was a problem hiding this comment.
The usage line for the -mode flag is inconsistent. It states onedir|onefile, but the flag definition and the implementation use dir|exe. To avoid confusion for users, the usage line should be updated to reflect the actual accepted values.
| UsageLine: "llgo bundle [-mode onedir|onefile] [-out output] [-archive zip|rar|tar] [-archiveOut file] [packages]", | |
| UsageLine: "llgo bundle [-mode dir|exe] [-out output] [-archive zip|rar|tar] [-archiveOut file] [packages]", |
| if wd, err2 := os.Getwd(); err2 == nil { | ||
| workDir = wd | ||
| } |
There was a problem hiding this comment.
The error err2 from os.Getwd() is not handled. If os.Stat(pkg) fails and os.Getwd() also fails, workDir will remain an invalid path. This will lead to a failure later in the build process with a potentially confusing error message. It's better to handle this error explicitly and exit.
if wd, err2 := os.Getwd(); err2 == nil {
workDir = wd
} else {
fmt.Fprintln(os.Stderr, "failed to get working directory:", err2)
os.Exit(1)
}| args = append(args, ctx.crossCompile.CCFLAGS...) | ||
| args = append(args, ctx.crossCompile.CFLAGS...) | ||
|
|
||
| inc := filepath.Join(pyenv.PythonHome(), "include", "python3.12") |
There was a problem hiding this comment.
The Python version 3.12 is hardcoded here. This could lead to issues if the Python version defined in internal/pyenv/pyenv.go is updated. Consider deriving this from a constant or a function in the pyenv package to ensure consistency. For example, you could add a helper function in pyenv that returns the major.minor version string.
| _ = exec.Command("install_name_tool", "-id", "@rpath/"+filepath.Base(libDst), libDst).Run() | ||
| } | ||
|
|
||
| // 2.1) 复制 PYHOME/lib 下其它所有动态库到 <exe_dir>/lib/python/lib |
There was a problem hiding this comment.
This comment is in Chinese, while the rest of the codebase is in English. For consistency and maintainability, please translate it to English.
| // 2.1) 复制 PYHOME/lib 下其它所有动态库到 <exe_dir>/lib/python/lib | |
| // 2.1) Copy all other dynamic libraries from PYHOME/lib to <exe_dir>/lib/python/lib |
| return nil | ||
| } | ||
| py := filepath.Join(pyHome, "bin", "python3") | ||
| site := filepath.Join(pyHome, "lib", "python3.12", "site-packages") |
There was a problem hiding this comment.
The Python version 3.12 is hardcoded in the site-packages path. This should be derived from a constant or function in the pyenv package to avoid inconsistencies when the Python version is updated, similar to the issue in internal/build/build.go.
cmd/llgo/bundle_cmd.gox
Outdated
|
|
||
| run args => { | ||
| self.Cmd.Run self.Cmd, args | ||
| } No newline at end of file |
cmd/internal/bundle/bundle.go
Outdated
|
|
||
| // llgo bundle | ||
| var Cmd = &base.Command{ | ||
| UsageLine: "llgo bundle [-mode onedir|onefile] [-out output] [-archive zip|rar|tar] [-archiveOut file] [packages]", |
| initial = newInitial | ||
| } | ||
| } | ||
|
|
internal/build/build.go
Outdated
| // need to be linked with external library | ||
| // format: ';' separated alternative link methods. e.g. | ||
| // link: $LLGO_LIB_PYTHON; $(pkg-config --libs python3-embed); -lpython3 | ||
| if pkg.Name == "py" { |
There was a problem hiding this comment.
通过 pkg.Name 为 "py" 识别可能会误判，可以通过 pkg.PkgPath 来判断是否要加载 github.com/goplus/lib/py 库。只有在 expdArgs 无法获取 python3 lib 时才需要启用 python 环境准备工具，包括之后的 rpath 修改等，否则可以使用系统默认安装的 python 库。
There was a problem hiding this comment.
已在最新的提交中改为 if pkg.PkgPath == "github.com/goplus/lib/py"
若用户在无 site-package 安装的情况下，编译流程不会对用户环境做额外更改。 但 pkgs 的遍历顺序为 lib/c lib/py lib/py/numpy, 在遍历到 lib/py 时会做当前环境准备， 此时并不能判断是否需要安装 site-package， 如计划使用本地环境，可能需要对当前编译流程中的包遍历逻辑做修改
internal/pyenv/pybuild.go
Outdated
| parent_dir(d2); // d2 = dirname(d1) | ||
| char home[PATH_MAX]; snprintf(home, sizeof(home), "%s", d2); | ||
| fprintf(stderr, "[llgo] PYTHONHOME=%s\n", home); |
internal/pyenv/pybuild.go
Outdated
|
|
||
| func FindPythonRpaths(pyHome string) []string { | ||
| return []string{filepath.Join(pyHome, "lib")} | ||
| // dedup := func(ss []string) []string { |
| addRpath(&linkArgs, dir) | ||
| } | ||
|
|
||
| addRpath(&linkArgs, "@executable_path/python/lib") |
There was a problem hiding this comment.
这个 rpath 的加入只对于打包的方式有用，可以将其移动到 bundle 过程中。
| } | ||
| *args = append(*args, flag) | ||
| } | ||
| for _, dir := range pyenv.FindPythonRpaths(pyenv.PythonHome()) { |
There was a problem hiding this comment.
只有使用了 pyenv 的时候才需要做处理，如果使用系统 python 不需要处理。
| } | ||
|
|
||
| func onefileBootMain() string { | ||
| return `package main |
There was a problem hiding this comment.
对于嵌入的源文件可以使用 embed 方式来加载，以方便编辑和测试。
| }) | ||
| } | ||
|
|
||
| func archiveRar(root, dst string) error { |
| } | ||
| } | ||
|
|
||
| func archiveZip(root, dst string) error { |
There was a problem hiding this comment.
archiveZip 和 archiveTarGz 实际工作方式相似。可以考虑对两个方法重构。
Detailed in #1306