Skip to content

Commit

Permalink
[internal] Generate _go_internal_package targets (#13139)
Browse files Browse the repository at this point in the history
Closes #13049. This greatly reduces boilerplate and also allows us to make some fields required like `import_path` and `subpath`, so that we don't have to calculate those in consuming rules like `build_go_pkg.py`.

## The address format

The generated address looks like `project#./dir`. @tdyas offered that this is intuitive for Go developers because they have to say `go build ./dir` already with the leading `./`. 

This solves how to represent when the `_go_internal_package` is in the same dir as the `go_mod`: `project#./`.

This also makes very clear the difference from external packages like `project#rsc.io/quote` vs. internal packages like `project#./dir`.

## Improves dependency inference

Now that the `import_path` field is required for both `_go_internal_package` and `_go_external_package`, we can create a global mapping of `import_path -> pkg_target`. This is necessary for #13114.

This also improves performance. We don't need to call `ResolvedGoPackage` on all the candidate targets a package might depend on to calculate their import paths. We do still need it when resolving the deps of a particular `_go_internal_package`, but we can be lazier when we call that codepath in not evaluating all candidate targets.

### `dependencies` benchmark

As expected, there is no difference because we are finding the dependencies of everything, so we still have to call `ResolvedGoPackage`. The perf gains are only in things sometimes being less eager, which isn't the case here.

Before:

```
❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache dependencies ::'
  Time (mean ± σ):     26.501 s ±  1.537 s    [User: 29.554 s, System: 24.115 s]
  Range (min … max):   24.928 s … 28.763 s    5 runs
```

After:
```
❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache dependencies ::'
  Time (mean ± σ):     26.359 s ±  0.526 s    [User: 29.600 s, System: 23.769 s]
  Range (min … max):   25.625 s … 26.993 s    5 runs
```

### `package` benchmark

Before:

```
❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache package ::'
  Time (mean ± σ):     33.777 s ±  0.248 s    [User: 39.221 s, System: 39.389 s]
  Range (min … max):   33.517 s … 34.062 s    5 runs
```

After:

```
❯ hyperfine -r 5 './pants_from_sources --no-pantsd --no-process-execution-local-cache package ::'
Benchmark #1: ./pants_from_sources --no-pantsd --no-process-execution-local-cache package ::
  Time (mean ± σ):     31.049 s ±  0.702 s    [User: 40.606 s, System: 40.537 s]
  Range (min … max):   30.512 s … 32.273 s    5 runs
```

## TODO: fix `go_binary` inference of `main` field

#13117 added inference of the `main` field for `go_binary`, that it defaults to the `go_package` defined in that directory.

But target generation no longer generates targets actually in each directory. All generated targets are "located" in the BUILD file of the `go_mod`, i.e. their `spec_path` is set to that. So it no longer looks to `AddressSpecs` like there are any targets in each subdirectory, and there are >1 `_go_internal_package` targets in the `go_mod` dir.

Instead, we should use the `subpath` field to determine what directory the targets correspond to.

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
Eric-Arellano authored Oct 6, 2021
1 parent 221dc1b commit 0044d1d
Show file tree
Hide file tree
Showing 21 changed files with 351 additions and 405 deletions.
4 changes: 2 additions & 2 deletions src/python/pants/backend/experimental/go/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
from pants.backend.go.target_types import (
GoBinaryTarget,
GoExternalPackageTarget,
GoInternalPackageTarget,
GoModTarget,
GoPackage,
)
from pants.backend.go.util_rules import (
assembly,
Expand All @@ -28,7 +28,7 @@


def target_types():
return [GoModTarget, GoPackage, GoExternalPackageTarget, GoBinaryTarget]
return [GoInternalPackageTarget, GoModTarget, GoExternalPackageTarget, GoBinaryTarget]


def rules():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from pants.backend.go import target_type_rules
from pants.backend.go.goals import package_binary
from pants.backend.go.goals.package_binary import GoBinaryFieldSet
from pants.backend.go.target_types import GoBinaryTarget, GoModTarget, GoPackage
from pants.backend.go.target_types import GoBinaryTarget, GoModTarget
from pants.backend.go.util_rules import (
assembly,
build_go_pkg,
Expand Down Expand Up @@ -50,7 +50,7 @@ def rule_runner() -> RuleRunner:
*sdk.rules(),
QueryRule(BuiltPackage, (GoBinaryFieldSet,)),
],
target_types=[GoBinaryTarget, GoPackage, GoModTarget],
target_types=[GoBinaryTarget, GoModTarget],
)
rule_runner.set_options([], env_inherit={"PATH"})
return rule_runner
Expand Down Expand Up @@ -88,7 +88,6 @@ def test_package_simple(rule_runner: RuleRunner) -> None:
"BUILD": dedent(
"""\
go_mod(name='mod')
go_package(name='pkg')
go_binary(name='bin')
"""
),
Expand Down Expand Up @@ -125,7 +124,6 @@ def test_package_with_dependencies(rule_runner: RuleRunner) -> None:
}
"""
),
"lib/BUILD": "go_package()",
"main.go": dedent(
"""\
package main
Expand Down Expand Up @@ -165,8 +163,8 @@ def test_package_with_dependencies(rule_runner: RuleRunner) -> None:
"BUILD": dedent(
"""\
go_mod(name='mod')
go_package(name='pkg')
go_binary(name='bin')
# TODO: Remove `main` once it's fixed for target gen.
go_binary(name='bin', main='//:mod#./')
"""
),
}
Expand Down
40 changes: 5 additions & 35 deletions src/python/pants/backend/go/goals/tailor.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import os
from dataclasses import dataclass

