feat: add module loading with dependency resolution #94
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 introduces a foundational module loading system for the project, enabling the dynamic retrieval and resolution of module dependencies. By integrating the MVS algorithm, it ensures that a consistent and minimal set of module versions is selected, preventing dependency conflicts. The 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. Changelog
Activity
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 module loading system with MVS-based dependency resolution. However, it contains critical security vulnerabilities: the tidy function incorrectly writes to the current working directory, potentially allowing arbitrary file overwrites, and os.MkdirTemp is susceptible to path traversal. Additionally, the code has issues with panics on recoverable errors, hardcoded values (like "github.com"), and improper context handling (context.TODO()). These issues need to be addressed to improve the tool's robustness and security.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 70.24% 73.51% +3.27%
==========================================
Files 26 27 +1
Lines 1099 1314 +215
==========================================
+ Hits 772 966 +194
- Misses 259 270 +11
- Partials 68 78 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/modules/load.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| current := depTable.Dependencies[mod.Version] |
There was a problem hiding this comment.
The current variable is named current, but in fact it represents the dependency list of a certain library under a specific version, so this variable name may not be very appropriate.
internal/modules/load_test.go
Outdated
| func TestResolveDeps_TransitiveDeps(t *testing.T) { | ||
| // depmod has deps: leafmod@1.0.0 | ||
| modFS := os.DirFS("testdata/load/towner/depmod").(fs.ReadFileFS) | ||
| frla := &formula.Formula{ | ||
| ModPath: "towner/depmod", | ||
| FromVer: "1.0.0", | ||
| } | ||
| mod := module.Version{Path: "towner/depmod", Version: "1.0.0"} | ||
|
|
||
| deps, err := resolveDeps(mod, modFS, frla) | ||
| if err != nil { | ||
| t.Fatalf("resolveDeps failed: %v", err) | ||
| } | ||
| if len(deps) != 1 { | ||
| t.Fatalf("expected 1 dep, got %d: %v", len(deps), deps) | ||
| } | ||
| if deps[0].Path != "towner/leafmod" || deps[0].Version != "1.0.0" { | ||
| t.Errorf("dep = %v, want {towner/leafmod 1.0.0}", deps[0]) | ||
| } | ||
| } |
There was a problem hiding this comment.
seem same with TestResolveDeps_NoOnRequire_DepsFromVersionsJson
|
/review |
Add module loading functionality with MVS-based dependency resolution. Includes the Load function for loading packages and resolving their dependencies, version handling, and test data for various module loading scenarios.
Add concurrency safety to the modules package by: - Replace map-based moduleCache with sync.Map for thread-safe access - Introduce loadMu mutex to serialize ixgo interpreter loading to prevent internal race conditions during concurrent operations - Add mutex protection (m.mu) for formula map access within formulaModule - Refactor comparator loading using sync.OnceValues for lazy initialization - Extract loadOrDefaultComparator function for cleaner comparator handling These changes prevent data races when Load() and formula.LoadFS are called concurrently, as the ixgo interpreter has internal race conditions that require serialized access.
- Replace context.TODO() with the provided ctx parameter to respect the caller's context - Remove hardcoded context.WithTimeout wrapper that was constraining operations to 10 minutes - Remove unused time import - Simplify context propagation throughout the Load function
- Add documentation for exported Module struct - Fix Options comment: LoadPackages -> Load - Fix FormulaStore field comment: FormulaRepo -> FormulaStore - Fix Load function comment: LoadPackages -> Load, formulas -> modules Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: MeteorsLiu <17515813+MeteorsLiu@users.noreply.github.com>
MeteorsLiu
force-pushed
the
feat/modules-load
branch
from
6b42609 to
b76bdfc
Compare
|
/review |
Code Review SummaryThe module loading and dependency resolution implementation is well-structured with good test coverage (including diamond dependency and edge case tests). The use of Key concerns:
|
…commit The moduleOf closure uses FormulaStore.ModuleFS which syncs with an empty ref, meaning formulas are always fetched from the latest commit on the default branch of the formula repository. Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
Move the comment directly onto the ModuleFS call as a simple inline note indicating it always fetches from the latest commit. Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: MeteorsLiu <17515813+MeteorsLiu@users.noreply.github.com>
Replace O(n log n) sort + first element with O(n) slices.MaxFunc to find the latest version tag. Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: xgopilot <noreply@goplus.org> Co-authored-by: MeteorsLiu <17515813+MeteorsLiu@users.noreply.github.com>
internal/modules/load.go
Outdated
| if mod.Path == main.Path && mod.Version == main.Version { | ||
| deps = modules[1:] | ||
| } else { | ||
| reqs, err := mvs.Req(module.Version{mod.Path, mod.Version}, []string{}, reqs) |
There was a problem hiding this comment.
There is a correctness issue in the fill the deps logic.
BuildList already represents the final version selection after MVS resolution (i.e., the global minimal version selection result). However, when rebuilding each module’s dependencies using mvs.Req(mod, ..., reqs), we are retrieving the module’s direct requirements as declared in its own module, not the versions selected in the final BuildList.
As a result, the reconstructed Deps may contain versions that differ from (and are lower than) the versions chosen by MVS in the final build list, leading to an inconsistent dependency graph.
For example:
Declared dependencies:
A -> B@1.0, C@1.0
B@1.0 -> Z@1.0
C@1.0 -> Z@1.1Final MVS result (BuildList):
A
├─ B@1.0
├─ C@1.0
└─ Z@1.1Reconstructed deps:
A
├─ B@1.0
│ └─ Z@1.0 <- obtained from Req(B@1.0), not aligned with BuildList
└─ C@1.0
└─ Z@1.1Here, B’s dependency on Z is filled as Z@1.0, even though the final selected version in BuildList is Z@1.1.
If Module.Deps is intended to represent the actual resolved dependency graph, then the dependencies returned by Req() should be normalized/projected to the versions selected in BuildList. Otherwise, the constructed graph does not accurately reflect the resolved build state.
There was a problem hiding this comment.
zzy0 versions.json
{
"path": "towner/zzyA",
"deps": {
"1.0.0": [
{
"path": "towner/zzyA",
"version": "1.0.0"
},
{
"path": "towner/zzyB",
"version": "1.0.0"
}
]
}
}zzyA versions.json
{
"path": "towner/zzyA",
"deps": {
"1.0.0": [
{
"path": "towner/leafmod",
"version": "1.0.0"
}
]
}
}zzyB version.json
{
"path": "towner/zzyB",
"deps": {
"1.0.0": [
{
"path": "towner/leafmod",
"version": "2.0.0"
}
]
}
}the buildlist is
and the zzyA 's depend is towner/leafmod v1.0.0 when fill the deps
There was a problem hiding this comment.
Yeah, that's a problem. When we try to fill the dependencies of a sub-dependency, using the sub-dependency as the root and calling mvs.Reqs, we might end up with the dependency graph of the sub-dependency. However, what we actually need is a pruned version of the full dependency graph, with the sub-dependency as the root.
…ph and add conflict tests - Introduce an mvs.Graph in Load and sync dependency edges via graph.Require during onLoad - Replace non-main module Deps reconstruction with BFS from the module root - Normalize RequiredBy edges to computed versions using graph.Selected(path) - Reverse collected results to keep the current sub-dependency output order contract - Strengthen Diamond assertions to verify depmod/altdep both resolve to leafmod@2.0.0 - Add deep conflict fixtures and tests (deepmain/deepb/deepd/deepc/deepe) to cover convergence to deepc@1.1.0 and deepe@1.3.0 - Switch dependency assertions to ordered checks with slices.Equal ensures version consistency across the dependency tree. Key changes: - Initialize MVS graph with comparator and main dependencies - Populate graph in onLoad callback with thread-safe locking - Replace mvs.Req() with BFS-based RequiredBy() and Selected() traversal - Fix latestVersion to use MaxFunc instead of SortFunc - Add test helpers and enhance diamond dependency test coverage
…Require panics
- Add `depLoadCache` in `Load` to memoize per-module dependency results inside `onLoad`
- Return cached deps when a module is requested again (e.g. during `tidy -> mvs.Req -> BuildList`)
- Keep `graph.Require` execution to the first load of each module, preventing:
`panic: requirements of {<mod> <ver>} have already been set`
- Preserve existing dependency resolution and graph traversal behavior
Context:
- `mvs.Graph.Require` is single-write per node; repeated calls for the same module panic by design.
- `Load` may trigger multiple MVS traversals in one call path when `Tidy` is enabled.
This commit: - Adds new tests for CMakeLists.txt parsing via OnRequire callback - Includes tests for successful dependency extraction and missing CMakeLists.txt handling - Removes obsolete TestResolveDeps_TransitiveDeps test - Consolidates TestLoad_ModuleFields into TestLoad_FormulaContent - Adds module path sorting verification in TestLoad_ChainDeps - Imports classfile package for Project type used in new tests
Improve code clarity by using a more descriptive variable name that explicitly indicates the variable contains versioned dependencies from the dependency table. This makes the code more maintainable and easier to understand at a glance.
…text.TODO() Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: MeteorsLiu <17515813+MeteorsLiu@users.noreply.github.com>
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: MeteorsLiu <17515813+MeteorsLiu@users.noreply.github.com>
Track visited modules to prevent cycles in BFS traversal and ensure the dependency graph uses the MVS-selected version of each module rather than the originally required version. This prevents missing dependencies that may only appear in higher selected versions. Add comprehensive test case demonstrating the scenario where MVS selects a higher version with different dependencies than originally required.
| } | ||
| var deps classfile.ModuleDeps | ||
|
|
||
| frla.OnRequire(proj, &deps) |
There was a problem hiding this comment.
maybe we can test the Load work expect at this case
Add TestIntegration_LoadCMakeListsDeps to verify that modules can correctly parse CMakeLists.txt files through OnRequire handlers, resolve find_package dependencies, and load them into the build list. This integration test ensures that the CMake-based dependency discovery workflow functions as expected when loading real module formulas.
3 participants
Add module loading functionality with MVS-based dependency resolution. Includes the Load function for loading packages and resolving their dependencies, version handling, and test data for various module loading scenarios.