Remove GenerateToolLockfileSentinel (#21225)

We now use `ExportableTool` instead of `GenerateToolLockfileSentinel` to export internal tools. This results in less boilerplate, a unifying of user and tool lockfiles, and overall less code. This MR removes `GenerateToolLockfileSentinel` and its codepaths. Simplification!
pantsbuild · Oct 12, 2024 · 6afd222 · 6afd222
1 parent 020b2fb
commit 6afd222
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 196 deletions.
diff --git a/docs/notes/2.24.x.md b/docs/notes/2.24.x.md
@@ -16,7 +16,7 @@ We offer [formal sponsorship tiers for companies](https://www.pantsbuild.org/spo
 
 ### Deprecations
 
-- **Python 2.7**: As announced in the v2.23.x release series, Pants v2.24 and later are not proactively tested in CI with Python 2.7 since [Python 2.7 is no longer supported by its maintainers as of 1 January 2020](https://www.python.org/doc/sunset-python-2/). While Pants may continue to work with Python 2.7 in the near term, Pants no longer officially supports use of Python 2.7, and, consequently, any remaining support for Python 2.7 may "bit rot" and diverge over time. Contributions to fix issues with Python 2.7 support will continue to be accepted, but will depend on any community contributions and will not consitute continued official support for Python 2.7.
+- **Python 2.7**: As announced in the v2.23.x release series, Pants v2.24 and later are not proactively tested in CI with Python 2.7 since [Python 2.7 is no longer supported by its maintainers as of 1 January 2020](https://www.python.org/doc/sunset-python-2/). While Pants may continue to work with Python 2.7 in the near term, Pants no longer officially supports use of Python 2.7, and, consequently, any remaining support for Python 2.7 may "bit rot" and diverge over time. Contributions to fix issues with Python 2.7 support will continue to be accepted, but will depend on any community contributions and will not constitute continued official support for Python 2.7.
 - **macOS verisons**: as announced in the v2.23.x release series, Pants v2.24 is built on macOS 12 and so may not work on versions of macOS 10.15 and 11 (which Apple no longer supports).
 
 
@@ -104,6 +104,8 @@ The `path_metadata_request` intrinsic rule can now access metadata for paths in
 
 PyO3, the interface crate between Rust and Python, has been upgraded to v0.22.x. A major change is that all Python values on the Rust side must be handled via either the `pyo3::Bound` or `pyo3::Py` smart pointers; direct references such as `&PyAny` are no longer supported.
 
+`GenerateToolLockfileSentinel` is removed. See the [porting guide details](https://www.pantsbuild.org/2.24/docs/writing-plugins/common-plugin-tasks/plugin-upgrade-guide#deprecated-generatetoollockfilesentinel) for instructions on migrating.
+
 ## Full Changelog
 
 For the full changelog, see the individual GitHub Releases for this series: <https://github.com/pantsbuild/pants/releases>
diff --git a/src/python/pants/backend/python/goals/lockfile.py b/src/python/pants/backend/python/goals/lockfile.py
@@ -216,10 +216,14 @@ def determine_python_user_resolves(
     python_setup: PythonSetup,
     union_membership: UnionMembership,
 ) -> KnownUserResolveNames:
+    """Find all know Python resolves, from both user-created resolves and internal tools."""
     python_tool_resolves = ExportableTool.filter_for_subclasses(union_membership, PythonToolBase)
 
     return KnownUserResolveNames(
-        names=(*python_setup.resolves.keys(), *python_tool_resolves.keys()),
+        names=(
+            *python_setup.resolves.keys(),
+            *python_tool_resolves.keys(),
+        ),  # the order of the keys doesn't matter since shadowing is done in `setup_user_lockfile_requests`
         option_name="[python].resolves",
         requested_resolve_names_cls=RequestedPythonUserResolveNames,
     )
@@ -232,6 +236,11 @@ async def setup_user_lockfile_requests(
     python_setup: PythonSetup,
     union_membership: UnionMembership,
 ) -> UserGenerateLockfiles:
+    """Transform the names of resolves requested into the `GeneratePythonLockfile` request object.
+
+    Shadowing is done here by only checking internal resolves if the resolve is not a user-created
+    resolve.
+    """
     if not (python_setup.enable_resolves and python_setup.resolves_generate_lockfiles):
         return UserGenerateLockfiles()
 

diff --git a/src/python/pants/core/goals/export.py b/src/python/pants/core/goals/export.py
@@ -12,7 +12,6 @@
 
 from pants.base.build_root import BuildRoot
 from pants.core.goals.generate_lockfiles import (
-    GenerateToolLockfileSentinel,
     KnownUserResolveNames,
     KnownUserResolveNamesRequest,
     UnrecognizedResolveNamesError,
@@ -251,10 +250,6 @@ async def export(
         all_valid_resolve_names = sorted(
             {
                 *itertools.chain.from_iterable(kurn.names for kurn in all_known_user_resolve_names),
-                *(
-                    sentinel.resolve_name
-                    for sentinel in union_membership.get(GenerateToolLockfileSentinel)
-                ),
             }
         )
         raise UnrecognizedResolveNamesError(

diff --git a/src/python/pants/core/goals/generate_lockfiles.py b/src/python/pants/core/goals/generate_lockfiles.py
@@ -8,18 +8,7 @@
 from collections import defaultdict
 from dataclasses import dataclass, replace
 from enum import Enum
-from typing import (
-    Callable,
-    ClassVar,
-    Iterable,
-    Iterator,
-    Mapping,
-    Protocol,
-    Sequence,
-    Tuple,
-    Type,
-    cast,
-)
+from typing import Callable, Iterable, Iterator, Mapping, Protocol, Sequence, Tuple, Type, cast
 
 from pants.core.goals.resolves import ExportableTool
 from pants.engine.collection import Collection
@@ -82,24 +71,6 @@ class WrappedGenerateLockfile:
     request: GenerateLockfile
 
 
-@union(in_scope_types=[EnvironmentName])
-class GenerateToolLockfileSentinel:
-    """Tools use this as an entry point to say how to generate their tool lockfile.
-
-    Each language ecosystem should set up a union member of `GenerateLockfile`, like
-    `GeneratePythonLockfile`, as explained in that class's docstring.
-
-    Each language ecosystem should also subclass `GenerateToolLockfileSentinel`, e.g.
-    `GeneratePythonToolLockfileSentinel`. The subclass does not need to do anything - it is only used to know which language ecosystems tools correspond to.
-
-    Then, each tool should subclass their language ecosystem's subclass of `GenerateToolLockfileSentinel` and set up a rule that goes from the
-    subclass -> the language's lockfile request, e.g. BlackLockfileSentinel ->
-    GeneratePythonLockfile. Register `UnionRule(GenerateToolLockfileSentinel, MySubclass)`.
-    """
-
-    resolve_name: ClassVar[str]
-
-
 class UserGenerateLockfiles(Collection[GenerateLockfile]):
     """All user resolves for a particular language ecosystem to build.
 
@@ -371,26 +342,9 @@ def _check_ambiguous_resolve_names(
 
 def determine_resolves_to_generate(
     all_known_user_resolve_names: Iterable[KnownUserResolveNames],
-    all_tool_sentinels: Iterable[type[GenerateToolLockfileSentinel]],
     requested_resolve_names: set[str],
-) -> tuple[list[RequestedUserResolveNames], list[type[GenerateToolLockfileSentinel]]]:
-    """Apply the `--resolve` option to determine which resolves are specified.
-
-    Return a tuple of `(user_resolves, tool_lockfile_sentinels)`.
-    """
-    # Let user resolve names silently shadow tools with the same name.
-    # This is necessary since we now support installing a tool from a named resolve,
-    # and it's not reasonable to ban the name of the tool as the resolve name, when it
-    # is the most obvious choice for that...
-    # This is likely only an issue if you were going to, e.g., have a named resolve called flake8
-    # but not use it as the resolve for the flake8 tool, which seems pretty unlikely.
-    all_known_user_resolve_name_strs = set(
-        itertools.chain.from_iterable(akurn.names for akurn in all_known_user_resolve_names)
-    )
-    all_tool_sentinels = [
-        ts for ts in all_tool_sentinels if ts.resolve_name not in all_known_user_resolve_name_strs
-    ]
-
+) -> list[RequestedUserResolveNames]:
+    """Apply the `--resolve` option to determine which resolves are specified."""
     # Resolve names must be globally unique, so check for ambiguity across backends.
     _check_ambiguous_resolve_names(all_known_user_resolve_names)
 
@@ -399,7 +353,7 @@ def determine_resolves_to_generate(
         return [
             known_resolve_names.requested_resolve_names_cls(known_resolve_names.names)
             for known_resolve_names in all_known_user_resolve_names
-        ], list(all_tool_sentinels)
+        ]
 
     requested_user_resolve_names = []
     for known_resolve_names in all_known_user_resolve_names:
@@ -410,12 +364,6 @@ def determine_resolves_to_generate(
                 known_resolve_names.requested_resolve_names_cls(requested)
             )
 
-    specified_sentinels = []
-    for sentinel in all_tool_sentinels:
-        if sentinel.resolve_name in requested_resolve_names:
-            requested_resolve_names.discard(sentinel.resolve_name)
-            specified_sentinels.append(sentinel)
-
     if requested_resolve_names:
         raise UnrecognizedResolveNamesError(
             unrecognized_resolve_names=sorted(requested_resolve_names),
@@ -424,50 +372,19 @@ def determine_resolves_to_generate(
                     known_resolve_names.names
                     for known_resolve_names in all_known_user_resolve_names
                 ),
-                *(sentinel.resolve_name for sentinel in all_tool_sentinels),
             },
             description_of_origin="the option `--generate-lockfiles-resolve`",
         )
 
-    return requested_user_resolve_names, specified_sentinels
-
-
-def filter_tool_lockfile_requests(
-    specified_requests: Sequence[WrappedGenerateLockfile], *, resolve_specified: bool
-) -> list[GenerateLockfile]:
-    result = []
-    for wrapped_req in specified_requests:
-        req = wrapped_req.request
-
-        if req.lockfile_dest != DEFAULT_TOOL_LOCKFILE:
-            result.append(req)
-            continue
-        if resolve_specified:
-            resolve = req.resolve_name
-            raise ValueError(
-                softwrap(
-                    f"""
-                    You requested to generate a lockfile for {resolve} because
-                    you included it in `--generate-lockfiles-resolve`, but
-                    `[{resolve}].lockfile` is set to `{req.lockfile_dest}`
-                    so a lockfile will not be generated.
-
-                    If you would like to generate a lockfile for {resolve}, please
-                    set `[{resolve}].lockfile` to the path where it should be
-                    generated and run again.
-                    """
-                )
-            )
-
-    return result
+    return requested_user_resolve_names
 
 
 def filter_lockfiles_for_unconfigured_exportable_tools(
     generate_lockfile_requests: Sequence[GenerateLockfile],
     exportabletools_by_name: dict[str, Type[ExportableTool]],
     *,
     resolve_specified: bool,
-) -> Tuple[Sequence[str], Sequence[GenerateLockfile]]:
+) -> tuple[tuple[str, ...], tuple[GenerateLockfile, ...]]:
     """Filter lockfile requests for tools still using their default lockfiles."""
 
     valid_lockfiles = []
@@ -509,7 +426,7 @@ def filter_lockfiles_for_unconfigured_exportable_tools(
             )
             continue
 
-    return errs, valid_lockfiles
+    return tuple(errs), tuple(valid_lockfiles)
 
 
 class GenerateLockfilesSubsystem(GoalSubsystem):
@@ -518,10 +435,7 @@ class GenerateLockfilesSubsystem(GoalSubsystem):
 
     @classmethod
     def activated(cls, union_membership: UnionMembership) -> bool:
-        return (
-            GenerateToolLockfileSentinel in union_membership
-            or KnownUserResolveNamesRequest in union_membership
-        )
+        return KnownUserResolveNamesRequest in union_membership
 
     resolve = StrListOption(
         advanced=False,
@@ -595,9 +509,8 @@ async def generate_lockfiles_goal(
         Get(KnownUserResolveNames, KnownUserResolveNamesRequest, request())
         for request in union_membership.get(KnownUserResolveNamesRequest)
     )
-    requested_user_resolve_names, requested_tool_sentinels = determine_resolves_to_generate(
+    requested_user_resolve_names = determine_resolves_to_generate(
         known_user_resolve_names,
-        union_membership.get(GenerateToolLockfileSentinel),
         set(generate_lockfiles_subsystem.resolve),
     )
 
@@ -610,18 +523,7 @@ async def generate_lockfiles_goal(
         )
         for resolve_names in requested_user_resolve_names
     )
-    specified_tool_requests = await MultiGet(
-        Get(
-            WrappedGenerateLockfile,
-            {sentinel(): GenerateToolLockfileSentinel, local_environment.val: EnvironmentName},
-        )
-        for sentinel in requested_tool_sentinels
-    )
     resolve_specified = bool(generate_lockfiles_subsystem.resolve)
-    applicable_tool_requests = filter_tool_lockfile_requests(
-        specified_tool_requests,
-        resolve_specified=resolve_specified,
-    )
     # We filter "user" requests because we're moving to combine user and tool lockfiles
     (
         tool_request_errors,
@@ -639,11 +541,10 @@ async def generate_lockfiles_goal(
     # Currently, since resolves specify a single filename for output, we pick a reasonable
     # environment to execute the request in. Currently we warn if multiple environments are
     # specified.
-    all_requests: Iterator[GenerateLockfile] = itertools.chain(
-        applicable_user_requests, applicable_tool_requests
-    )
     if generate_lockfiles_subsystem.request_diffs:
-        all_requests = (replace(req, diff=True) for req in all_requests)
+        all_requests = tuple(replace(req, diff=True) for req in applicable_user_requests)
+    else:
+        all_requests = applicable_user_requests
 
     results = await MultiGet(
         Get(