from pants.backend.go.target_types import GoModTarget, GoPackage
from pants.backend.go.target_types import GoModTarget
from pants.core.goals.tailor import (
AllOwnedSources,
PutativeTarget,
Expand All @@ -19,32 +19,6 @@
from pants.util.logging import LogLevel


@dataclass(frozen=True)
class PutativeGoPackageTargetsRequest(PutativeTargetsRequest):
pass


@rule(level=LogLevel.DEBUG, desc="Determine candidate Go `go_package` targets to create")
async def find_putative_go_package_targets(
request: PutativeGoPackageTargetsRequest, all_owned_sources: AllOwnedSources
) -> PutativeTargets:
all_go_files = await Get(Paths, PathGlobs, request.search_paths.path_globs("*.go"))
unowned_go_files = set(all_go_files.files) - set(all_owned_sources)

putative_targets = []
for dirname, filenames in group_by_dir(unowned_go_files).items():
putative_targets.append(
PutativeTarget.for_target_type(
GoPackage,
dirname,
os.path.basename(dirname),
sorted(filenames),
)
)

return PutativeTargets(putative_targets)


@dataclass(frozen=True)
class PutativeGoModuleTargetsRequest(PutativeTargetsRequest):
pass
Expand All @@ -62,18 +36,14 @@ async def find_putative_go_mod_targets(
putative_targets.append(
PutativeTarget.for_target_type(
GoModTarget,
dirname,
os.path.basename(dirname),
sorted(filenames),
path=dirname,
name=os.path.basename(dirname),
triggering_sources=sorted(filenames),
)
)

return PutativeTargets(putative_targets)


def rules():
return [
*collect_rules(),
UnionRule(PutativeTargetsRequest, PutativeGoPackageTargetsRequest),
UnionRule(PutativeTargetsRequest, PutativeGoModuleTargetsRequest),
]
return [*collect_rules(), UnionRule(PutativeTargetsRequest, PutativeGoModuleTargetsRequest)]
50 changes: 9 additions & 41 deletions src/python/pants/backend/go/goals/tailor_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,63 +3,27 @@

import pytest

from pants.backend.go import target_type_rules
from pants.backend.go.goals.tailor import (
PutativeGoModuleTargetsRequest,
PutativeGoPackageTargetsRequest,
)
from pants.backend.go.goals.tailor import PutativeGoModuleTargetsRequest
from pants.backend.go.goals.tailor import rules as go_tailor_rules
from pants.backend.go.target_types import GoModTarget, GoPackage
from pants.backend.go.util_rules import external_pkg, go_mod, sdk
from pants.backend.go.target_types import GoModTarget
from pants.core.goals.tailor import (
AllOwnedSources,
PutativeTarget,
PutativeTargets,
PutativeTargetsSearchPaths,
)
from pants.core.goals.tailor import rules as core_tailor_rules
from pants.core.util_rules import external_tool, source_files
from pants.engine.rules import QueryRule
from pants.testutil.rule_runner import RuleRunner


@pytest.fixture
def rule_runner() -> RuleRunner:
rule_runner = RuleRunner(
return RuleRunner(
rules=[
*core_tailor_rules(),
*go_tailor_rules(),
*external_tool.rules(),
*source_files.rules(),
*external_pkg.rules(),
*go_mod.rules(),
*sdk.rules(),
*target_type_rules.rules(),
QueryRule(PutativeTargets, [PutativeGoPackageTargetsRequest, AllOwnedSources]),
QueryRule(PutativeTargets, [PutativeGoModuleTargetsRequest, AllOwnedSources]),
],
target_types=[GoPackage, GoModTarget],
)
return rule_runner


def test_find_putative_go_package_targets(rule_runner: RuleRunner) -> None:
rule_runner.write_files(
{
"src/go/owned/BUILD": "go_package()\n",
"src/go/owned/src.go": "package owned\n",
"src/go/unowned/src.go": "package unowned\n",
}
)
putative_targets = rule_runner.request(
PutativeTargets,
[
PutativeGoPackageTargetsRequest(PutativeTargetsSearchPaths(("src/",))),
AllOwnedSources(["src/go/owned/src.go"]),
],
)
assert putative_targets == PutativeTargets(
[PutativeTarget.for_target_type(GoPackage, "src/go/unowned", "unowned", ["src.go"])]
target_types=[GoModTarget],
)


Expand All @@ -79,5 +43,9 @@ def test_find_putative_go_mod_targets(rule_runner: RuleRunner) -> None:
],
)
assert putative_targets == PutativeTargets(
[PutativeTarget.for_target_type(GoModTarget, "src/go/unowned", "unowned", ["go.mod"])]
[
PutativeTarget.for_target_type(
GoModTarget, path="src/go/unowned", name="unowned", triggering_sources=["go.mod"]
)
]
)
10 changes: 7 additions & 3 deletions src/python/pants/backend/go/goals/test.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).
from pants.backend.go.target_types import GoPackageSources

