Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/notes/2.34.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ Linked Go binaries and test binaries now record a Go toolchain build ID, as `go
Fixed `[go-test].force_race` and the `go_package` fields `test_race`, `test_msan` and `test_asan` being silently ignored. The `test` goal built its Go build options without requesting the test-specific fields, so the sanitizer settings never reached the test binary and only the owning `go_mod`'s `race` field had any effect.
Dependency inference now resolves imports across local directory `replace` directives. When a `go.mod` replaces a required module with a path to another first-party module in the repo, imports of that module's packages are inferred automatically instead of having to be listed by hand in BUILD metadata. This is a no-op for modules without directory `replace` directives. See [#22097](https://github.com/pantsbuild/pants/issues/22097).

The new option `[golang].third_party_target_generation` controls which third-party packages a `go_mod` generates targets for. The default `build-list` keeps today's behavior, generating targets for every package of every module in the module build list. `imported` instead generates targets only for third-party packages reachable through the import graph from the `go_mod`'s own first-party code, and stops target generation from downloading and analyzing modules that nothing imports.

The build list over-approximates what a repository actually uses: minimal version selection resolves at module granularity, so importing one package from a module puts that whole module, and everything it requires, on the list. Because target generation must analyze each listed module, it also downloads each one, whether or not anything imports it -- and the cost is independent of what you asked to build.

Opting in changes two behaviors: third-party packages nothing imports no longer appear in `pants list ::`, and hand-written addresses pointing at such a package no longer resolve. Imports guarded by build tags, imports for other platforms, and the `tools.go` pattern are all treated as imports, so they still generate targets. See [#20340](https://github.com/pantsbuild/pants/issues/20340).

Third-party module downloads are no longer duplicated across callers that differ only in build options which cannot affect them. Downloading and analyzing a module consults just `cgo_enabled` -- `go mod download` ignores build options entirely, and the package analyzer reads only `CGO_ENABLED` -- but the whole `GoBuildOptions` was part of the memoization key. Target generation passes default options while a compile resolves the real ones, so a repository building with e.g. `race=True` downloaded every third-party module twice per run.

### Plugin API changes
Expand Down
56 changes: 55 additions & 1 deletion src/python/pants/backend/codegen/protobuf/go/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@
GoStdLibPackagesRequest,
analyze_go_stdlib_packages,
)
from pants.backend.go.util_rules.import_roots import (
GoCodegenImportRoots,
GoCodegenImportRootsRequest,
)
from pants.backend.go.util_rules.pkg_analyzer import PackageAnalyzerSetup
from pants.backend.go.util_rules.sdk import GoSdkProcess
from pants.backend.go.util_rules.third_party_pkg import (
Expand All @@ -89,6 +93,7 @@
Directory,
FileContent,
MergeDigests,
PathGlobs,
RemovePrefix,
)
from pants.engine.internals.graph import hydrate_sources, resolve_source_paths, transitive_targets
Expand Down Expand Up @@ -158,6 +163,53 @@ class ProtobufGoModuleImportPathsMappingsHook(GoModuleImportPathsMappingsHook):
pass


class ProtobufGoCodegenImportRootsRequest(GoCodegenImportRootsRequest):
pass


# The packages `protoc-gen-go` and `protoc-gen-go-grpc` emit into generated files. Named as whole
# modules rather than individual import paths -- a module is downloaded and analyzed as a unit, so
# naming every package of it costs no extra downloads.
_PROTOBUF_RUNTIME_MODULE = "google.golang.org/protobuf"
_GRPC_RUNTIME_MODULE = "google.golang.org/grpc"


@rule(desc="Determine Go import roots contributed by Protobuf codegen", level=LogLevel.DEBUG)
async def protobuf_go_codegen_import_roots(
request: ProtobufGoCodegenImportRootsRequest,
) -> GoCodegenImportRoots:
"""Report the runtime modules that generated `.pb.go` files import.

Generated sources do not exist when the import scan runs, so their imports are invisible to it.
Without this, `[golang].third_party_target_generation = "imported"` would drop the protobuf
runtime targets and generated code would fail to build.

NB: this deliberately does not consult the target graph. The only consumer is
`generate_targets_from_go_mod`, which is itself a target generator, so requesting targets here
deadlocks the engine with "The dependency graph contained a cycle". That also rules out reading
each target's `grpc` toggle, so the gRPC runtime is reported whenever any proto generates Go.
Over-reporting costs one extra module's targets; under-reporting breaks the build.
"""
base_dir = os.path.dirname(request.go_mod_path)
prefix = f"{base_dir}/" if base_dir else ""

# NB: a `.proto` under a *nested* go.mod is counted for the parent too. That is a harmless
# over-approximation of an already over-approximating hook.
protos = await digest_to_snapshot(**implicitly(PathGlobs([f"{prefix}**/*.proto"])))
if not protos.files:
return GoCodegenImportRoots(())

proto_contents = await get_digest_contents(protos.digest)
# `option go_package` is what makes a proto generate Go at all.
generates_go = any(
parse_go_package_option(entry.content) is not None for entry in proto_contents
)
if not generates_go:
return GoCodegenImportRoots(())

return GoCodegenImportRoots((_PROTOBUF_RUNTIME_MODULE, _GRPC_RUNTIME_MODULE))


@rule(desc="Map import paths for all Go Protobuf targets.", level=LogLevel.DEBUG)
async def map_import_paths_of_all_go_protobuf_targets(
_request: ProtobufGoModuleImportPathsMappingsHook,
Expand Down Expand Up @@ -413,7 +465,8 @@ async def setup_full_package_build_request(
go_mod_info.digest,
go_mod_info.mod_path,
build_opts=request.build_opts,
)
),
**implicitly(),
)
third_party_index = all_third_party_packages.import_paths_to_pkg_info

Expand Down Expand Up @@ -777,6 +830,7 @@ def rules():
UnionRule(GenerateSourcesRequest, GenerateGoFromProtobufRequest),
UnionRule(GoCodegenBuildRequest, GoCodegenBuildProtobufRequest),
UnionRule(GoModuleImportPathsMappingsHook, ProtobufGoModuleImportPathsMappingsHook),
UnionRule(GoCodegenImportRootsRequest, ProtobufGoCodegenImportRootsRequest),
ProtobufSourcesGeneratorTarget.register_plugin_field(GoOwningGoModAddressField),
ProtobufSourceTarget.register_plugin_field(GoOwningGoModAddressField),
*protoc.rules(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from pants.backend.codegen.protobuf import protobuf_dependency_inference, target_types
from pants.backend.codegen.protobuf.go.rules import (
GenerateGoFromProtobufRequest,
ProtobufGoCodegenImportRootsRequest,
parse_go_package_option,
)
from pants.backend.codegen.protobuf.go.rules import rules as go_protobuf_rules
Expand All @@ -35,6 +36,7 @@
tests_analysis,
third_party_pkg,
)
from pants.backend.go.util_rules.import_roots import GoCodegenImportRoots
from pants.build_graph.address import Address
from pants.core.goals.test import TestResult, get_filtered_environment
from pants.core.util_rules import config_files, source_files, stripped_source_files
Expand Down Expand Up @@ -77,6 +79,7 @@ def rule_runner() -> RuleRunner:
QueryRule(GeneratedSources, [GenerateGoFromProtobufRequest]),
QueryRule(DigestContents, (Digest,)),
QueryRule(TestResult, (GoTestRequest.Batch,)),
QueryRule(GoCodegenImportRoots, [ProtobufGoCodegenImportRootsRequest]),
],
target_types=[
GoModTarget,
Expand Down Expand Up @@ -408,3 +411,66 @@ def test_generates_go_grpc(rule_runner: RuleRunner) -> None:
"protos/service_grpc.pb.go",
],
)


def test_codegen_import_roots_reported_for_go_generating_protos(rule_runner: RuleRunner) -> None:
"""Protobuf contributes the runtime modules that generated `.pb.go` files import.

Those files do not exist when the import scan runs, so without this hook
`[golang].third_party_target_generation = "imported"` would generate no targets for the
protobuf runtime and the generated code would fail to build.
"""
rule_runner.write_files(
{
"BUILD": "go_mod(name='mod')\n",
"go.mod": "module example.com/m\ngo 1.16\n",
"f.proto": dedent(
"""\
syntax = "proto3";
package example;
option go_package = "example.com/m/gen";
message Foo { string bar = 1; }
"""
),
}
)
result = rule_runner.request(
GoCodegenImportRoots,
[
ProtobufGoCodegenImportRootsRequest(
go_mod_address=Address("", target_name="mod"),
go_mod_path="go.mod",
)
],
)
assert "google.golang.org/protobuf" in result.module_paths
# The per-target grpc toggle lives in the target graph, which this hook must not touch, so the
# grpc runtime is reported unconditionally.
assert "google.golang.org/grpc" in result.module_paths


def test_no_codegen_import_roots_without_go_generating_protos(rule_runner: RuleRunner) -> None:
"""A proto with no `option go_package` generates no Go, so it contributes no roots."""
rule_runner.write_files(
{
"BUILD": "go_mod(name='mod')\n",
"go.mod": "module example.com/m\ngo 1.16\n",
"f.proto": dedent(
"""\
syntax = "proto3";
package example;
message Foo { string bar = 1; }
"""
),
}
)
result = rule_runner.request(
GoCodegenImportRoots,
[
ProtobufGoCodegenImportRootsRequest(
go_mod_address=Address("", target_name="mod"),
go_mod_path="go.mod",
)
],
)
assert result.module_paths == ()
2 changes: 2 additions & 0 deletions src/python/pants/backend/experimental/go/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
implicit_linker_deps,
import_analysis,
import_config,
import_roots,
link,
pkg_analyzer,
sdk,
Expand Down Expand Up @@ -65,6 +66,7 @@ def rules():
*coverage_output.rules(),
*cgo.rules(),
*dep_inference_rules(),
*import_roots.rules(),
*third_party_pkg.rules(),
*generate.rules(),
*go_bootstrap.rules(),
Expand Down
60 changes: 58 additions & 2 deletions src/python/pants/backend/go/go_sources/analyze_package/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,22 @@ type Package struct {
TestImports []string `json:",omitempty"`
XTestImports []string `json:",omitempty"`

// Every import appearing in any .go file in this directory, regardless of whether the file
// is selected by the current build constraints and regardless of whether its `package`
// directive agrees with the rest of the directory.
//
// Imports/TestImports/XTestImports above describe what *this* build would compile, so they
// are filtered by GOOS/GOARCH/build tags and are only reported when the directory forms a
// single valid package. Import-scoped third-party target generation needs the opposite: a
// superset that still resolves build-tag-gated imports, imports for other platforms, and the
// `tools.go` pattern (a constraint-excluded file whose package name deliberately differs).
// Missing a root there means failing to generate a target the user asked for.
//
// Files Go ignores outright -- those prefixed with `_` or `.` -- are excluded, as is the
// pseudo-import "C".
AllImports []string `json:",omitempty"`
AllTestImports []string `json:",omitempty"`

// //go:embed patterns found in Go source files
// For example, if a source file says
// //go:embed a* b.c
Expand Down Expand Up @@ -232,6 +248,28 @@ func analyzePackage(directory string, buildContext *build.Context) (*Package, er
testImportsMap := make(map[string]bool)
xtestImportsMap := make(map[string]bool)

allImportsMap := make(map[string]bool)
allTestImportsMap := make(map[string]bool)

// Record a file's imports into the constraint-free sets. Test files are split out by filename
// rather than by package name, since a constraint-excluded file's package directive is exactly
// what we are declining to trust here.
harvestAllImports := func(name string, analysis *fileInfo) {
if analysis == nil {
return
}
target := allImportsMap
if strings.HasSuffix(name, "_test.go") {
target = allTestImportsMap
}
for _, imp := range analysis.imports {
if imp.path == "C" {
continue
}
target[imp.path] = true
}
}

embedsMap := make(map[string]bool)
testEmbedsMap := make(map[string]bool)
xtestEmbedsMap := make(map[string]bool)
Expand Down Expand Up @@ -263,17 +301,24 @@ func analyzePackage(directory string, buildContext *build.Context) (*Package, er
// TODO: `MatchFile` will actually parse the imports but does not return the AST. Consider vendoring
// the MatchFile logic to avoid double parsing.
binaryOnly := false
fileInfo, err := matchFile(buildContext, directory, name, allTags, &binaryOnly, fileSet)
matchedFile, err := matchFile(buildContext, directory, name, allTags, &binaryOnly, fileSet)
if err != nil {
pkg.InvalidGoFiles[name] = err.Error()
continue
}
if fileInfo == nil {
if matchedFile == nil {
if strings.HasPrefix(name, "_") || strings.HasPrefix(name, ".") {
// `go` ignores files prefixed with underscore or period. Since this is not due to
// build constraints, do not report it as an ignored file. Fall through.
} else if ext == ".go" {
pkg.IgnoredGoFiles = append(pkg.IgnoredGoFiles, name)
// This file lost to a build constraint, but its imports are still real: a
// `//go:build` guarded file, an other-platform file, or `tools.go`. Parse it
// solely to harvest roots. Parse failures are ignored -- the file is excluded
// from the build either way, and matchFile already recorded genuine errors.
if excludedAnalysis, err := analyzeFile(fileSet, filepath.Join(directory, name)); err == nil {
harvestAllImports(name, excludedAnalysis)
}
} else if fileListForExt(pkg, ext) != nil {
pkg.IgnoredOtherFiles = append(pkg.IgnoredOtherFiles, name)
}
Expand Down Expand Up @@ -382,6 +427,11 @@ func analyzePackage(directory string, buildContext *build.Context) (*Package, er
}
}

