Skip to content

Commit

Permalink
Make tools exportable (#20730)
Browse files Browse the repository at this point in the history
When we simplified tool lockfiles into user lockfiles, we removed a lot
of boilerplate machinery. But one problem is that we lost the ability to
export tools from their default lockfile, as that lockfile isn't a user
resolve. This MR adds an implementation template for language backends
to hook into. It works as follows:

- A union `ExportableTool` which provides a single point of integration.
We could in theory pull common information up into here, such as the
resolve name
- Language backends use this `ExportableTool` to pull all such tools.
They then filter these based on whether they're subclasses of classes
they know how to export.
- Language backends generate resolves for these tools and add them to
the list of known resolves. This lets Pants know that these are valid
lockfiles
- Language backends, in their implementations of export, construct the
resolve for the tool. This should only happen if the resolve is not
defined by a user, so that users can shadow default tool lockfiles
- Plugin authors must register their tools to the `ExportableTool`
UnionRule. (We could probably do something with `__init_subclass__` but
that sounds janky.) This is a single line, and seems like acceptable
boilerplate to me. In theory this also enables making some tools
non-exportable. An argument can be made that this would allow us to hide
internal tools (such as dependency parsers). I'm not sure how good an
argument this actually is.

Limitations:
- the list of resolves is the same for `generate-lockfiles` and
`export`. Generating lockfiles doesn't make sense for a tool using the
default lockfile, so we have to error late. I've got it print some
helpful error messages though.
- this MR doesn't enroll tools in this new mechanism. I'll split that
out to separate MRs, per-backend, so it's easier to roll back if needed.
  • Loading branch information
lilatomic authored Apr 9, 2024
1 parent ff09e71 commit 3d9e59b
Show file tree
Hide file tree
Showing 15 changed files with 513 additions and 135 deletions.
3 changes: 1 addition & 2 deletions docs/docs/writing-plugins/common-plugin-tasks/index.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,4 @@
- [Add Tests](./run-tests.mdx)
- [Add lockfile support](./plugin-lockfiles.mdx)
- [Custom `setup-py` kwargs](./custom-python-artifact-kwargs.mdx)
- [Plugin upgrade guide](./plugin-upgrade-guide.mdx)
- [Plugin helpers](./plugin-helpers.mdx)
- [Plugin upgrade guide](./plugin-upgrade-guide.mdx)
30 changes: 0 additions & 30 deletions docs/docs/writing-plugins/common-plugin-tasks/plugin-helpers.mdx

This file was deleted.

27 changes: 27 additions & 0 deletions docs/docs/writing-plugins/common-subsystem-tasks.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,30 @@ class FortranLintFieldSet(FieldSet):
def opt_out(cls, tgt: Target) -> bool:
return tgt.get(SkipFortranLintField).value
```

## Making subsystems exportable with their default lockfile

:::note Support depends on language backend of the subsystem
Only some language backends support `pants export`. These include the Python and JVM backends. Only tools which are themselves written to use a backend with this feature can be exported. For example, a Python-based tool which operates on a different language is exportable.


1. Make the subsystem a subclass of `ExportableTool`

:::note Language backends may have done this in their Tool base class. For example, the Python backend with `PythonToolRequirementsBase` and JVM with `JvmToolBase` are already subclasses.

```python
from pants.backend.python.subsystems.python_tool_base import PythonToolBase
from pants.core.goals.resolves import ExportableTool

class FortranLint(PythonToolBase, ExportableTool):
...
```

2. Register your class with a `UnionRule` with `ExportableTool`

```python
def rules():
return [
UnionRule(ExportableTool, FortranLint)
]
```
31 changes: 23 additions & 8 deletions src/python/pants/backend/python/goals/export.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
import uuid
from dataclasses import dataclass
from enum import Enum
from typing import Any
from typing import Any, cast

from pants.backend.python.subsystems.python_tool_base import PythonToolBase
from pants.backend.python.subsystems.setup import PythonSetup
from pants.backend.python.target_types import PexLayout
from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
Expand All @@ -31,12 +32,13 @@
ExportSubsystem,
PostProcessingCommand,
)
from pants.core.goals.resolves import ExportableTool
from pants.engine.engine_aware import EngineAwareParameter
from pants.engine.internals.native_engine import AddPrefix, Digest, MergeDigests, Snapshot
from pants.engine.internals.selectors import Get, MultiGet
from pants.engine.process import ProcessCacheScope, ProcessResult
from pants.engine.rules import collect_rules, rule
from pants.engine.unions import UnionRule
from pants.engine.unions import UnionMembership, UnionRule
from pants.option.option_types import EnumOption, StrListOption
from pants.util.strutil import path_safe, softwrap

Expand Down Expand Up @@ -307,18 +309,31 @@ async def export_virtualenv_for_resolve(
request: _ExportVenvForResolveRequest,
python_setup: PythonSetup,
export_subsys: ExportSubsystem,
union_membership: UnionMembership,
) -> MaybeExportResult:
resolve = request.resolve
lockfile_path = python_setup.resolves.get(resolve)
if not lockfile_path:
if lockfile_path:
lockfile = Lockfile(
url=lockfile_path,
url_description_of_origin=f"the resolve `{resolve}`",
resolve_name=resolve,
)
else:
maybe_exportable = ExportableTool.filter_for_subclasses(
union_membership, PythonToolBase
).get(resolve)
if maybe_exportable:
lockfile = cast(
PythonToolBase, maybe_exportable
).pex_requirements_for_default_lockfile()
else:
lockfile = None

if not lockfile:
raise ExportError(
f"No resolve named {resolve} found in [{python_setup.options_scope}].resolves."
)
lockfile = Lockfile(
url=lockfile_path,
url_description_of_origin=f"the resolve `{resolve}`",
resolve_name=resolve,
)

interpreter_constraints = InterpreterConstraints(
python_setup.resolves_to_interpreter_constraints.get(
Expand Down
14 changes: 0 additions & 14 deletions src/python/pants/backend/python/goals/export_integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import os
import platform
import shutil
from dataclasses import dataclass
from textwrap import dedent
from typing import Mapping, MutableMapping

Expand Down Expand Up @@ -37,19 +36,6 @@
}


@dataclass
class _ToolConfig:
name: str
version: str
experimental: bool = False
backend_prefix: str | None = "lint"
takes_ics: bool = True

@property
def package(self) -> str:
return self.name.replace("-", "_")


def build_config(tmpdir: str, py_resolve_format: PythonResolveExportFormat) -> Mapping:
cfg: MutableMapping = {
"GLOBAL": {
Expand Down
31 changes: 26 additions & 5 deletions src/python/pants/backend/python/goals/export_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from pants.backend.python import target_types_rules
from pants.backend.python.goals import export
from pants.backend.python.goals.export import ExportVenvsRequest, PythonResolveExportFormat
from pants.backend.python.lint.isort import subsystem as isort_subsystem
from pants.backend.python.macros.python_artifact import PythonArtifact
from pants.backend.python.target_types import (
PythonDistribution,
Expand All @@ -19,13 +20,23 @@
from pants.backend.python.util_rules import local_dists_pep660, pex_from_targets
from pants.base.specs import RawSpecs
from pants.core.goals.export import ExportResults
from pants.core.goals.resolves import ExportableTool
from pants.core.util_rules import distdir
from pants.engine.internals.parametrize import Parametrize
from pants.engine.rules import QueryRule
from pants.engine.target import Targets
from pants.engine.unions import UnionRule
from pants.testutil.rule_runner import RuleRunner
from pants.util.frozendict import FrozenDict

pants_args_for_python_lockfiles = [
"--python-enable-resolves=True",
# Turn off lockfile validation to make the test simpler.
"--python-invalid-lockfile-behavior=ignore",
# Turn off python synthetic lockfile targets to make the test simpler.
"--no-python-enable-lockfile-targets",
]


@pytest.fixture
def rule_runner() -> RuleRunner:
Expand All @@ -36,6 +47,10 @@ def rule_runner() -> RuleRunner:
*target_types_rules.rules(),
*distdir.rules(),
*local_dists_pep660.rules(),
*isort_subsystem.rules(), # add a tool that we can try exporting
UnionRule(
ExportableTool, isort_subsystem.Isort
), # TODO: remove this manual export when we add ExportableTool to tools
QueryRule(Targets, [RawSpecs]),
QueryRule(ExportResults, [ExportVenvsRequest]),
],
Expand Down Expand Up @@ -80,15 +95,11 @@ def test_export_venv_new_codepath(
format_flag = f"--export-py-resolve-format={py_resolve_format.value}"
rule_runner.set_options(
[
*pants_args_for_python_lockfiles,
f"--python-interpreter-constraints=['=={current_interpreter}']",
"--python-enable-resolves=True",
"--python-resolves={'a': 'lock.txt', 'b': 'lock.txt'}",
"--export-resolve=a",
"--export-resolve=b",
# Turn off lockfile validation to make the test simpler.
"--python-invalid-lockfile-behavior=ignore",
# Turn off python synthetic lockfile targets to make the test simpler.
"--no-python-enable-lockfile-targets",
"--export-py-editable-in-resolve=['a', 'b']",
format_flag,
],
Expand Down Expand Up @@ -148,3 +159,13 @@ def test_export_venv_new_codepath(
f"python/virtualenvs/a/{current_interpreter}",
f"python/virtualenvs/b/{current_interpreter}",
]


def test_export_tool(rule_runner: RuleRunner) -> None:
"""Test exporting an ExportableTool."""
rule_runner.set_options([*pants_args_for_python_lockfiles, "--export-resolve=isort"])
results = rule_runner.request(ExportResults, [ExportVenvsRequest(tuple())])
assert len(results) == 1
result = results[0]
assert result.resolve == isort_subsystem.Isort.options_scope
assert "isort" in result.description
86 changes: 66 additions & 20 deletions src/python/pants/backend/python/goals/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from dataclasses import dataclass
from operator import itemgetter

from pants.backend.python.subsystems.python_tool_base import PythonToolBase
from pants.backend.python.subsystems.setup import PythonSetup
from pants.backend.python.target_types import (
PythonRequirementFindLinksField,
Expand All @@ -26,6 +27,7 @@
ResolvePexConfigRequest,
)
from pants.core.goals.generate_lockfiles import (
DEFAULT_TOOL_LOCKFILE,
GenerateLockfile,
GenerateLockfileResult,
GenerateLockfilesSubsystem,
Expand All @@ -35,14 +37,16 @@
UserGenerateLockfiles,
WrappedGenerateLockfile,
)
from pants.core.goals.resolves import ExportableTool
from pants.core.util_rules.lockfile_metadata import calculate_invalidation_digest
from pants.engine.fs import CreateDigest, Digest, DigestContents, FileContent, MergeDigests
from pants.engine.internals.synthetic_targets import SyntheticAddressMaps, SyntheticTargetsRequest
from pants.engine.internals.target_adaptor import TargetAdaptor
from pants.engine.process import ProcessCacheScope, ProcessResult
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import AllTargets
from pants.engine.unions import UnionRule
from pants.engine.unions import UnionMembership, UnionRule
from pants.option.subsystem import _construct_subsystem
from pants.util.docutil import bin_name
from pants.util.logging import LogLevel
from pants.util.ordered_set import FrozenOrderedSet
Expand Down Expand Up @@ -198,20 +202,35 @@ class KnownPythonUserResolveNamesRequest(KnownUserResolveNamesRequest):
pass


def python_exportable_tools(union_membership: UnionMembership) -> dict[str, type[PythonToolBase]]:
exportable_tools = union_membership.get(ExportableTool)
names_of_python_tools: dict[str, type[PythonToolBase]] = {
e.options_scope: e for e in exportable_tools if issubclass(e, PythonToolBase) # type: ignore # mypy isn't narrowing with `issubclass`
}
return names_of_python_tools


@rule
def determine_python_user_resolves(
_: KnownPythonUserResolveNamesRequest, python_setup: PythonSetup
_: KnownPythonUserResolveNamesRequest,
python_setup: PythonSetup,
union_membership: UnionMembership,
) -> KnownUserResolveNames:
python_tool_resolves = ExportableTool.filter_for_subclasses(union_membership, PythonToolBase)

return KnownUserResolveNames(
names=tuple(python_setup.resolves.keys()),
names=(*python_setup.resolves.keys(), *python_tool_resolves.keys()),
option_name="[python].resolves",
requested_resolve_names_cls=RequestedPythonUserResolveNames,
)


@rule
async def setup_user_lockfile_requests(
requested: RequestedPythonUserResolveNames, all_targets: AllTargets, python_setup: PythonSetup
requested: RequestedPythonUserResolveNames,
all_targets: AllTargets,
python_setup: PythonSetup,
union_membership: UnionMembership,
) -> UserGenerateLockfiles:
if not (python_setup.enable_resolves and python_setup.resolves_generate_lockfiles):
return UserGenerateLockfiles()
Expand All @@ -225,23 +244,50 @@ async def setup_user_lockfile_requests(
resolve_to_requirements_fields[resolve].add(tgt[PythonRequirementsField])
find_links.update(tgt[PythonRequirementFindLinksField].value or ())

return UserGenerateLockfiles(
GeneratePythonLockfile(
requirements=PexRequirements.req_strings_from_requirement_fields(
resolve_to_requirements_fields[resolve]
),
find_links=FrozenOrderedSet(find_links),
interpreter_constraints=InterpreterConstraints(
python_setup.resolves_to_interpreter_constraints.get(
resolve, python_setup.interpreter_constraints
tools = ExportableTool.filter_for_subclasses(union_membership, PythonToolBase)

out = []
for resolve in requested:
if resolve in python_setup.resolves:
out.append(
GeneratePythonLockfile(
requirements=PexRequirements.req_strings_from_requirement_fields(
resolve_to_requirements_fields[resolve]
),
find_links=FrozenOrderedSet(find_links),
interpreter_constraints=InterpreterConstraints(
python_setup.resolves_to_interpreter_constraints.get(
resolve, python_setup.interpreter_constraints
)
),
resolve_name=resolve,
lockfile_dest=python_setup.resolves[resolve],
diff=False,
)
),
resolve_name=resolve,
lockfile_dest=python_setup.resolves[resolve],
diff=False,
)
for resolve in requested
)
)
else:
tool_cls: type[PythonToolBase] = tools[resolve]
tool = await _construct_subsystem(tool_cls)

# TODO: we shouldn't be managing default ICs in lockfile identification.
# We should find a better place to do this or a better way to default
if tool.register_interpreter_constraints:
ic = tool.interpreter_constraints
else:
ic = InterpreterConstraints(tool.default_interpreter_constraints)

out.append(
GeneratePythonLockfile(
requirements=FrozenOrderedSet(sorted(tool.requirements)),
find_links=FrozenOrderedSet(find_links),
interpreter_constraints=ic,
resolve_name=resolve,
lockfile_dest=DEFAULT_TOOL_LOCKFILE,
diff=False,
)
)

return UserGenerateLockfiles(out)


@dataclass(frozen=True)
Expand Down
4 changes: 3 additions & 1 deletion src/python/pants/backend/python/lint/isort/subsystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,6 @@ def config_request(self, dirs: Iterable[str]) -> ConfigFilesRequest:


def rules():
return collect_rules()
return [
*collect_rules(),
]
Loading

0 comments on commit 3d9e59b

Please sign in to comment.