build: use loaded package metadata for assembly files - #2412
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Clean, well-motivated refactor. pkgSFiles no longer spawns a go list -json subprocess per package; instead it derives assembly files from the already-loaded pkg.OtherFiles (populated by packages.Load with the target GOOS/GOARCH). This removes a fork/exec + JSON parse per package, drops the encoding/json and os/exec imports, and simplifies selectedSFiles. Doc comments and tests were updated to match, and the tests even set an empty PATH to prove no subprocess runs.
I verified the core assumption: packages.Load with NeedFiles and the per-target GOOS/GOARCH env returns OtherFiles already filtered to the build target (confirmed internal/chacha8rand yields only chacha8_amd64.s on GOOS=linux GOARCH=amd64), so dropping the subprocess is behavior-preserving. The .s/.S extension filter added to selectedSFiles is correct since OtherFiles also contains .c/.h/etc. The new len(paths) != 0 guard on the chacha8rand branch is a small correctness improvement.
Findings are minor / non-blocking — see inline comments.
Findings without inline locations
internal/build/collect.go:200: Possible duplicate digest entries (observation, likely pre-existing).pkgSFilesnow returns paths taken directly fromp.OtherFiles, and those same.spaths are already present inotherFiles(which starts as a copy ofp.OtherFiles).digestFilesWithOverlaydoes not deduplicate, so each selected assembly file gets digested twice into the fingerprint'sOtherFiles. I believe this pre-dates the PR (the oldfilepath.Join(lp.Dir, f)produced the same absolute paths), and the result stays deterministic because the digest list is sorted — so it's harmless but redundant. Worth confirming whether the double-count is intended; if not, dedup beforedigestFilesWithOverlay.
| if len(paths) != 0 && pkg.PkgPath == "internal/chacha8rand" && pkg.Dir != "" { | ||
| stub := filepath.Join(pkg.Dir, "chacha8_stub.s") | ||
| if _, err := os.Stat(stub); err == nil { | ||
| paths := []string{stub} |
There was a problem hiding this comment.
Shadowed paths (minor). paths := []string{stub} declares a new variable that shadows the outer paths accumulator. It's correct here only because both the cache write and the return sit inside this block, but the shadow is unnecessary and forces a reader to check the scope. Prefer a plain assignment paths = []string{stub} (drop the :=) or a distinct name like stubPaths.
There was a problem hiding this comment.
Fixed in f7059c1: reuse the outer paths variable.
| // tail-jumps to block_generic and preserves package behavior. | ||
| if pkg.PkgPath == "internal/chacha8rand" && lp.Dir != "" { | ||
| stub := filepath.Join(lp.Dir, "chacha8_stub.s") | ||
| if len(paths) != 0 && pkg.PkgPath == "internal/chacha8rand" && pkg.Dir != "" { |
There was a problem hiding this comment.
Redundant pkg.Dir != "" (minor). pkgSFiles already returns early when pkg.Dir == "" (a few lines above), so pkg.Dir is guaranteed non-empty here. In the old code this clause checked lp.Dir from the JSON output (a different value); after the refactor it's a dead condition and can be dropped. A one-line comment noting that len(paths) != 0 intentionally scopes the stub override to targets that actually select chacha8 asm would also help future readers not "simplify" the guard away.
There was a problem hiding this comment.
Fixed in f7059c1: removed the redundant pkg.Dir guard and documented why the selected-path guard is intentional.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
3 participants
Summary
go list.s/.S, test-assembly, synthetic test-main, andinternal/chacha8randhandlinggo listinvocation failThis fixes the
k8s.io/client-go@v0.36.2root-package compile gap from the LLGo compatibility suite. Dependencies loaded from the module cache remain tied to the original module/workspace context, rather than being queried again from a dependency directory.Compatibility result: https://xgo-dev.github.io/benchmarks/compatibility.html
Verification
go test ./internal/build -run 'Test(SelectedSFiles|PkgSFiles|ShouldSkipPlan9AsmSFiles)' -count=1go test ./internal/build -count=1go vet ./internal/buildllgo test -short k8s.io/client-goin a module pinned tok8s.io/client-go@v0.36.2(PASS)