feat(build): add build system with cross-process file locking #56
Conversation
MeteorsLiu
commented
Dec 17, 2025
MeteorsLiu
commented
- Implement Builder struct with Init and Build methods for formula builds
- Add lockedfile package for safe concurrent file operations
- Support platform-specific file locking (Unix, Windows, Plan9)
- Include build caching to avoid redundant builds
- Add Mutex type for cross-process synchronization
- Implement comprehensive tests for file locking behavior
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 introduces a foundational build system designed for managing and executing formula builds. It prioritizes concurrency safety and efficiency by incorporating cross-process file locking and a caching mechanism. The system is built to be robust across different operating systems and provides controlled execution environments for builds, ensuring reliable and repeatable results. 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
|
![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 new build system with cross-process file locking to ensure safe concurrent builds. The lockedfile package, which provides platform-specific file locking, is well-implemented and tested, seemingly adapted from Go's internal tooling. The overall build logic in the Builder struct correctly uses these locks to manage build caches and outputs.
However, there are several critical issues in internal/build/build.go related to the modification of process-wide state. The use of os.Chdir, modification of process environment variables (os.Setenv), and redirection of global os.Stdout/os.Stderr make the build process unsafe for concurrent execution within the same process (e.g., via goroutines). These operations can lead to race conditions and unpredictable behavior. I've provided detailed comments and suggestions to address these concurrency-related problems.
Additionally, I've found a compilation error in the test setup and some minor code style issues in the tests.
internal/build/build.go
Outdated
| return nil | ||
| } | ||
|
|
||
| if err := os.Chdir(f.Proj.BuildDir); err != nil { |
There was a problem hiding this comment.
The use of os.Chdir changes the current working directory for the entire process. This is not safe for concurrency. If multiple builds are run in parallel within the same process (e.g., in different goroutines), they will interfere with each other, leading to race conditions and incorrect build behavior. The build script execution should be configured to run in a specific directory without changing the process's global state. For example, if using os/exec, the Cmd.Dir field should be set. To make this safer even in sequential execution, you should also defer a change back to the original directory to handle errors and panics correctly.
internal/build/build.go
Outdated
| savedEnv := os.Environ() | ||
|
|
||
| // Redirect stdout/stderr if not verbose | ||
| var savedStdout, savedStderr *os.File | ||
| if !opts.Verbose { | ||
| savedStdout = os.Stdout | ||
| savedStderr = os.Stderr | ||
| devNull, err := os.Open(os.DevNull) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| os.Stdout = devNull | ||
| os.Stderr = devNull | ||
| defer func() { | ||
| devNull.Close() | ||
| os.Stdout = savedStdout | ||
| os.Stderr = savedStderr | ||
| }() | ||
| } | ||
|
|
||
| if err := f.OnBuild(f.Proj, results); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| os.Clearenv() | ||
| for _, e := range savedEnv { | ||
| if k, v, ok := strings.Cut(e, "="); ok { | ||
| os.Setenv(k, v) | ||
| } | ||
| } |
There was a problem hiding this comment.
The build logic modifies the process-wide environment variables using os.Environ, os.Clearenv, and os.Setenv. This is not safe for concurrent execution within the same process, as parallel builds would overwrite each other's environment. Environment variables should be passed directly to the child process being executed (e.g., via exec.Cmd.Env).
Furthermore, the restoration logic (lines 114-119) is not in a defer block. If f.OnBuild panics, the environment will not be restored, leaving the process in a corrupted state. This logic should be moved into a defer function to ensure it always executes.
internal/build/build.go
Outdated
| // Redirect stdout/stderr if not verbose | ||
| var savedStdout, savedStderr *os.File | ||
| if !opts.Verbose { | ||
| savedStdout = os.Stdout | ||
| savedStderr = os.Stderr | ||
| devNull, err := os.Open(os.DevNull) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| os.Stdout = devNull | ||
| os.Stderr = devNull | ||
| defer func() { | ||
| devNull.Close() | ||
| os.Stdout = savedStdout | ||
| os.Stderr = savedStderr | ||
| }() | ||
| } |
There was a problem hiding this comment.
Redirecting os.Stdout and os.Stderr affects the entire process. If multiple builds run concurrently in goroutines, their output will be unpredictably silenced or redirected, and they may interfere with each other's stream restoration. Instead of redirecting the global os.Stdout/os.Stderr, the output streams of the executed command should be controlled directly (e.g., by setting cmd.Stdout and cmd.Stderr on an exec.Cmd struct to io.Discard or a file).
internal/build/build_test.go
Outdated
| } | ||
| os.RemoveAll(formulaDir) | ||
|
|
||
| if err = os.CopyFS(formulaDir, os.DirFS("testdata")); err != nil { |
There was a problem hiding this comment.
The function os.CopyFS does not exist in the standard os package. This will cause a compilation error. You might be looking for a function to copy a directory from an fs.FS, which would typically require walking the fs.FS (e.g., with fs.WalkDir) and copying files individually, or using a helper from a third-party library.
| ) | ||
|
|
||
| type Builder struct { | ||
| initOnce sync.Once |
There was a problem hiding this comment.
The initOnce field in the Builder struct is declared but never used. If it's intended for future use, a comment explaining its purpose would be helpful. Otherwise, it should be removed to avoid dead code.
| } | ||
| } | ||
|
|
||
| func TestBuildZlib(t *testing.T) { |
There was a problem hiding this comment.
The test function TestBuildZlib also includes a sub-test for "libpng". It would be more descriptive to rename this test function to something more general, like TestBuild, to accurately reflect that it tests the build process for multiple libraries.
| t.Fatal(err) | ||
| return |
There was a problem hiding this comment.
The return statement after t.Fatal(err) is redundant because t.Fatal stops the execution of the current test goroutine. This can be removed to make the code more concise. This applies to other similar error handling blocks in this file (e.g., lines 41-42, 53-54, etc.).
| t.Fatal(err) | |
| return | |
| t.Fatal(err) |
Add Matrix struct with Require, Options, and DefaultOptions fields to support multi-dimensional build configurations. Implement methods for string representation, cartesian product combinations, and combination counting. Also update OnBuild callback to return error for better error handling.
- Add app() method to ModuleF to expose gsh.App instance - Initialize gsh.App in Gopt_ModuleF_Main entry point - Add OutputDir field to BuildResult struct - Bump github.com/qiniu/x dependency from v1.15.1 to v1.15.3
Remove unused OutputDir field from the BuildResult struct to simplify the build result representation.
- Implement Builder struct with Init and Build methods for formula builds - Add lockedfile package for safe concurrent file operations - Support platform-specific file locking (Unix, Windows, Plan9) - Include build caching to avoid redundant builds - Add Mutex type for cross-process synchronization - Implement comprehensive tests for file locking behavior
- Rename formula package import to classfile for clarity
- Relocate lockedfile to internal/lockedfile, modload to internal/modules
- Remove Init method and integrate VCS sync into Build method
- Simplify Build signature to accept pre-loaded module targets
- Remove BuildOptions and verbose output handling
- Change directory structure to use .source/ and .build/{version}/{matrix}
- Replace panic with error return for missing OnBuild handler
- Remove environment save/restore logic during build execution
- Add Result struct with OutputDir field to return build outputs - Move matrix parameter from Build() to NewBuilder() constructor - Change Build() signature to return []Result with main target at index 0 - Rename BuildDir to OutputDir in Project struct for consistency - Update tests to use new Builder API and verify returned results
- Add OutputDir field to BuildResult struct - Extend Project struct with Deps, BuildResults, and Matrix fields - Change DirFS to FileFS (ReadFileFS) for simpler file reading - Add Use() method to configure build environment for dependencies - Set PKG_CONFIG_PATH, CMAKE paths, and platform-specific compiler flags - Support both Windows (MSVC) and Unix (GCC/Autotools) environments
- Add Context struct to encapsulate build environment (matrix, source dir, build results) - Change OnBuild signature to accept Context, Project, and BuildResult parameters - Update error handling to check results.Err instead of OnBuild return value - Rename module.Version.ID to module.Version.Path for clarity - Change BuildResults from pointer to value type in maps - Rename BuildResult.OutputDir to BuildResult.Dir - Add FileFS field to Project using fs.ReadFileFS interface - Update all tests to use Path instead of ID This refactoring improves separation of concerns by introducing a Context object that carries build-related state, making the build process more flexible and testable.
1 participant
