mod: add local module replacement support with robust path resolution#4214
mod: add local module replacement support with robust path resolution#4214rawkode wants to merge 4 commits intocue-lang:masterfrom
Conversation
Add support for local path replacements in module.cue files, similar to Go's replace directive. This allows replacing module dependencies with local directories using paths like "./local-dep" or "../sibling". Key changes: - Add modreplace package for handling local path replacements - Add localreg.go to wrap Registry with replacement support - Update schema.cue to allow empty version for replace-only deps - Compute absolute paths at creation time instead of using os.Getwd() - Add comprehensive tests including script tests for various scenarios The path resolution now requires either: - A filesystem implementing module.OSRootFS, or - An absolute directory path This eliminates fragile reliance on os.Getwd() which could fail in containers, tests, or when the working directory changes. Signed-off-by: David Flanagan <david@rawkode.dev>
5034727 to
bde1f96
Compare
Address feedback from PR cue-lang#4214 review: - Fix misleading comment in requirements.go about local path replacement handling (local replacements are handled by localReplacementRegistry wrapper, not in cueModSummary) - Clarify schema.cue documentation: use "version-independent replacement" instead of "version is unknown" - Use module.Version.Equal() method instead of != operator in tidy.go for idiomatic comparison - Add clarifying comment explaining intentional orphan replacement preservation (matches Go's go mod tidy behavior) - Add explicit validation rejecting absolute paths in parseReplacement() with clear error messages for both Unix and Windows paths - Add test for self-referencing local replacements to verify the system handles edge cases gracefully
Address feedback from PR cue-lang#4214 review: - Fix misleading comment in requirements.go about local path replacement handling (local replacements are handled by localReplacementRegistry wrapper, not in cueModSummary) - Clarify schema.cue documentation: use "version-independent replacement" instead of "version is unknown" - Use module.Version.Equal() method instead of != operator in tidy.go for idiomatic comparison - Add clarifying comment explaining intentional orphan replacement preservation (matches Go's go mod tidy behavior) - Add explicit validation rejecting absolute paths in parseReplacement() with clear error messages for both Unix and Windows paths - Add test for self-referencing local replacements to verify the system handles edge cases gracefully Signed-off-by: David Flanagan <david@rawkode.dev>
a34a599 to
d3af39b
Compare
Address remaining PR cue-lang#4214 review feedback: - Improve Windows drive letter validation to verify the first character is actually a letter using unicode.IsLetter() (fixes false positives on paths like "9:\notpath") - Add UNC path rejection for \\server\share and //server/share paths - Add comprehensive unit tests for parseReplacement() covering valid local/remote paths, absolute path rejection, UNC paths, strict mode, and invalid module paths/versions - Add script test verifying replace directives are preserved when dependencies are removed during tidy Signed-off-by: David Flanagan <david@rawkode.dev>
mvdan
left a comment
There was a problem hiding this comment.
Did a quick pass and spotted a few issues.
Overall I notice that you're adding ~900 lines of tests, mostly unit tests, to support ~400 lines of added code. This seems like a case where the AI has gone into overdrive and covered every possible edge case. Which is not a bad thing per se, but we tend to lean towards covering the most common good and bad paths via integration tests (like testscript), and we only use unit tests where necessary, e.g. rare edge cases or for what doesn't fit testscript.
So my intuition is that the 900 lines of tests should be reduced by about half. For example:
modreplace_local_commands.txtarcan be joined withmodreplace_local.txtarmodreplace_local_nested.txtardoesn't seem to test anything new overmodreplace_local.txtar- they both replace into a module nested underneath the main one- I'm not sure that
internal/mod/modreplace/local_test.goadds much over e.g.modreplace_local_missing.txtar, at least in terms of realistic scenarios from the CLI
I also wonder if a new modreplace package is warranted - I think it's a bit premature for just 150 lines of code.
internal/mod/modfiledata/modfile.go
Outdated
| // Reject UNC paths (\\server\share or //server/share) | ||
| if len(replace) >= 2 && ((replace[0] == '\\' && replace[1] == '\\') || (replace[0] == '/' && replace[1] == '/')) { | ||
| return Replacement{}, fmt.Errorf("UNC path replacement %q not allowed; use relative path starting with ./ or ../", replace) | ||
| } |
There was a problem hiding this comment.
we have Unix and Windows path support via pkg/path - use those APIs like so:
if cuepath.IsAbs(s, cuepath.Windows) || cuepath.IsAbs(s, cuepath.Unix) {
internal/mod/modload/tidy.go
Outdated
| return slices.Equal(rs0.RootModules(), rs1RootMods) && | ||
| maps.Equal(rs0.DefaultMajorVersions(), rs1.DefaultMajorVersions()) | ||
| maps.Equal(rs0.DefaultMajorVersions(), rs1.DefaultMajorVersions()) && | ||
| equalReplacements(rs0.Replacements(), rs1.Replacements()) |
There was a problem hiding this comment.
why can't you use maps.Equal here?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: David Flanagan <david@rawkode.dev>
e68e4fa to
c0fbd95
Compare
|
I have hopefully fixed this up. If you don't think this is on the right path, I am happy to close this. |
Add support for local path replacements in module.cue files, similar to Go's replace directive. This allows replacing module dependencies with local directories using paths like "./local-dep" or "../sibling".
First pass at #2956
Key changes:
The path resolution now requires either:
This eliminates fragile reliance on os.Getwd() which could fail in containers, tests, or when the working directory changes.