[WIP]MVP #57
[WIP]MVP #57
Conversation
Summary of ChangesHello @MeteorsLiu, 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 establishes the foundational components for 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 a significant amount of new functionality, laying the groundwork for the llar command-line tool with build, get, init, install, and tidy commands. The changes include core logic for module loading, version selection using MVS, and interaction with version control systems.
While the PR is marked as a work-in-progress and not ready for review, I've taken a look at the code and have a few high-level observations and specific suggestions that might be helpful as you continue development.
There are several places where errors are handled by panicking, which could be refactored to return errors for more idiomatic Go error handling. I've also noticed some non-deterministic behavior in version selection and opportunities to reduce code duplication.
One important point to consider is the inclusion of pkgs/gnu/sort.go, which appears to be licensed under LGPLv3+. Please ensure that this is compatible with the project's overall license.
My detailed comments are on the specific files.
cmd/llar/internal/build.go
Outdated
| var currentVersion string | ||
| for ver := range v.Dependencies { | ||
| currentVersion = ver | ||
| break | ||
| } |
There was a problem hiding this comment.
The way currentVersion is selected is non-deterministic. Iterating over a map in Go does not guarantee any specific order. If versions.json contains multiple version keys in Dependencies, this could lead to different versions being built on different runs. You should define a deterministic way to select the version, for example, by sorting the keys and picking the highest version according to some versioning scheme.
cmd/llar/internal/tidy.go
Outdated
| var currentVersion string | ||
| for ver := range v.Dependencies { | ||
| currentVersion = ver | ||
| break | ||
| } |
There was a problem hiding this comment.
Similar to build.go, the selection of currentVersion here is non-deterministic due to iterating over a map without a guaranteed order. This can lead to inconsistent behavior. Please select the version in a deterministic way.
internal/modules/resolver.go
Outdated
| func execCommand(dir, mainCmd string, subcmd ...string) []byte { | ||
| cmd := exec.Command(mainCmd, subcmd...) | ||
| cmd.Dir = dir | ||
| cmd.Stderr = os.Stderr | ||
| ret, err := cmd.Output() | ||
| if err != nil { | ||
| panic(string(ret)) | ||
| } | ||
| return ret | ||
| } |
There was a problem hiding this comment.
The execCommand function panics on error. This is generally not idiomatic Go for error handling, as it forces callers to use defer and recover. It's better to return the error and let the caller decide how to handle it. Panicking can make control flow harder to understand and debug.
| } | ||
| compare, err := formulaContext.comparatorOf(p) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
This function panics if formulaContext.comparatorOf returns an error. This can crash the program. It's better to handle the error gracefully, for example by propagating it up the call stack. Panics should be reserved for unrecoverable errors, but an error loading a comparator seems like something that could be handled. Since the cmp function signature doesn't allow returning an error, you might need to preload comparators before starting the MVS process.
cmd/llar/internal/build.go
Outdated
| ctx := context.Background() | ||
|
|
||
| builder := build.NewBuilder() | ||
| if err := builder.Init(ctx, vcs.NewGitVCS(), "https://github.com/aspect-build/llb-formulas"); err != nil { |
There was a problem hiding this comment.
The formula repository URL is hardcoded. It would be more flexible to make this configurable, for example, through a command-line flag or an environment variable. This would allow users to use their own private formula repositories.
cmd/llar/internal/install.go
Outdated
| ctx := context.Background() | ||
|
|
||
| builder := build.NewBuilder() | ||
| if err := builder.Init(ctx, vcs.NewGitVCS(), "https://github.com/MeteorsLiu/llarmvp-formula"); err != nil { |
There was a problem hiding this comment.
The formula repository URL is hardcoded. It would be more flexible to make this configurable, for example, through a command-line flag or an environment variable. This would allow users to use their own private formula repositories.
cmd/llar/internal/install.go
Outdated
| // Handle verbose output | ||
| var savedStdout, savedStderr *os.File | ||
| if !installVerbose { | ||
| // Redirect stdout/stderr for formulas | ||
| for i := range formulas { | ||
| formulas[i].SetStdout(io.Discard) | ||
| formulas[i].SetStderr(io.Discard) | ||
| } | ||
|
|
||
| // Also redirect os.Stdout/os.Stderr | ||
| savedStdout = os.Stdout | ||
| savedStderr = os.Stderr | ||
| devNull, err := os.Open(os.DevNull) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open devnull: %w", err) | ||
| } | ||
| os.Stdout = devNull | ||
| os.Stderr = devNull | ||
| defer func() { | ||
| devNull.Close() | ||
| os.Stdout = savedStdout | ||
| os.Stderr = savedStderr | ||
| }() | ||
| } |
There was a problem hiding this comment.
This block of code for redirecting stdout/stderr is duplicated in cmd/llar/internal/build.go. To improve maintainability, consider refactoring this logic into a shared helper function. This function could take a boolean verbose flag and a function to execute while output is suppressed, and it would handle the redirection and restoration of os.Stdout and os.Stderr.
cmd/llar/internal/install.go
Outdated
| } | ||
|
|
||
| // parseModuleArg parses a module argument in the form "owner/repo@version" or "owner/repo". | ||
| func parseModuleArg(arg string) (modID, version string) { |
There was a problem hiding this comment.
The function parseModuleArg is also used by the get command. It would be better to move this and other shared utility functions (like printPkgConfigInfo) into a separate util.go file within the internal package to improve code organization and avoid potential circular dependencies.
cmd/llar/internal/install.go
Outdated
| if out, err := cmd.Output(); err == nil { | ||
| if result := strings.TrimSpace(string(out)); result != "" { | ||
| fmt.Printf("%s: %s\n", pkgName, result) | ||
| } | ||
| } |
There was a problem hiding this comment.
Errors from pkg-config are silently ignored here. If pkg-config fails (e.g., the package is not found, or pkg-config itself is not installed), the error will be suppressed. This can lead to silent build failures or unexpected behavior. You should at least log the error, especially in verbose mode.
if out, err := cmd.Output(); err != nil {
if installVerbose {
fmt.Fprintf(os.Stderr, "pkg-config for %s failed: %v\n", pkgName, err)
}
} else {
if result := strings.TrimSpace(string(out)); result != "" {
fmt.Printf("%s: %s\n", pkgName, result)
}
}| return depInTable.ModuleID == dep.ModuleID | ||
| }) | ||
| if idx < 0 { | ||
| // It seems safe to drop deps here, because we resolve deps recursively and finally we will find that dep. |
There was a problem hiding this comment.
The comment here says 'It seems safe to drop deps here'. Silently dropping a dependency because its version couldn't be resolved could lead to subtle and hard-to-debug issues later in the build process. It would be safer to return an error if a dependency version cannot be resolved, or at least log a warning.
- Move Use method from ModuleF to Project - Simplify Builder by removing newRepo injection - Fix source directory and build output paths - Update tests and fixtures accordingly
1 participant
This PR is NOT considered to be merged, which is a demo to show how LLAR works and will be separated into multiple small PRs, please do not review.