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

Add python_resolve field to protobuf_source and thrift_source to support multiple resolves with codegen #14693

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -5,14 +5,18 @@
ProtobufSourcesGeneratorTarget,
ProtobufSourceTarget,
)
from pants.backend.python.target_types import InterpreterConstraintsField
from pants.backend.python.target_types import InterpreterConstraintsField, PythonResolveField
from pants.engine.target import StringField


class ProtobufPythonInterpreterConstraints(InterpreterConstraintsField):
class ProtobufPythonInterpreterConstraintsField(InterpreterConstraintsField):
alias = "python_interpreter_constraints"


class ProtobufPythonResolveField(PythonResolveField):
alias = "python_resolve"


class PythonSourceRootField(StringField):
alias = "python_source_root"
help = (
Expand All @@ -23,8 +27,12 @@ class PythonSourceRootField(StringField):

def rules():
return [
ProtobufSourceTarget.register_plugin_field(ProtobufPythonInterpreterConstraints),
ProtobufSourcesGeneratorTarget.register_plugin_field(ProtobufPythonInterpreterConstraints),
ProtobufSourceTarget.register_plugin_field(ProtobufPythonInterpreterConstraintsField),
ProtobufSourcesGeneratorTarget.register_plugin_field(
ProtobufPythonInterpreterConstraintsField
),
ProtobufSourceTarget.register_plugin_field(ProtobufPythonResolveField),
ProtobufSourcesGeneratorTarget.register_plugin_field(ProtobufPythonResolveField),
ProtobufSourceTarget.register_plugin_field(PythonSourceRootField),
ProtobufSourcesGeneratorTarget.register_plugin_field(PythonSourceRootField),
]
Expand Up @@ -11,6 +11,8 @@
from pants.backend.python.goals import lockfile
from pants.backend.python.goals.lockfile import GeneratePythonLockfile
from pants.backend.python.subsystems.python_tool_base import PythonToolRequirementsBase
from pants.backend.python.subsystems.setup import PythonSetup
from pants.backend.python.target_types import PythonResolveField
from pants.core.goals.generate_lockfiles import GenerateToolLockfileSentinel
from pants.engine.addresses import Address, Addresses, UnparsedAddressInputs
from pants.engine.rules import Get, collect_rules, rule
Expand Down Expand Up @@ -63,6 +65,10 @@ class PythonProtobufSubsystem(Subsystem):
"`protobuf` module (usually from the `protobuf` requirement). If the `protobuf_source` "
"target sets `grpc=True`, will also add a dependency on the `python_requirement` "
"target exposing the `grpcio` module.\n\n"
"If `[python].enable_resolves` is set, Pants will only infer dependencies on "
"`python_requirement` targets that use the same resolve as the particular "
"`protobuf_source` / `protobuf_sources` target uses, which is set via its "
"`python_resolve` field.\n\n"
"Unless this option is disabled, Pants will error if no relevant target is found or "
"if more than one is found which causes ambiguity."
),
Expand Down Expand Up @@ -112,6 +118,7 @@ class InjectPythonProtobufDependencies(InjectDependenciesRequest):
async def inject_dependencies(
request: InjectPythonProtobufDependencies,
python_protobuf: PythonProtobufSubsystem,
python_setup: PythonSetup,
# TODO(#12946): Make this a lazy Get once possible.
module_mapping: ThirdPartyPythonModuleMapping,
) -> InjectedDependencies:
Expand All @@ -124,25 +131,32 @@ async def inject_dependencies(
if not python_protobuf.infer_runtime_dependency:
return InjectedDependencies()

wrapped_tgt = await Get(WrappedTarget, Address, request.dependencies_field.address)
tgt = wrapped_tgt.target
resolve = tgt.get(PythonResolveField).normalized_value(python_setup)

result = [
find_python_runtime_library_or_raise_error(
module_mapping,
request.dependencies_field.address,
"google.protobuf",
resolve=resolve,
resolves_enabled=python_setup.enable_resolves,
recommended_requirement_name="protobuf",
recommended_requirement_url="https://pypi.org/project/protobuf/",
disable_inference_option=f"[{python_protobuf.options_scope}].infer_runtime_dependency",
)
]

wrapped_tgt = await Get(WrappedTarget, Address, request.dependencies_field.address)
if wrapped_tgt.target.get(ProtobufGrpcToggleField).value:
result.append(
find_python_runtime_library_or_raise_error(
module_mapping,
request.dependencies_field.address,
# Note that the library is called `grpcio`, but the module is `grpc`.
"grpc",
resolve=resolve,
resolves_enabled=python_setup.enable_resolves,
recommended_requirement_name="grpcio",
recommended_requirement_url="https://pypi.org/project/grpcio/",
disable_inference_option=f"[{python_protobuf.options_scope}].infer_runtime_dependency",
Expand Down
Expand Up @@ -33,6 +33,9 @@ def test_find_protobuf_python_requirement() -> None:
rule_runner.write_files(
{"codegen/dir/f.proto": "", "codegen/dir/BUILD": "protobuf_sources(grpc=True)"}
)
rule_runner.set_options(
["--python-resolves={'python-default': '', 'another': ''}", "--python-enable-resolves"]
)
proto_tgt = rule_runner.get_target(Address("codegen/dir", relative_file_path="f.proto"))
request = InjectPythonProtobufDependencies(proto_tgt[Dependencies])

Expand All @@ -49,7 +52,20 @@ def test_find_protobuf_python_requirement() -> None:
[Address("proto1"), Address("grpc1")]
)

# If multiple, error.
# Multiple is fine if from other resolve.
rule_runner.write_files(
{
"another_resolve/BUILD": (
"python_requirement(name='r1', requirements=['protobuf'], resolve='another')\n"
"python_requirement(name='r2', requirements=['grpc'], resolve='another')\n"
)
}
)
assert rule_runner.request(InjectedDependencies, [request]) == InjectedDependencies(
[Address("proto1"), Address("grpc1")]
)

# If multiple from the same resolve, error.
rule_runner.write_files({"grpc2/BUILD": "python_requirement(requirements=['grpc'])"})
with engine_error(
AmbiguousPythonCodegenRuntimeLibrary, contains="['grpc1:grpc1', 'grpc2:grpc2']"
Expand Down
@@ -0,0 +1,19 @@
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.backend.codegen.thrift.target_types import (
ThriftSourcesGeneratorTarget,
ThriftSourceTarget,
)
from pants.backend.python.target_types import PythonResolveField


class ThriftPythonResolveField(PythonResolveField):
alias = "python_resolve"


def rules():
return [
ThriftSourceTarget.register_plugin_field(ThriftPythonResolveField),
ThriftSourcesGeneratorTarget.register_plugin_field(ThriftPythonResolveField),
]
@@ -1,7 +1,10 @@
# Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.backend.codegen.thrift.apache.python import python_thrift_module_mapper
from pants.backend.codegen.thrift.apache.python import (
additional_fields,
python_thrift_module_mapper,
)
from pants.backend.codegen.thrift.apache.python.rules import rules as apache_thrift_python_rules
from pants.backend.codegen.thrift.apache.rules import rules as apache_thrift_rules
from pants.backend.codegen.thrift.rules import rules as thrift_rules
Expand All @@ -19,6 +22,7 @@ def target_types():

def rules():
return [
*additional_fields.rules(),
*thrift_rules(),
*apache_thrift_rules(),
*apache_thrift_python_rules(),
Expand Down
14 changes: 11 additions & 3 deletions src/python/pants/backend/codegen/thrift/apache/python/rules.py
Expand Up @@ -12,15 +12,17 @@
from pants.backend.codegen.thrift.target_types import ThriftDependenciesField, ThriftSourceField
from pants.backend.codegen.utils import find_python_runtime_library_or_raise_error
from pants.backend.python.dependency_inference.module_mapper import ThirdPartyPythonModuleMapping
from pants.backend.python.target_types import PythonSourceField
from pants.backend.python.subsystems.setup import PythonSetup
from pants.backend.python.target_types import PythonResolveField, PythonSourceField
from pants.engine.addresses import Address
from pants.engine.fs import AddPrefix, Digest, Snapshot
from pants.engine.internals.selectors import Get
from pants.engine.rules import collect_rules, rule
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import (
GeneratedSources,
GenerateSourcesRequest,
InjectDependenciesRequest,
InjectedDependencies,
WrappedTarget,
)
from pants.engine.unions import UnionRule
from pants.source.source_root import SourceRoot, SourceRootRequest
Expand Down Expand Up @@ -69,16 +71,22 @@ class InjectApacheThriftPythonDependencies(InjectDependenciesRequest):
async def find_apache_thrift_python_requirement(
request: InjectApacheThriftPythonDependencies,
thrift_python: ThriftPythonSubsystem,
python_setup: PythonSetup,
# TODO(#12946): Make this a lazy Get once possible.
module_mapping: ThirdPartyPythonModuleMapping,
) -> InjectedDependencies:
if not thrift_python.infer_runtime_dependency:
return InjectedDependencies()

wrapped_tgt = await Get(WrappedTarget, Address, request.dependencies_field.address)
resolve = wrapped_tgt.target.get(PythonResolveField).normalized_value(python_setup)

addr = find_python_runtime_library_or_raise_error(
module_mapping,
request.dependencies_field.address,
"thrift",
resolve=resolve,
resolves_enabled=python_setup.enable_resolves,
recommended_requirement_name="thrift",
recommended_requirement_url="https://pypi.org/project/thrift/",
disable_inference_option=f"[{thrift_python.options_scope}].infer_runtime_dependency",
Expand Down
Expand Up @@ -182,6 +182,9 @@ def test_top_level_source_root(rule_runner: RuleRunner) -> None:

def test_find_thrift_python_requirement(rule_runner: RuleRunner) -> None:
rule_runner.write_files({"codegen/dir/f.thrift": "", "codegen/dir/BUILD": "thrift_sources()"})
rule_runner.set_options(
["--python-resolves={'python-default': '', 'another': ''}", "--python-enable-resolves"]
)
thrift_tgt = rule_runner.get_target(Address("codegen/dir", relative_file_path="f.thrift"))
request = InjectApacheThriftPythonDependencies(thrift_tgt[Dependencies])

Expand All @@ -195,7 +198,15 @@ def test_find_thrift_python_requirement(rule_runner: RuleRunner) -> None:
[Address("reqs1")]
)

# If multiple, error.
# Multiple is fine if from other resolve.
rule_runner.write_files(
{"another_resolve/BUILD": "python_requirement(requirements=['thrift'], resolve='another')"}
)
assert rule_runner.request(InjectedDependencies, [request]) == InjectedDependencies(
[Address("reqs1")]
)

# If multiple from the same resolve, error.
rule_runner.write_files({"reqs2/BUILD": "python_requirement(requirements=['thrift'])"})
with engine_error(
AmbiguousPythonCodegenRuntimeLibrary, contains="['reqs1:reqs1', 'reqs2:reqs2']"
Expand Down
Expand Up @@ -26,6 +26,10 @@ class ThriftPythonSubsystem(Subsystem):
help=(
"If True, will add a dependency on a `python_requirement` target exposing the `thrift` "
"module (usually from the `thrift` requirement).\n\n"
"If `[python].enable_resolves` is set, Pants will only infer dependencies on "
"`python_requirement` targets that use the same resolve as the particular "
"`thrift_source` / `thrift_source` target uses, which is set via its "
"`python_resolve` field.\n\n"
"Unless this option is disabled, Pants will error if no relevant target is found or "
"more than one is found which causes ambiguity."
),
Expand Down
49 changes: 37 additions & 12 deletions src/python/pants/backend/codegen/utils.py
Expand Up @@ -24,39 +24,64 @@ def find_python_runtime_library_or_raise_error(
codegen_address: Address,
runtime_library_module: str,
*,
resolve: str,
resolves_enabled: bool,
recommended_requirement_name: str,
recommended_requirement_url: str,
disable_inference_option: str,
) -> Address:
addresses = [
module_provider.addr
for module_provider in module_mapping.providers_for_module(
runtime_library_module, resolve=None
runtime_library_module, resolve=resolve
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder that if --no-enable-resolves, Pants will have set the resolve name to <ignore-me> or something like that. So everything ends up being in the same resolve.

)
if module_provider.typ == ModuleProviderType.IMPL
]
if len(addresses) == 1:
return addresses[0]

for_resolve_str = f" for the resolve '{resolve}'" if resolves_enabled else ""
if not addresses:
resolve_note = (
(
"Note that because `[python].enable_resolves` is set, you must specifically have a "
f"`python_requirement` target that uses the same resolve '{resolve}' as the target "
f"{codegen_address}. Alternatively, update {codegen_address} to use a different "
"resolve.\n\n"
)
if resolves_enabled
else ""
)
raise MissingPythonCodegenRuntimeLibrary(
f"No `python_requirement` target was found with the module `{runtime_library_module}` "
f"in your project, so the Python code generated from the target {codegen_address} will "
f"not work properly. See {doc_url('python-third-party-dependencies')} for how to "
f"in your project{for_resolve_str}, so the Python code generated from the target "
f"{codegen_address} will not work properly. See "
f"{doc_url('python-third-party-dependencies')} for how to "
"add a requirement, such as adding to requirements.txt. Usually you will want to use "
f"the `{recommended_requirement_name}` project at {recommended_requirement_url}.\n\n"
f"{resolve_note}"
f"To ignore this error, set `{disable_inference_option} = false` in `pants.toml`."
)

if len(addresses) > 1:
raise AmbiguousPythonCodegenRuntimeLibrary(
"Multiple `python_requirement` targets were found with the module "
f"`{runtime_library_module}` in your project, so it is ambiguous which to use for the "
f"runtime library for the Python code generated from the the target {codegen_address}: "
f"{sorted(addr.spec for addr in addresses)}\n\n"
"To fix, remove one of these `python_requirement` targets so that there is no "
"ambiguity and Pants can infer a dependency. Alternatively, if you do want to have "
alternative_solution = (
(
f"Alternatively, change the resolve field for {codegen_address} to use a different "
"resolve from `[python].resolves`."
)
if resolves_enabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If resolves are enabled, then you must not have two python_requirement targets for the same project in the resolve. That is nonsensical.

else (
"Alternatively, if you do want to have "
f"multiple conflicting versions of the `{runtime_library_module}` requirement, set "
f"`{disable_inference_option} = false` in `pants.toml`. "
f"Then manually add a dependency on the relevant `python_requirement` target to each "
"target that directly depends on this generated code (e.g. `python_source` targets)."
)
return addresses[0]
)
raise AmbiguousPythonCodegenRuntimeLibrary(
"Multiple `python_requirement` targets were found with the module "
f"`{runtime_library_module}` in your project{for_resolve_str}, so it is ambiguous which to "
f"use for the runtime library for the Python code generated from the the target "
f"{codegen_address}: {sorted(addr.spec for addr in addresses)}\n\n"
f"To fix, remove one of these `python_requirement` targets{for_resolve_str} so that "
f"there is no ambiguity and Pants can infer a dependency. {alternative_solution}"
)