Add genception, a Bazel-backed go/packages driver - #17307
Conversation
ecc9798 to
6b385fa
Compare
1a78c8b to
730d29e
Compare
6b385fa to
6a0fb66
Compare
730d29e to
346a8de
Compare
6a0fb66 to
75b7e27
Compare
beba51a to
56e6c00
Compare
75b7e27 to
c10d031
Compare
56e6c00 to
a67a389
Compare
184d1eb to
ec17e1a
Compare
a67a389 to
e3b9bf1
Compare
5c101d4 to
0440be1
Compare
| } | ||
| bi := 0 | ||
| for i := range a { | ||
| if a[i] == b[bi] { |
There was a problem hiding this comment.
Will panic if len(b) == 0.
There was a problem hiding this comment.
You're right, I mentally inverted what the check at the top of function did (and did so again when checking out your comment). I think it's not possible in practice, since b is the result of reading the package manifest, which can be assumed to not have an empty file list in practice because of how it is constructed, but doesn't hurt to be safe.
| // extract test sources and imports into their own flatPackage | ||
| testFp := pkg.deriveTestPackage() | ||
| if testFp != nil { | ||
| r.packages[testFp.ID] = testFp |
There was a problem hiding this comment.
Items of r.packages are added while iterating on r.packages itself.
Could it cause an issue here?
There was a problem hiding this comment.
Hmm yeah, it's pseudorandom whether we visit the test package in the iterator (implementation detail in go's spec, but iterating a sparse bucket slice in practice). If we do visit one of these added packages, things could get weird since we're constructing a package name derived from the package containing the tests. Usually we always name an external test package package foo_test when in the same directory as package foo, but if we 1) had package bar_test in package foo and 2) get unlucky hash dice rolls on the map key bucket on every iteration such that keys keep landing after the previous iteration, I guess you could fail to terminate, or terminate after building a giant map that fills the disk lol. Insanely unlikely scenario but easy enough to just stick these in a separate slice and merge them at the end.
| } | ||
|
|
||
| if key := stdlibId(imp); key != "" { | ||
| fp.Imports[imp] = key |
There was a problem hiding this comment.
Is it guaranteed that fp.Imports is always non-nil?
What if flatPackage decodes from a JSON with no Imports key?
There was a problem hiding this comment.
IIRC Imports is always output as an empty dict {} due to how the starlark side of things is coded, but it's easy enough to check+initialize so you don't have to trust the implicit assumption / lore.
| internalTests, externalTests, nonTests, err := fp.groupTestFiles(fp.GoFiles) | ||
| if err != nil { | ||
| log.WithError(err).WithField("package", fp.PkgPath).Warn("unable to group test files; skipping test package derivation") | ||
| return nil |
There was a problem hiding this comment.
If we are here, fp.GoFiles is not modified.
==> The package mixes package xxx and package xxx_test files.
There was a problem hiding this comment.
If we are here, fp.GoFiles is not modified.
Right, this error handling path represents a situation where go's parser tried to parse a file, only up to package foo, and couldn't parse the package name out for some reason (for instance, an empty file). If such a file is present in a package then the package registry just gives up trying to deal with that package.
Previously I thought about this as a choice between the build failing dramatically or just giving up and letting the test files (potentially in another package) persist, since I don't think it would usually break anything. But looking at this again the simplest choice seems like filtering out the file that couldn't be parsed from all lists. I'll do that instead.
| if err != nil { | ||
| log.WithError(err).Error("unable to handle driver request") | ||
| } | ||
| _, err = out.Write(resp) |
There was a problem hiding this comment.
The pd.Handle error is overwritten here.
==> The process exits with 0 code with 0 written bits (but with an error log).
prestonvanloon
left a comment
There was a problem hiding this comment.
LGTM - Manu had some good feedback though
Code generators that resolve types through golang.org/x/tools/go/packages cannot run inside the Bazel sandbox: the go toolchain is unavailable, so the default `go list` driver has nothing to talk to. genception implements the GOPACKAGESDRIVER protocol against a package inventory that Bazel provides, answering file and pattern queries from that inventory instead of shelling out. It exits 0 even on error, because gopls silently falls back to `go list` on a non-zero exit, which is the failure mode this is meant to avoid.
0440be1 to
7aba3e0
Compare
Add genception, a Bazel-backed go/packages driver
Code generators that resolve Go types through
golang.org/x/tools/go/packagescannot run inside the Bazel sandbox. The default driver shells out togo list, and the go toolchain is not available in the sandbox, so there is nothing for it to talk to. That blocks codegen tools that usex/tools/go/packagesfor semantic parsing of source (rather than trying to deal with source code as raw text).tools/genceptionis aGOPACKAGESDRIVERimplementation that closes that gap. Instead of invoking the go toolchain, it answersgo/packagesqueries from a package inventory generated inside of a Bazel rules plugin ("aspect"): the per-target*.pkg.jsonfiles produced by the rules_gogopackagesdriveraspect, plus an index file listing them. It loads that inventory into an in-memory registry, post-processes it into a complete package graph (absolute paths, stdlib imports, test packages), and servesfile=/pattern=/bare-pattern queries against it.This PR adds the tool and its tests only. Nothing in the tree invokes it yet, so existing builds are unaffected; the Bazel rule that wires it into codegen (
tools/methodical.bzl) lands later in the series.Key changes
Entrypoint
tools/genception/cmd/main.go— reads apackages.DriverRequestfrom stdin, writes the driver response to stdout. Builds the registry once per invocation (driver.NewJSONDriver), since loading the inventory is IO-heavy.go listwhen it is non-zero, which is exactly the failure mode this tool exists to avoid; a hidden fallback is worse than a visibly empty response.Inventory ingest and package graph (
tools/genception/driver/)jsondriver.go—JSONDriverorchestrates the flow: load env, read the inventory index, stream-decode each*.pkg.jsoninto the registry, resolve, then serve queries viaHandle.inventory.go—loadJsonListingreads the index file (a JSON[]stringof exec-root-relative paths) named byPACKAGE_JSON_INVENTORY.flatpackage.go—flatPackageis the trimmed JSON form ofpackages.Package, and owns the Bazel-to-go/packagestranslations:resolveStdlibre-adds stdlib imports by parsing each compiled file's import block, since Bazel is unaware of stdlib and omits those edges from the JSON.deriveTestPackagesplits_test.gofiles by their package clause: internal tests fold back into the base package, external tests become a derived<id>_xtestpackage that imports the package under test.registry.go— the queryable graph.add/updatemerge packages that share a Go package path. Multiple Bazel labels can map to one package path (e.g.:go_protoand:go_default_libraryunder//proto/prysm/v1alpha1); without the merge, one clobbers the other and source files disappear from the graph. The entry with the superset source list wins wholesale, keepingGoFiles/CompiledGoFiles/Importsfrom a single consistent target.rewritePackage/canonicalizeIDnormalize package IDs to import paths and strip the@@io_bazel_rules_go//stdlib:label prefix.resolveQueryIDhandles the three query forms from the driver protocol;query/walkreturn the matched roots plus their transitive dependency closure. Queries that match nothing are logged and dropped rather than emitted as dangling roots.indexFilesbuilds the file-to-package index used byfile=queries, after paths are absolute and test packages exist.resolver.go— rewrites Bazel's symbolic path prefixes (__BAZEL_EXECROOT__,__BAZEL_OUTPUT_BASE__,__BAZEL_WORKSPACE__) into real absolute paths, stat-checking each candidate root.tagfilt.go— filters the file lists throughgo/build'sMatchFileusing the configured build tags, so the response reflects what would actually compile (relevant here given Prysm'sdeveloptag).response.go—driverResponsemirrorspackages.DriverResponsebut carriesflatPackage;parseGoMinorVersionderives the protocol'sGoVersionfield fromruntime.Version(), returning 0 for unparseable values such as devel builds.logger.go— logs to the configured file, falling back to stderr. Configured during driver construction rather thaninit()so that importing the package has no filesystem side effects (stdout is the protocol channel, so log output must not go there).recorder.go— optional debugging aid; when enabled, dumps the request and response JSON to a timestamped directory.Tests
env_test.go(required/optional env vars, includingPWD's fallback to the process value),inventory_test.gowithtestdata/json-list.json,registry_test.go(isSuperset, the superset-wins merge in both add orders, query-form resolution, resolved roots),response_test.go(Go version parsing).Configuration
The driver is configured entirely through environment variables (
tools/genception/driver/env.go). Required:PACKAGE_JSON_INVENTORY*.pkg.jsonfilesPACKAGES_BASEPWDPWDif unsetOptional:
GOTAGS(comma-separated build tags for file filtering),GOPACKAGESDRIVER_LOG_PATH(defaults togenception.logunderPWD), andGOPACKAGESDRIVER_RECORDER_PATH(unset disables request/response recording).Behavior notes
file=queries the response describes the whole enclosing package, soCompiledGoFilescontains more than the single file named in the query. This is intentional — the caller needs the full package to type-check it.//tools/genception/cmd(publicgo_binary) and//tools/genception/driver(go_default_library+go_default_test). No existing target depends on them in this PR.Acknowledgements
Stack created with GitHub Stacks CLI • Give Feedback 💬