from __future__ import annotations

from pants.backend.go.target_types import GoInternalPackageSourcesField
from pants.core.goals.test import TestDebugRequest, TestFieldSet, TestResult
from pants.engine.rules import collect_rules, rule
from pants.engine.unions import UnionRule


class GoTestFieldSet(TestFieldSet):
required_fields = (GoPackageSources,)
sources: GoPackageSources
required_fields = (GoInternalPackageSourcesField,)

sources: GoInternalPackageSourcesField


@rule
Expand Down
33 changes: 19 additions & 14 deletions src/python/pants/backend/go/goals/test_test.py
Original file line number Diff line number Diff line change
@@ -1,41 +1,46 @@
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import annotations

import pytest

from pants.backend.go import target_type_rules
from pants.backend.go.goals.test import GoTestFieldSet
from pants.backend.go.goals.test import rules as test_rules
from pants.backend.go.target_types import GoModTarget, GoPackage
from pants.backend.go.target_types import GoModTarget
from pants.backend.go.util_rules import external_pkg, go_mod, go_pkg, sdk
from pants.build_graph.address import Address
from pants.core.goals.test import TestResult
from pants.engine.internals.scheduler import ExecutionError
from pants.engine.rules import QueryRule
from pants.testutil.rule_runner import RuleRunner
from pants.testutil.rule_runner import QueryRule, RuleRunner, engine_error


@pytest.fixture
def rule_runner() -> RuleRunner:
rule_runner = RuleRunner(
rules=[
*test_rules(),
*go_mod.rules(),
*go_pkg.rules(),
*external_pkg.rules(),
*sdk.rules(),
*target_type_rules.rules(),
QueryRule(TestResult, [GoTestFieldSet]),
],
target_types=[GoPackage, GoModTarget],
target_types=[GoModTarget],
)
rule_runner.set_options([], env_inherit={"PATH"})
return rule_runner


def test_stub_is_a_stub(rule_runner: RuleRunner) -> None:
rule_runner.write_files(
{
"foo/BUILD": "go_mod()\ngo_package(name='lib')\n",
"foo/go.mod": "module foo\n",
"foo/go.sum": "",
"foo/bar_test.go": "package foo\n",
"foo/BUILD": "go_mod()",
"foo/go.mod": "module foo",
"foo/bar_test.go": "package foo",
}
)

with pytest.raises(ExecutionError) as exc_info:
tgt = rule_runner.get_target(Address("foo", target_name="lib"))
tgt = rule_runner.get_target(Address("foo", generated_name="./"))
with engine_error(NotImplementedError):
rule_runner.request(TestResult, [GoTestFieldSet.create(tgt)])

assert "NotImplementedError: This is a stub." in str(exc_info.value)
8 changes: 5 additions & 3 deletions src/python/pants/backend/go/lint/fmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from dataclasses import dataclass
from typing import Iterable

from pants.backend.go.target_types import GoSources
from pants.backend.go.target_types import GoInternalPackageSourcesField
from pants.core.goals.fmt import FmtResult, LanguageFmtResults, LanguageFmtTargets
from pants.core.goals.style_request import StyleRequest
from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest
Expand All @@ -18,7 +18,7 @@
# While gofmt is one of those, this is more generic and can work with other formatters.
@dataclass(frozen=True)
class GoLangFmtTargets(LanguageFmtTargets):
required_fields = (GoSources,)
required_fields = (GoInternalPackageSourcesField,)


@union
Expand All @@ -32,7 +32,9 @@ async def format_golang_targets(
) -> LanguageFmtResults:
original_sources = await Get(
SourceFiles,
SourceFilesRequest(target[GoSources] for target in go_fmt_targets.targets),
SourceFilesRequest(
target[GoInternalPackageSourcesField] for target in go_fmt_targets.targets
),
)
prior_formatter_result = original_sources.snapshot

Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/go/lint/gofmt/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from pants.backend.go.lint.gofmt.subsystem import GofmtSubsystem
from pants.backend.go.subsystems import golang
from pants.backend.go.subsystems.golang import GoRoot
from pants.backend.go.target_types import GoSources
from pants.backend.go.target_types import GoInternalPackageSourcesField
from pants.core.goals.fmt import FmtResult
from pants.core.goals.lint import LintRequest, LintResult, LintResults
from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest
Expand All @@ -28,9 +28,9 @@

@dataclass(frozen=True)
class GofmtFieldSet(FieldSet):
required_fields = (GoSources,)
required_fields = (GoInternalPackageSourcesField,)

sources: GoSources
sources: GoInternalPackageSourcesField

@classmethod
def opt_out(cls, tgt: Target) -> bool:
Expand Down
Loading

0 comments on commit 0044d1d

Please sign in to comment.