bazel-contrib / bazel-contrib/rules_go
go_path: embedsrcs path normalization uses lstrip() and can silently corrupt non-generated source paths
- Dominant language
- Go
- Stars
- 1.5k
- Forks
- 760
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 12
Description
`go_path` appears to mis-handle `embedsrcs` when source paths are plain source (not generated) because it uses `lstrip(ctx.bin_dir.path + "/")` as if that were exact prefix removal.
In `go/private/tools/path.bzl`:
```starlark
dst = pkg.dir + "/" + paths.relativize(
embedpath.lstrip(ctx.bin_dir.path + "/"),
src_dir.lstrip(ctx.bin_dir.path + "/"),
)
```
`lstrip` removes a *set of leading characters*, not a literal prefix.
So this can silently alter paths when they begin with characters that are present in `ctx.bin_dir.path + "/"`, producing incorrect relative paths without throwing an error.
## Why this is a bug
Using `lstrip(prefix)` is not equivalent to removing `prefix`:
- expected behavior: remove exact `ctx.bin_dir.path + "/"` prefix only when present
- actual behavior: repeatedly strip any leading chars in that character set
That can cause malformed `dst` for `embedsrcs`, especially for plain-source paths where these values aren’t both rooted under the same generated prefix.
## Why existing tests may miss this
`tests/core/go_path/go_path_test.go` (e.g. `TestEmbedPath`) may not trigger this because the fixture’s `embedsrcs` file and `.go` source share a directory prefix (like `example.com/repo/pkg/lib/...`).
With the current code, `lstrip(ctx.bin_dir.path + "/")` can corrupt both `embedpath` and `src_dir` in the same way; then `paths.relativize(...)` may effectively cancel that shared corruption.
The bug is more likely to surface when the `go_library` sources are at the module/package root (e.g. `src_dir == "."` or otherwise lacking a shared removable prefix), where there is no common prefix distortion to cancel out.
So a fix should include a **new targeted test case** for this shape, rather than relying on existing `go_path` embed tests.
## Suggested fix
Use exact-prefix removal semantics, e.g.:
- check `startswith(prefix)` then slice by `len(prefix)`, or
- equivalent helper that removes only the exact prefix.
## Additional context
I found this while investigating `go_path` + `embedsrcs` behavior; this seems distinct from existing issues about directory embedsrcs / generated embedsrcs placement.
Contributor guide
Research direction
Start in go/private/tools/path.bzl, focusing on the embedsrcs path construction and how plain-source paths reach it. Review tests/core/go_path/go_path_test.go, especially TestEmbedPath, then add a targeted root-level source case and run the go_path tests. Done means non-generated embedsrcs paths retain their correct relative destinations without shared-prefix corruption.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- build-system
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100