From b32f1d1ed6c9276fbb2f1e2636a623a449678268 Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Thu, 15 Oct 2020 13:13:44 -0700 Subject: [PATCH] Add mechanism to deprecate target types and fields (#10966) We leave off deprecated target types and fields from `./pants target-types` for now. In a followup, we may want to do something like `./pants help` where it shows the values, but in red. We only log on the original targets in BUILD files, rather than on generated subtargets. This avoids unnecessary noise, where one target owning 7 source files would have had resulted in 8 different warnings. [ci skip-rust] [ci skip-build-wheels] --- .../backend/pants_info/list_target_types.py | 14 ++--- .../pants/backend/python/target_types.py | 44 ++++------------ src/python/pants/engine/target.py | 51 +++++++++++++++---- 3 files changed, 58 insertions(+), 51 deletions(-) diff --git a/src/python/pants/backend/pants_info/list_target_types.py b/src/python/pants/backend/pants_info/list_target_types.py index b0b3652b250..143bcf98833 100644 --- a/src/python/pants/backend/pants_info/list_target_types.py +++ b/src/python/pants/backend/pants_info/list_target_types.py @@ -198,6 +198,7 @@ def create( fields=[ FieldInfo.create(field) for field in target_type.class_field_types(union_membership=union_membership) + if not field.alias.startswith("_") and field.deprecated_removal_version is None ], ) @@ -208,11 +209,7 @@ def format_for_cli(self, console: Console) -> str: output.extend( [ "Valid fields:\n", - *sorted( - f"{field.format_for_cli(console)}\n" - for field in self.fields - if not field.alias.startswith("_") - ), + *sorted(f"{field.format_for_cli(console)}\n" for field in self.fields), ] ) return "\n".join(output).rstrip() @@ -239,7 +236,7 @@ def list_target_types( target_type, union_membership=union_membership ).as_dict() for alias, target_type in registered_target_types.aliases_to_types.items() - if not alias.startswith("_") + if not alias.startswith("_") and target_type.deprecated_removal_version is None } print_stdout(json.dumps(all_target_types, sort_keys=True, indent=4)) elif target_types_subsystem.details: @@ -261,6 +258,10 @@ def list_target_types( target_infos = [ AbbreviatedTargetInfo.create(target_type) for target_type in registered_target_types.types + if ( + not target_type.alias.startswith("_") + and target_type.deprecated_removal_version is None + ) ] longest_target_alias = max( len(target_type.alias) for target_type in registered_target_types.types @@ -276,7 +277,6 @@ def list_target_types( *( target_info.format_for_cli(console, longest_target_alias=longest_target_alias) for target_info in target_infos - if not target_info.alias.startswith("_") ), ] print_stdout("\n".join(lines).rstrip()) diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index e18a5bffcf1..508406c4efe 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -5,7 +5,7 @@ import logging import os.path from textwrap import dedent -from typing import Any, Dict, Iterable, Optional, Tuple, Union, cast +from typing import Iterable, Optional, Tuple, Union, cast from pkg_resources import Requirement @@ -36,7 +36,6 @@ Target, WrappedTarget, ) -from pants.engine.unions import UnionMembership from pants.option.subsystem import Subsystem from pants.python.python_requirement import PythonRequirement from pants.python.python_setup import PythonSetup @@ -281,26 +280,13 @@ class PythonBinary(PexBinary): """ alias = "python_binary" - - def __init__( # type: ignore[misc] # This is `@final`, but it's fine to override here. - self, - unhydrated_values: Dict[str, Any], - *, - address: Address, - union_membership: Optional[UnionMembership] = None, - ) -> None: - warn_or_error( - removal_version="2.1.0.dev0", - deprecated_entity_description="the `python_binary` target", - hint=( - f"Use the target type `pex_binary`, rather than `python_binary` for {address}. " - "The behavior is identical.\n\nTo fix this globally, run the below command:\n\t" - "macOS: for f in $(find . -name BUILD); do sed -i '' -Ee " - "'s#^python_binary\\(#pex_binary(#g' $f ; done\n\tLinux: for f in $(find . " - "-name BUILD); do sed -i -Ee 's#^python_binary\\(#pex_binary(#g' $f ; done\n" - ), - ) - super().__init__(unhydrated_values, address=address, union_membership=union_membership) + deprecated_removal_version = "2.1.0.dev0" + deprecated_removal_hint = ( + "Use `pex_binary` instead, which behaves identically.\n\nTo fix this globally, run the " + "below command:\n\tmacOS: for f in $(find . -name BUILD); do sed -i '' -Ee " + "'s#^python_binary\\(#pex_binary(#g' $f ; done\n\tLinux: for f in $(find . -name BUILD); " + "do sed -i -Ee 's#^python_binary\\(#pex_binary(#g' $f ; done" + ) # ----------------------------------------------------------------------------------------------- @@ -344,18 +330,8 @@ class PythonRuntimeBinaryDependencies(SpecialCasedDependencies): types like `archive` and `python_awslambda`.""" alias = "runtime_binary_dependencies" - - @classmethod - def sanitize_raw_value( - cls, raw_value: Optional[Iterable[str]], *, address: Address - ) -> Optional[Tuple[str, ...]]: - if raw_value is not None: - logger.warning( - f"Using the `runtime_binary_dependencies` field in the target {address}. This " - "field is now deprecated in favor of the more flexible " - "`runtime_package_dependencies` field, and it will be removed in 2.1.0.dev0." - ) - return super().sanitize_raw_value(raw_value, address=address) + deprecated_removal_version = "2.1.0dev0" + deprecated_removal_hint = "Use the more flexible `runtime_package_dependencies` field instead." class PythonTestsTimeout(IntField): diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index f9f4f1b202c..9f002854d57 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -5,7 +5,7 @@ import dataclasses import itertools import os.path -from abc import ABC, ABCMeta, abstractmethod +from abc import ABC, ABCMeta from dataclasses import dataclass from enum import Enum from pathlib import PurePath @@ -28,6 +28,7 @@ from typing_extensions import final +from pants.base.deprecated import warn_or_error from pants.base.specs import Spec from pants.engine.addresses import Address, UnparsedAddressInputs, assert_single_address from pants.engine.collection import Collection, DeduplicatedCollection @@ -58,17 +59,26 @@ class Field(ABC): default: ClassVar[ImmutableValue] # Subclasses may define these. required: ClassVar[bool] = False + deprecated_removal_version: ClassVar[Optional[str]] = None + deprecated_removal_hint: ClassVar[Optional[str]] = None - # This is a little weird to have an abstract __init__(). We do this to ensure that all - # subclasses have this exact type signature for their constructor. - # - # Normally, with dataclasses, each constructor parameter would instead be specified via a - # dataclass field declaration. But, we don't want to declare either `address` or `raw_value` as - # attributes because we make no assumptions whether the subclasses actually store those values - # on each instance. All that we care about is a common constructor interface. - @abstractmethod + # NB: We still expect `PrimitiveField` and `AsyncField` to define their own constructors. This + # is only implemented so that we have a common deprecation mechanism. def __init__(self, raw_value: Optional[Any], *, address: Address) -> None: - pass + if self.deprecated_removal_version and address.is_base_target and raw_value is not None: + if not self.deprecated_removal_hint: + raise ValueError( + f"You specified `deprecated_removal_version` for {self.__class__}, but not " + "the class property `deprecated_removal_hint`." + ) + warn_or_error( + removal_version=self.deprecated_removal_version, + deprecated_entity_description=f"the {repr(self.alias)} field", + hint=( + f"Using the `{self.alias}` field in the target {address}. " + f"{self.deprecated_removal_hint}" + ), + ) @frozen_after_init @@ -123,6 +133,7 @@ def __init__(self, raw_value: Optional[Any], *, address: Address) -> None: # this Field could not be passed around in the engine. # * Don't store `address` to avoid the cost in memory of storing `Address` on every single # field encountered by Pants in a run. + super().__init__(raw_value, address=address) self.value = self.compute_value(raw_value, address=address) @classmethod @@ -214,6 +225,7 @@ def rules(): @final def __init__(self, raw_value: Optional[Any], *, address: Address) -> None: + super().__init__(raw_value, address=address) self.address = address self.sanitized_raw_value = self.sanitize_raw_value(raw_value, address=address) @@ -260,6 +272,10 @@ class Target(ABC): alias: ClassVar[str] core_fields: ClassVar[Tuple[Type[Field], ...]] + # Subclasses may define these. + deprecated_removal_version: ClassVar[Optional[str]] = None + deprecated_removal_hint: ClassVar[Optional[str]] = None + # These get calculated in the constructor address: Address plugin_fields: Tuple[Type[Field], ...] @@ -276,6 +292,21 @@ def __init__( # rarely directly instantiate Targets and should instead use the engine to request them. union_membership: Optional[UnionMembership] = None, ) -> None: + if self.deprecated_removal_version and address.is_base_target: + if not self.deprecated_removal_hint: + raise ValueError( + f"You specified `deprecated_removal_version` for {self.__class__}, but not " + "the class property `deprecated_removal_hint`." + ) + warn_or_error( + removal_version=self.deprecated_removal_version, + deprecated_entity_description=f"the {repr(self.alias)} target type", + hint=( + f"Using the `{self.alias}` target type for {address}. " + f"{self.deprecated_removal_hint}" + ), + ) + self.address = address self.plugin_fields = self._find_plugin_fields(union_membership or UnionMembership({}))