// NB: unconditional, and deliberately not gated on importsMapForFile -- a cgo file with
// cgo disabled lands in IgnoredGoFiles with a nil imports map, but its imports are still
// roots for target generation.
harvestAllImports(name, analysis)

if embedsMapForFile != nil && len(analysis.embeds) > 0 {
for _, e := range analysis.embeds {
embedsMapForFile[e.pattern] = true
Expand All @@ -395,6 +445,12 @@ func analyzePackage(directory string, buildContext *build.Context) (*Package, er
pkg.TestImports = cleanStringSet(testImportsMap)
pkg.XTestImports = cleanStringSet(xtestImportsMap)

// NB: assigned here, before the "multiple package names" and "no buildable Go source files"
// errors below can return. Those are precisely the cases -- e.g. a `tools.go`-only directory --
// where the caller still needs the roots, and `main` marshals the package even when Error is set.
pkg.AllImports = cleanStringSet(allImportsMap)
pkg.AllTestImports = cleanStringSet(allTestImportsMap)

pkg.EmbedPatterns = cleanStringSet(embedsMap)
pkg.TestEmbedPatterns = cleanStringSet(testEmbedsMap)
pkg.XTestEmbedPatterns = cleanStringSet(xtestEmbedsMap)
Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/backend/go/goals/debug_goals.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ async def go_show_package_analysis(
request.go_mod_digest,
request.go_mod_path,
build_opts=request.build_opts,
)
),
**implicitly(),
)
if request.import_path not in all_packages.import_paths_to_pkg_info:
console.write_stdout(
Expand Down
38 changes: 38 additions & 0 deletions src/python/pants/backend/go/subsystems/golang.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ class ThirdPartyTargetGranularity(Enum):
MODULE = "module"


class GoThirdPartyTargetGeneration(Enum):
"""Which third-party packages a `go_mod` generates targets for."""

BUILD_LIST = "build-list"
IMPORTED = "imported"


class GolangSubsystem(Subsystem):
options_scope = "golang"
help = "Options for Golang support."
Expand Down Expand Up @@ -270,6 +277,37 @@ def extra_tools(self) -> tuple[str, ...]:
advanced=True,
)

third_party_target_generation = EnumOption(
default=GoThirdPartyTargetGeneration.BUILD_LIST,
help=softwrap(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the Pants-internalhelp_text function instead of softwrap should allow you to avoid the backslash-continued lines.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Which `go_third_party_package` targets to generate for each `go_mod` target.

* `build-list`: generate a target for every package of every module in the module
build list, i.e. everything `go list -m all` reports.
* `imported`: generate targets only for third-party packages reachable through the
import graph from the `go_mod`'s own first-party code.

The build list over-approximates what a repo actually uses, often by a lot: minimal
version selection resolves at module granularity, so importing one package from a
module puts that whole module -- and everything it requires -- on the list, and every
package of every listed module becomes a target. Because target generation must
analyze each of those modules, it also downloads them, whether or not anything
imports them.

Setting `imported` can dramatically reduce target count, memory use and the number of
module downloads, at the cost of two behavior changes:

* third-party packages nothing imports no longer appear in `pants list ::`;
* hand-written addresses pointing at an unimported third-party package no longer
resolve.

Imports guarded by build tags, imports for other platforms and the `tools.go` pattern
are all treated as imports, so they still generate targets.
"""
),
)

cgo_enabled = BoolOption(
default=True,
help=softwrap(
Expand Down
6 changes: 4 additions & 2 deletions src/python/pants/backend/go/target_type_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ async def expand_module_import_paths(
go_mod_info.mod_path,
# Matches target generation (see `generate_targets_from_go_mod`).
build_opts=GoBuildOptions(),
)
),
**implicitly(),
)
return ModuleImportPathsExpansion(
{
Expand Down Expand Up @@ -591,7 +592,8 @@ async def generate_targets_from_go_mod(
# `GoBuildOptions`, so target generation and a compile resolving e.g. `race=True` share
# one memoized download. Only a caller that actually differs in `cgo_enabled` pays twice.
build_opts=GoBuildOptions(),
)
),
**implicitly(),
)

def gen_file_tgt(fp: str) -> TargetGeneratorSourcesHelperTarget:
Expand Down
Loading
Loading