Conversation
Summary of ChangesHello @joeykchen, 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 introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the spxrun command, a runner for SPX 2.0 projects. The implementation is well-structured, handling runtime downloads, project building, and execution. My review focuses on improving portability and adhering to security best practices. I've suggested replacing an external command dependency with a pure Go solution and adjusting file permissions to be more secure. Overall, these are solid additions to the project.
| func copyFile(src, dst string) error { | ||
| input, err := os.ReadFile(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(dst, input, 0755) | ||
| } |
There was a problem hiding this comment.
The copyFile function currently sets file permissions to 0755, which makes the destination file executable by everyone. This is not always necessary and violates the principle of least privilege. For example, Go source files or configuration files copied with this function do not need to be executable.
It's better to use more restrictive permissions like 0644 for regular files. Any file that needs to be executable can have its permissions changed explicitly using os.Chmod, as is already being done for the runtime executable in ensureRuntime.
| func copyFile(src, dst string) error { | |
| input, err := os.ReadFile(src) | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(dst, input, 0755) | |
| } | |
| func copyFile(src, dst string) error { | |
| input, err := os.ReadFile(src) | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(dst, input, 0644) | |
| } |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| if err := downloadFile(url, tmpZip); err != nil { | ||
| return fmt.Errorf("failed to download runtime: %w", err) | ||
| } | ||
| defer os.Remove(tmpZip) |
There was a problem hiding this comment.
🔴 Critical: Resource leak from misplaced defer
The defer os.Remove(tmpZip) is placed AFTER the error check, meaning if downloadFile fails, the temporary file won't be cleaned up.
Impact: Repeated failures will accumulate temporary files in $GOPATH/bin.
Fix: Move defer immediately after successful download, or clean up explicitly on error:
if err := downloadFile(url, tmpZip); err != nil {
os.Remove(tmpZip) // Clean up on error
return fmt.Errorf("failed to download runtime: %w", err)
}
defer os.Remove(tmpZip)Same issue at line 261 in downloadRuntimePck().
cmd/spxrun/runner/runner.go
Outdated
| tidyCmd.Dir = r.GoDir | ||
| tidyCmd.Stdout = os.Stdout | ||
| tidyCmd.Stderr = os.Stderr | ||
| tidyCmd.Run() // Ignore error, may not have go.mod |
There was a problem hiding this comment.
🟠 Missing error handling
Silently ignoring ALL errors from go mod tidy makes debugging difficult. Users won't know if there's a legitimate issue vs. missing go.mod.
Recommendation: Log the error with context:
if err := tidyCmd.Run(); err != nil {
fmt.Printf("Warning: go mod tidy failed (this may be normal if go.mod doesn't exist): %v\n", err)
}
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(dst, input, 0755) |
There was a problem hiding this comment.
🟠 Security: Incorrect file permissions
The generic copyFile() makes ALL copied files executable (0755), including text configs and .pck files. Only binaries and libraries should be executable.
Fix: Preserve source file permissions:
func copyFile(src, dst string) error {
input, err := os.ReadFile(src)
if err != nil {
return err
}
srcInfo, err := os.Stat(src)
if err != nil {
return err
}
return os.WriteFile(dst, input, srcInfo.Mode())
}Additionally, this loads entire files into memory. For large runtime files (50-100MB), use streaming copy with io.Copy instead.
cmd/spxrun/runner/runner.go
Outdated
| defer os.RemoveAll(tmpDir) | ||
|
|
||
| // Run unzip command | ||
| cmd := exec.Command("unzip", "-o", zipPath, "-d", tmpDir) |
There was a problem hiding this comment.
🔴 Security: Command injection risk + portability issue
- Portability: Windows users without Git Bash won't have
unzip, causing cryptic failures - Security: Passing file paths to shell commands without validation could enable command injection if input sources change
Recommendation: Use Go's built-in archive/zip package:
func extractFileFromZip(zipPath, fileName, destPath string) error {
r, err := zip.OpenReader(zipPath)
if err != nil {
return fmt.Errorf("failed to open zip: %w", err)
}
defer r.Close()
for _, f := range r.File {
if f.Name == fileName || strings.HasSuffix(f.Name, fileName) {
// Validate path to prevent traversal
if strings.Contains(f.Name, "..") {
return fmt.Errorf("invalid file path in zip")
}
rc, err := f.Open()
if err != nil {
return err
}
defer rc.Close()
out, err := os.Create(destPath)
if err != nil {
return err
}
defer out.Close()
_, err = io.Copy(out, rc)
return err
}
}
return fmt.Errorf("file %s not found in zip", fileName)
}
| // Build for each architecture | ||
| for _, arch := range archs { | ||
| libPath := r.getLibPathForArch(arch) | ||
| fmt.Printf("Building for %s/%s -> %s\n", r.GOOS, arch, libPath) | ||
|
|
||
| // Set environment variables | ||
| env := append(os.Environ(), | ||
| "CGO_ENABLED=1", | ||
| "GOARCH="+arch, | ||
| ) | ||
|
|
||
| // Build command | ||
| args := []string{ | ||
| "build", | ||
| "-buildmode=c-shared", | ||
| "-o", libPath, | ||
| } | ||
|
|
||
| cmd := exec.Command("go", args...) | ||
| cmd.Env = env | ||
| cmd.Dir = r.GoDir | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
|
|
||
| fmt.Printf("Running: CGO_ENABLED=1 GOARCH=%s go %s\n", arch, strings.Join(args, " ")) | ||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("build failed for %s: %w", arch, err) | ||
| } | ||
|
|
||
| // Update LibPath if this is the current architecture | ||
| if arch == r.GOARCH { | ||
| r.LibPath = libPath | ||
| } | ||
| } |
There was a problem hiding this comment.
⚡ Performance: Serial architecture builds
On macOS, building for arm64 and amd64 sequentially doubles build time unnecessarily. Modern Macs can handle parallel CGO builds.
Estimated improvement: 40-50% faster builds on macOS (30s instead of 60s for typical projects).
Recommendation: Use goroutines with sync.WaitGroup for parallel builds:
var wg sync.WaitGroup
errChan := make(chan error, len(archs))
for _, arch := range archs {
wg.Add(1)
go func(arch string) {
defer wg.Done()
// Build logic here
}(arch)
}
wg.Wait()
close(errChan)
for err := range errChan {
if err != nil {
return err
}
}
| func downloadFile(url, dest string) error { | ||
| fmt.Printf("Downloading: %s\n", url) | ||
|
|
||
| resp, err := http.Get(url) |
There was a problem hiding this comment.
🔴 Security + Performance: Missing timeout and integrity verification
Critical security issue: Downloaded executables have no checksum verification. An attacker via MITM or compromised GitHub could serve malicious code.
Performance issue: No timeout means downloads can hang indefinitely.
Recommendations:
- Add timeout with context:
client := &http.Client{Timeout: 5 * time.Minute}
resp, err := client.Get(url)- Add checksum verification (critical for security):
const RuntimeChecksums = map[string]string{
"macos-x86_64.zip": "abc123...",
"linux-x86_64.zip": "def456...",
}
func verifyChecksum(filepath, expectedHash string) error {
f, err := os.Open(filepath)
if err != nil {
return err
}
defer f.Close()
h := sha256.New()
if _, err := io.Copy(h, f); err != nil {
return err
}
actualHash := hex.EncodeToString(h.Sum(nil))
if actualHash != expectedHash {
return fmt.Errorf("checksum mismatch: expected %s, got %s", expectedHash, actualHash)
}
return nil
}
cmd/spxrun/runner/runner.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| // Version is the SPX runtime version |
There was a problem hiding this comment.
📝 Documentation: Misleading comment
This is the Godot runtime version tag, not the SPX runtime version. The implementation shows it's used in RuntimeURLBase + "spx" + Version.
Suggested fix:
// Version is the Godot runtime version tag for downloading releases
// Used in URL: https://github.com/goplus/godot/releases/download/spx{VERSION}/...
Version = "2.1.36"
| func (r *Runner) buildLibrary() error { | ||
| fmt.Println("Building dynamic library...") | ||
|
|
||
| // Ensure lib directory exists | ||
| if err := os.MkdirAll(r.LibDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create lib directory: %w", err) | ||
| } | ||
|
|
||
| // Ensure go directory exists | ||
| if err := os.MkdirAll(r.GoDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create go directory: %w", err) | ||
| } | ||
|
|
||
| // Always regenerate Go code from .spx files (project may have changed) | ||
| fmt.Println("Generating Go code with xgo...") | ||
| if err := r.generateGoCode(); err != nil { | ||
| return fmt.Errorf("failed to generate Go code: %w", err) | ||
| } |
There was a problem hiding this comment.
🔧 Code Quality: Function complexity
The buildLibrary() function handles too many responsibilities (91 lines):
- Directory creation
- Go code generation
- File validation/copying
- Working directory management
- go mod tidy
- Multi-architecture compilation
- Library path tracking
Impact: Hard to test, debug, and maintain.
Recommendation: Break into focused functions:
func (r *Runner) buildLibrary() error {
if err := r.prepareDirectories(); err != nil {
return err
}
if err := r.generateAndCopyGoCode(); err != nil {
return err
}
return r.compileLibraries()
}
Code Review SummaryComprehensive review completed using specialized agents. The PR adds a well-structured spxrun command, but several critical issues need attention before merge. 🔴 Critical Issues (Must Fix)
🟠 High Priority
⚡ Performance Opportunities
✅ Positive AspectsWell-organized code structure, good error wrapping, progress tracking for downloads, and comprehensive platform support. See inline comments for details and suggested fixes. |
joeykchen
force-pushed
the
pr_xgo_run
branch
2 times, most recently
from
8e1067b to
5e14879
Compare
|
@joeykchen Better to integrate it into |
joeykchen
force-pushed
the
pr_xgo_run
branch
from
206e145 to
e1edf2c
Compare
ok， after： |
Refactor : Do Not Create A Temporary Directory And Walking It Rename JSFuncOfWithError Add SPX Runtime Version Makefile Support Genarate Template Remove xgo_autogen.go From Project Directory To Keep It Clean Refactor Ensure GopMod For Xgo Run
joeykchen
force-pushed
the
pr_xgo_run
branch
from
e1edf2c to
e687575
Compare
| @@ -0,0 +1,7 @@ | |||
| gop 1.3 | |||
There was a problem hiding this comment.
rename gop 1.3 => xgo 1.3
rename gop.mod => gox.mod
|
|
||
| "github.com/goplus/spx/v2/cmd/spxrun/runner" | ||
| ) | ||
|
|
There was a problem hiding this comment.
是否还要考虑命令行参数支持，比如 -f 表示 full screen 等。
3 participants
No description provided.