Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[internal] Fix go_binary's main field inference to work with generated targets #13163

Merged
merged 1 commit into from
Oct 8, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,7 @@ def test_package_with_dependencies(rule_runner: RuleRunner) -> None:
"BUILD": dedent(
"""\
go_mod(name='mod')
# TODO: Remove `main` once it's fixed for target gen.
go_binary(name='bin', main='//:mod#./')
go_binary(name='bin')
"""
),
}
Expand Down
40 changes: 23 additions & 17 deletions src/python/pants/backend/go/target_type_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
from pants.backend.go.util_rules.go_pkg import ResolvedGoPackage, ResolveGoPackageRequest
from pants.backend.go.util_rules.import_analysis import GoStdLibImports
from pants.base.exceptions import ResolveError
from pants.base.specs import AddressSpecs, DescendantAddresses, SiblingAddresses
from pants.base.specs import AddressSpecs, AscendantAddresses, DescendantAddresses
from pants.core.goals.tailor import group_by_dir
from pants.engine.addresses import Address, AddressInput
from pants.engine.fs import PathGlobs, Paths
Expand Down Expand Up @@ -291,34 +291,40 @@ async def determine_main_pkg_for_go_binary(
if not wrapped_specified_tgt.target.has_field(GoInternalPackageSourcesField):
raise InvalidFieldException(
f"The {repr(GoBinaryMainPackageField.alias)} field in target {addr} must point to "
"a `go_package` target, but was the address for a "
"a `_go_internal_package` target, but was the address for a "
f"`{wrapped_specified_tgt.target.alias}` target.\n\n"
"Hint: consider leaving off this field so that Pants will find the `go_package` "
"target for you."
"Hint: consider leaving off this field so that Pants will find the "
"`_go_internal_package` target for you."
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this error explain to the user that _go_internal_package is a hidden target type that is auto-generated for them? Some of the other error messages in this PR do mention that fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. Will audit all the error messages when renaming these targets.

)
return GoBinaryMainPackage(wrapped_specified_tgt.target.address)

# TODO: fix this to account for `_go_internal_package` being generated.
build_dir_targets = await Get(Targets, AddressSpecs([SiblingAddresses(addr.spec_path)]))
internal_pkg_targets = [
tgt for tgt in build_dir_targets if tgt.has_field(GoInternalPackageSourcesField)
candidate_targets = await Get(Targets, AddressSpecs([AscendantAddresses(addr.spec_path)]))
relevant_pkg_targets = [
tgt
for tgt in candidate_targets
if (
tgt.has_field(GoInternalPackageSubpathField)
and tgt[GoInternalPackageSubpathField].full_dir_path == addr.spec_path
)
]
if len(internal_pkg_targets) == 1:
return GoBinaryMainPackage(internal_pkg_targets[0].address)
if len(relevant_pkg_targets) == 1:
return GoBinaryMainPackage(relevant_pkg_targets[0].address)

wrapped_tgt = await Get(WrappedTarget, Address, addr)
alias = wrapped_tgt.target.alias
if not internal_pkg_targets:
if not relevant_pkg_targets:
raise ResolveError(
f"The `{alias}` target {addr} requires that there is a `go_package` "
"target in the same directory, but none were found."
f"The `{alias}` target {addr} requires that there is a `_go_internal_package` "
f"target for its directory {addr.spec_path}, but none were found.\n\n"
"Have you added a `go_mod` target (which will generate `_go_internal_package` targets)?"
)
raise ResolveError(
f"There are multiple `go_package` targets in the same directory of the `{alias}` "
f"target {addr}, so it is ambiguous what to use as the `main` package.\n\n"
f"There are multiple `_go_internal_package` targets for the same directory of the "
"`{alias}` target {addr}: {addr.spec_path}. It is ambiguous what to use as the `main` "
"package.\n\n"
f"To fix, please either set the `main` field for `{addr} or remove these "
"`go_package` targets so that only one remains: "
f"{sorted(tgt.address.spec for tgt in internal_pkg_targets)}"
"`_go_internal_package` targets so that only one remains: "
f"{sorted(tgt.address.spec for tgt in relevant_pkg_targets)}"
)


Expand Down
42 changes: 24 additions & 18 deletions src/python/pants/backend/go/target_type_rules_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,29 +263,29 @@ def gen_external_tgt(mod_path: str, version: str, import_path: str) -> GoExterna
# -----------------------------------------------------------------------------------------------


@pytest.mark.xfail
def test_determine_main_pkg_for_go_binary(rule_runner: RuleRunner) -> None:
rule_runner.write_files(
{
"explicit/BUILD": dedent(
"go.mod": dedent(
"""\
go_package(name='pkg', sources=[])
go_binary(main=':pkg')
"""
),
"inferred/BUILD": dedent(
"""\
go_package(name='pkg', sources=[])
go_binary()
module example.com/foo
go 1.17
"""
),
"ambiguous/BUILD": dedent(
"BUILD": "go_mod(name='mod')",
"explicit/f.go": "",
"explicit/BUILD": "go_binary(main='//:mod#./explicit')",
"inferred/f.go": "",
"inferred/BUILD": "go_binary()",
"ambiguous/f.go": "",
"ambiguous/go.mod": dedent(
"""\
go_package(name='pkg1', sources=[])
go_package(name='pkg2', sources=[])
go_binary()
module example.com/ambiguous
go 1.17
"""
),
"ambiguous/BUILD": "go_binary()",
# Note there are no `.go` files in this dir, so no package targets will be created.
"missing/BUILD": "go_binary()",
"explicit_wrong_type/BUILD": dedent(
"""\
Expand All @@ -307,12 +307,18 @@ def get_main(addr: Address) -> Address:
assert [main_addr] == list(injected_addresses)
return main_addr

assert get_main(Address("explicit")) == Address("explicit", target_name="pkg")
assert get_main(Address("inferred")) == Address("inferred", target_name="pkg")
assert get_main(Address("explicit")) == Address(
"", target_name="mod", generated_name="./explicit"
)
assert get_main(Address("inferred")) == Address(
"", target_name="mod", generated_name="./inferred"
)

with engine_error(ResolveError, contains="none were found"):
get_main(Address("missing"))
with engine_error(ResolveError, contains="There are multiple `go_package` targets"):
with engine_error(ResolveError, contains="There are multiple `_go_internal_package` targets"):
get_main(Address("ambiguous"))
with engine_error(InvalidFieldException, contains="must point to a `go_package` target"):
with engine_error(
InvalidFieldException, contains="must point to a `_go_internal_package` target"
):
get_main(Address("explicit_wrong_type"))
21 changes: 17 additions & 4 deletions src/python/pants/backend/go/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class GoInternalPackageDependenciesField(Dependencies):
pass


class GoInternalPackageSubpathField(StringField):
class GoInternalPackageSubpathField(StringField, AsyncFieldMixin):
alias = "subpath"
help = (
"The path from the owning `go.mod` to this package's directory, e.g. `subdir`.\n\n"
Expand All @@ -144,6 +144,19 @@ class GoInternalPackageSubpathField(StringField):
required = True
value: str

@property
def full_dir_path(self) -> str:
"""The full path to this package's directory, relative to the build root."""
# NB: Assumes that the `spec_path` points to the ancestor `go.mod`, which will be the
# case when `go_mod` targets generate.
if not self.address.is_generated_target:
# TODO: Make this error more eager via target validation.
raise AssertionError(
f"Target was manually created, but expected to be generated: {self.address}"
)
Comment on lines +152 to +156
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tdyas I've been thinking a lot the past few days about whether to keep _go_{first,third}_party_package targets private.

The downside is they won't show up in ./pants help and the docs, and I think it would be helpful for people to understand what the atoms of their builds are. In particular, it will be important when adding an overrides mechanism to go_mod.

I think the answer is to make them public, but use eager validation that they are never explicitly created.

Copy link
Contributor

Choose a reason for hiding this comment

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

I share your concern with that downside. Letting users understand the magic even a little is probably beneficial, even if the targets should be auto-generated in the standard case. Should the first-party target then be called go_package again, or keep the longer name so users are less likely to try and use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should still be go_first_party_package, per discussion today. But document that it should not be manually created + add eager validation.

go_mod_path = self.address.spec_path
return os.path.join(go_mod_path, self.value)


class GoInternalPackageTarget(Target):
alias = "_go_internal_package"
Expand Down Expand Up @@ -203,9 +216,9 @@ class GoExternalPackageTarget(Target):
class GoBinaryMainPackageField(StringField, AsyncFieldMixin):
alias = "main"
help = (
"Address of the `go_package` with the `main` for this binary.\n\n"
"If not specified, will default to the `go_package` in the same directory as this target's "
"BUILD file."
"Address of the `_go_internal_package` with the `main` for this binary.\n\n"
"If not specified, will default to the `_go_internal_package` for the same "
"directory as this target's BUILD file."
)
value: str

Expand Down