Skip to content

fix(goctl): resolve pb imports across sibling modules (#5511)#5552

Open
Meng-Xin wants to merge 1 commit intozeromicro:masterfrom
Meng-Xin:fix-goctl-sibling-module-imports
Open

fix(goctl): resolve pb imports across sibling modules (#5511)#5552
Meng-Xin wants to merge 1 commit intozeromicro:masterfrom
Meng-Xin:fix-goctl-sibling-module-imports

Conversation

@Meng-Xin
Copy link
Copy Markdown

@Meng-Xin Meng-Xin commented Apr 26, 2026

Summary

Fix goctl rpc protoc import generation when --go_out / --go-grpc_out point to a sibling Go module instead of the current module.

Previously, goctl derived the generated pb import path by joining the current project module path with the pb output directory on disk. On Windows, when the pb output directory lived outside the current module, this could produce invalid imports such as:

"sales-admin/F:/coding/learn/zero-rpc/sales-center/pb/admin"

@Meng-Xin
Copy link
Copy Markdown
Author

This PR fixes the import-path resolution in `goctl` itself when pb output is generated into a sibling Go module. It is intended to support the multi-module `go.work` layout directly, rather than only relying on changing `go_package` as a workaround.

@Meng-Xin Meng-Xin changed the title fix(goctl): resolve pb imports across sibling modules fix(goctl): resolve pb imports across sibling modules (#5511) Apr 26, 2026
@kevwan kevwan added kind/bug Something isn't working area/goctl Categorizes issue or PR as related to goctl. labels Apr 30, 2026
@kevwan
Copy link
Copy Markdown
Contributor

kevwan commented Apr 30, 2026

Review

This PR fixes a real pain point in multi-module Go workspaces on Windows (#5511).

Problem: When running goctl rpc protoc from one module (sales-admin) with --go_out pointing to a sibling module's directory, goctl incorrectly prepends the current module name to the absolute path of the output directory, producing imports like:

import "sales-admin/F:/coding/learn/zero-rpc/sales-center/gen/admin"  // ❌

instead of the correct:

import "sales-center/gen/admin"  // ✅

The fix: When the pb output directory belongs to a different Go module (detected by finding a go.mod in a parent of the output path), use that module's name as the import prefix instead of the current module's name.

Code review points to verify:

  1. Does the fix correctly traverse parent directories to find go.mod? Check for proper handling of the root directory boundary.
  2. Does the path separator handling work correctly on both Windows (\) and Unix (/)? This is especially important since the bug was reported on Windows.
  3. Are there tests that run on both platforms, or at least test the path normalization logic independently?
  4. What happens when the go.mod in the output directory doesn't exist (e.g., module not yet initialized)? The fix should fall back gracefully to the previous behavior.

Overall: The approach is correct. Please ensure the cross-platform path handling is covered by tests before merging.

…utside current module

The previous implementation used strings.TrimPrefix to compute Go import
paths, which assumed all target directories reside under the current
project directory. This caused incorrect import paths when the protobuf
output directory was in a sibling module or an external location.

Introduce resolveDirPackage() which determines the correct import path by:
1. Checking if the directory is within the current module
2. Traversing parent directories to find go.mod for sibling modules
3. Falling back to the proto go_package option or directory basename

Also unify all Dir.Package assignments (including wd, call, and
getChildPackage) to use the new resolver, and harden isImportPath()
to reject Unix-style absolute paths on Windows.
@Meng-Xin Meng-Xin force-pushed the fix-goctl-sibling-module-imports branch from aa0b4a8 to f877110 Compare May 7, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/goctl Categorizes issue or PR as related to goctl. kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants