Repository navigation
Conversation
`ModuleDownloadRequest` carried the whole `GoBuildOptions` in its memoization key, but downloading and analyzing a module barely depends on it: `go mod download` never consults build options, and the only consumer anywhere in `download_and_analyze_module` is `CGO_ENABLED` for the package analyzer. So callers differing only in a field that cannot affect the result each paid for their own download of every module in the graph. That is not hypothetical: target generation passes `GoBuildOptions()` defaults to dodge a rule-graph cycle (see the TODO in `target_type_rules.py`) while a compile resolves the real options, so a repo building with `race=True` downloaded every third-party module exactly twice per run. Measured against a real monorepo: ~1,979 remote-cache AC lookups for 1,059 distinct modules. Narrow the key to `cgo_enabled`. Both it and `[golang].cgo_enabled` default to `True`, so the two keys collapse into one. This also fixes the general N-way case -- any two callers differing only in irrelevant options now share one download -- which resolving the rule-graph cycle would not have done, since targets in a module can legitimately differ in build options. The TODO is annotated rather than removed: the cycle still exists, it just no longer costs a duplicate download. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Import-scoped third-party target generation needs to know every third-party
import a `go_mod`'s first-party code could reference. `Imports`/`TestImports`
cannot answer that: they describe what *this* build compiles, so they are
filtered by the host's GOOS/GOARCH and build tags, and they are only reported
when the directory forms a single valid package.
Three cases would silently lose roots, and a missing root means failing to
generate a target the user asked for:
* a `//go:build` guarded file, or a file for another platform;
* a cgo file when cgo is disabled, which lands in IgnoredGoFiles with its
imports dropped entirely;
* the `tools.go` pattern, where a constraint-excluded file deliberately
declares a different package name.
Add `AllImports`/`AllTestImports`, collected from every .go file regardless of
build constraints or package-name agreement. Files Go ignores outright (`_` or
`.` prefixed) and the pseudo-import "C" are excluded. Constraint-excluded files
are parsed only to harvest imports, and parse failures there are ignored --
the file is out of the build either way and matchFile already recorded real
errors.
The fields are assigned before the "multiple package names encountered" and
"no buildable Go source files" errors can return, because those are exactly the
cases the caller still needs roots for -- a directory holding only a
build-tag-gated tools.go hits the latter. `main` marshals the package even when
Error is set.
Carry them on `FallibleFirstPartyPkgAnalysis` rather than `FirstPartyPkgAnalysis`
for the same reason: in those cases `analysis` is None.
Verified against a directory combining all three cases: Imports stays
["github.com/normal/dep"] while AllImports also reports the plan9-only and
tools.go imports; a tools-only directory still reports its root alongside its
"no buildable Go source files" error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Stage one of import-scoped `go_third_party_package` generation: find the
third-party import paths a `go_mod`'s own code actually references, so the walk
that follows can resolve only those modules instead of the whole build list.
The scan reads `.go` files from the filesystem rather than resolving
`go_package` targets. That is not a shortcut -- the only caller is
`generate_targets_from_go_mod`, which *is* a target generator, so consulting the
target graph here would reintroduce the same rule-graph cycle already documented
by the `build_opts` TODO in `target_type_rules.py`.
Roots come from `AllImports`/`AllTestImports` (constraint-free, so build-tag
gated files, other-platform files and the `tools.go` pattern all resolve) plus
`tool` directives in go.mod, which name executable packages no `.go` file
imports -- the Go 1.24+ replacement for `tools.go`.
Two details worth keeping:
* `execute_process`, not `execute_process_or_raise`. A `tools.go`-only
directory legitimately fails analysis with "no buildable Go source files"
while still reporting the roots we need, so a non-zero exit must not abort
the scan.
* a nested `go.mod` carves its whole subtree out of the parent module, so its
sources contribute nothing to the parent's roots. Prefix matching is done on
path segments, so a sibling like `vendored_helpers` is not mistaken for a
child of `vendored`.
Filtering roots down to third-party packages and walking them to a fixpoint
comes next; this commit only produces the root set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Second half of import-scoped generation: walk the import graph outward from the first-party roots, downloading a module only when something actually reaches into it, and iterating until no new import paths appear. The default stays `build-list`, so this is a no-op unless opted into. Under `imported`, `go_third_party_package` targets are generated only for packages reachable from the `go_mod`'s own code, which also means target generation stops downloading the entire module graph -- the reason it was expensive in the first place. Two behavior changes come with opting in, both called out in the option help: unimported third-party packages no longer appear in `pants list ::`, and hand-written addresses pointing at one no longer resolve. Import paths are resolved to modules the way `cmd/go` does: a module provides an import path when its path is that path or a path-segment prefix of it, longest match winning. Names are sorted longest-first so that is deterministic rather than dict-order dependent. An import path that resolves to no downloaded package is skipped rather than raising. That is routine -- an import reachable only under an unsatisfied build tag may name a package that does not exist for this platform, or name no module on the build list at all -- and the build-list path would not have produced a target for it either. Tested against a go.mod requiring both `rsc.io/quote` and `github.com/google/uuid` where first-party code imports only `quote`: `imported` yields `quote` and its transitive `rsc.io/sampler` (so the walk demonstrably iterates rather than stopping at direct imports) while excluding `uuid`, and the result is a strict subset of what `build-list` produces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Generated `.pb.go` files do not exist when the import scan runs, so nothing reports the runtime packages they import. Under `[golang].third_party_target_generation = "imported"` that meant no targets for the protobuf runtime and generated code that could not build. Since we enable the Go protobuf backend, this made `imported` unusable rather than merely imprecise. Add a `GoCodegenImportRootsRequest` union so codegen backends can name the modules their output imports, and implement it for Protobuf. Roots are named as whole *modules*, not individual import paths. Precision would buy nothing: a module is downloaded and analyzed as a unit, so treating all of its packages as reachable costs no extra downloads -- only a handful of extra targets in the one or two runtime modules involved. The expensive behavior this option exists to fix is unaffected either way. The hook must not consult the target graph. Its only consumer is `generate_targets_from_go_mod`, which is itself a target generator; requesting targets from there deadlocks the engine with "The dependency graph contained a cycle" (confirmed by probe: injecting `AllTargets` into that rule fails 3 of 5 tests in `target_type_rules_test.py` with exactly that error, while leaving the Pants repo's own `pants list` green, since this repo has no `go_mod` targets). That constraint has one visible consequence: `ProtobufGrpcToggleField` is per-target BUILD metadata with no existence outside the target graph, so the hook cannot know whether the grpc plugin runs and reports the grpc runtime whenever any proto generates Go. Over-reporting costs one module's targets; under-reporting breaks the build. Whether a proto generates Go at all is decided by `option go_package`, which is readable straight from the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
…se note `download_and_analyze_third_party_packages` now takes a subsystem and union membership, so the two call sites introduced on main by pantsbuild#23499 -- `expand_module_import_paths` and the module-granularity branch of `setup_full_package_build_request` -- need `**implicitly()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
rdeknijf
left a comment
There was a problem hiding this comment.
Read the walk, the scan, the analyzer change and the codegen hook. Nothing blocking; details inline.
|
@aegis-cipherowl thanks for opening this, and for asking on #20340 first. I only saw that comment today. Go ahead with this PR. I will not open my branch. Yours is the better design anyway; You scope inside CI has not run on either PR yet, so here are the touched tests on this branch against current main (free-threaded Python 3.14, Go 1.27): That is head 5adb776, which already has the #23664 key change, so it covers both PRs. I also ran it end to end on the 3-go.mod reproducer from #20340 with a fresh local store per mode. Review comments are inline, nothing blocking. |
|
|
||
| third_party_target_generation = EnumOption( | ||
| default=GoThirdPartyTargetGeneration.BUILD_LIST, | ||
| help=softwrap( |
There was a problem hiding this comment.
Using the Pants-internalhelp_text function instead of softwrap should allow you to avoid the backslash-continued lines.
There was a problem hiding this comment.
Leaving this one open for you, since I did not do quite what you suggested.
help_text does not typecheck for EnumOption: it returns str | Callable[[], str], EnumOption expects Callable[[Any], Any] | str, and mypy rejects it. Its docstring says it exists because "mypy does not respect the type hints defined on base Field and Target classes" — and no EnumOption in the tree uses it.
The backslash continuations are gone regardless: softwrap preserves indented bullet blocks on its own (same shape as [go-test].coverage_mode), so they were never needed. Verified the rendered pants help golang still shows the bullets correctly.
Happy to go another way if you had a different form in mind.
…/import-scoped-target-generation # Conflicts: # docs/notes/2.34.x.md # src/python/pants/backend/go/util_rules/third_party_pkg.py # src/python/pants/backend/go/util_rules/third_party_pkg_test.py
Correctness:
* The walk followed `ThirdPartyPkgAnalysis.imports`, which the analyzer had
already filtered by the host's build constraints, while the roots feeding it
were constraint-free. Past the first hop the computed closure therefore
depended on the machine running Pants. Carry `all_imports` on the analysis
and walk that instead (@rdeknijf).
* The source glob took every `.go` file under the module, but `go list ./...`
skips `testdata`, `vendor`, and any component starting with `.` or `_`. A
`testdata` fixture importing a build-list module made it a root, downloading
the module and generating targets for code nothing builds (@rdeknijf).
* `ModuleImportPathResolver` now indexes modules under both `name` and
`import_path`, so a versioned `replace` that changes the path resolves
(@rdeknijf).
Cleanups:
* Drop the unused `all_imports`/`all_test_imports` from
`FallibleFirstPartyPkgAnalysis`. Nothing read them: the scan parses the
analyzer's JSON directly, and it cannot go through that type without
resolving targets, which is the rule-graph cycle this design avoids
(@rdeknijf).
* `FirstPartyImportRootsRequest` no longer carries `cgo_enabled`.
`AllImports` is harvested regardless of cgo, so it only split the scan's
memo key; `CGO_ENABLED=1` is pinned in the process env instead (@rdeknijf).
* Use `ijson.backends.python`, matching the other Go rules (@rdeknijf).
* Replace the sorted-tuple-plus-free-function with `ModuleImportPathResolver`,
so the longest-first invariant is established in the constructor rather than
upheld by callers (@tdyas).
* Rename the `fileInfo` variable in the analyzer's file loop, which shadowed
the `fileInfo` type (@tdyas).
* Drop the backslash continuations from the option's help text. `help_text`
was suggested, but it returns `str | Callable[[], str]` and exists for
`Field`/`Target`, so it does not typecheck for `EnumOption`; `softwrap`
preserves the indented bullets on its own (@tdyas).
New tests cover the `go list` walk exclusions, at both the helper and scan level.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Per review, `tool` directive support is a distinct change and goes up as its own PR stacked on this one. The option help and the release note no longer claim `tool` directives are treated as imports, since after this commit they are not. `imported` still handles the older `tools.go` pattern, which the constraint-free scan covers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
|
Merged latest main (#23664 landed, so the stacked commits collapse out) and pushed the review fixes. @rdeknijf — thanks for the go-ahead, and for running the tests on main. That closed a gap I had flagged: my machine SIGSEGVs on every pytest invocation on Correctness, all three yours:
Cleanups: dropped the redundant Two places I did not do what was suggested:
|
CI caught three more callers of `download_and_analyze_third_party_packages` that need `**implicitly()` now that the rule takes a subsystem and union membership: `build_pkg_third_party`, `build_pkg_target` and `debug_goals`. Missed locally because mypy was run over an explicit file list rather than the whole repo, which is what CI checks. Verified with `pants check ::` this time: clean across all 1831 source files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
|
@tdyas can you PTAL? Thanks |
|
How does this PR interact with the prior work to build all of the packages in a third-party module together instead of separately? |
@aegis-cipherowl can you respond? I'd like to get this merged once @tdyas approves. |
I'm not 100% sure about what the "prior work" refers to, but imported mode never changes what a build packages sees. pkg_info — including digest, the slicing tier, and the native-code decision — comes from download_and_analyze_module, which analyses a whole module as a unit. imported changes only |
A `tool` directive (Go 1.24+) names an executable package a module depends on without any `.go` file importing it. It is the official replacement for the `tools.go` pattern, which `[golang].third_party_target_generation = "imported"` already handles via the constraint-free scan. Without this, a repository that has migrated to `tool` directives loses those targets under `imported` and fails to build, while one still using `tools.go` does not -- an inconsistency between the old idiom and its successor. Parses both the single-line form (`tool example.com/cmd/foo`) and the block form. Comments are stripped first, and matching is on the `tool` keyword rather than a prefix, so `module example.com/toolkit` and `require example.com/toolbox` are not mistaken for directives. Split out of pantsbuild#23665 at review request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBwx1ZTZJ5oGxSAadmtucS
Background
Closes #20340. Implements the design @rdeknijf proposed and @benjyw approved there; the design is theirs, this is our implementation of it, opened with their go-ahead.
Adds
[golang].third_party_target_generation=build-list(default, today's behavior) |imported. Underimported,go_third_party_packagetargets are generated only for third-party packages reachable through the import graph from first-party code, so target generation stops downloading and analyzing modules nothing imports.#23499's
third_party_target_granularity = moduledoes not cover this:generate_targets_from_go_modcallsdownload_and_analyze_third_party_packagesunconditionally, and the granularity branch selects from the already-resolved result. The two compose — import scoping decides which modules are resolved, granularity decides how many targets each produces.Four parts: constraint-free
AllImports/AllTestImportson the package analyzer (so build-tag-gated, other-platform andtools.goimports still resolve); a first-party root scan; a fixpoint walk downloading only reached modules; and a codegen roots union with Protobuf registered, so generated.pb.goimports still get targets.The root scan reads
.gofiles from the filesystem rather than resolving targets. That is forced: its only caller is a target generator, so touching the target graph deadlocks the engine withThe dependency graph contained a cycle. It is easy to miss, because injectingAllTargetsthere leavespants listgreen in this repo — which has nogo_modtargets — and only fails once a real one exists.Measured
32-
go_modproduction repo,check src/go/config::(an 8-module leaf), fresh local store:The identical compile count is the correctness check. GOSUMDB fallbacks go to zero structurally: Go 1.17+ prunes
go.sumto exactly the imported closure, which is what the walk resolves, so the fallback branch is unreachable.Notes
tooldirective support is split out to #23670, stacked on this.LLM assistance notice
This PR was written primarily by Claude Code (Claude Opus 5). I directed the scope and reviewed the result.
🤖 Generated with Claude Code