From 3b6fbd7438938fb1856f6d3dc338ce60ca55628d Mon Sep 17 00:00:00 2001 From: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> Date: Mon, 5 Oct 2020 20:09:02 -0700 Subject: [PATCH] Add `output_path` field to `python_binary`, `python_awslambda`, and `archive` (#10899) ### Problem When building an `archive`, we believe users will want to be able to control where certain files/packages show up. This is why we added `relocated_files` in https://github.com/pantsbuild/pants/pull/10880. However, `relocated_files` would not work with a package. If you tried using `relocated_files` with a package, then it would not build the actual package, as it's not `FilesSources`. Further, even if you're not using `archive`, users may want to control the output path when running `./pants package` or using `runtime_package_dependencies` in `python_tests`. For example, they may want to hardcode a certain value so that changing the target name or directory path would not change the final package name. In v1, we had `basename` for this. But `basename` is not adequate because this solely changes the final file name, but not the full path, like `src.python.pants/pants.pex`. ### Solution For package target types, allow users to override the default path. We warn that this can result in ambiguous paths if the user is not careful, whereas our default is always unambiguous. While this could be surprising, the user must go out of their way to opt-in, and we will warn both in `./pants target-types` and the online docs. [ci skip-build-wheels] [ci skip-rust] --- .../pants/backend/awslambda/python/rules.py | 37 ++++++--------- .../python/goals/package_python_binary.py | 23 +++------- .../pants/backend/python/target_types.py | 2 + src/python/pants/core/goals/package.py | 46 ++++++++++++++++++- src/python/pants/core/target_types.py | 26 +++++++---- src/python/pants/core/target_types_test.py | 2 + src/python/pants/engine/target.py | 16 ++----- 7 files changed, 91 insertions(+), 61 deletions(-) diff --git a/src/python/pants/backend/awslambda/python/rules.py b/src/python/pants/backend/awslambda/python/rules.py index e20be07c8d1..85baddf532d 100644 --- a/src/python/pants/backend/awslambda/python/rules.py +++ b/src/python/pants/backend/awslambda/python/rules.py @@ -1,8 +1,6 @@ # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import logging -import os from dataclasses import dataclass from pants.backend.awslambda.common.rules import AWSLambdaFieldSet, CreatedAWSLambda @@ -25,7 +23,7 @@ PexFromTargetsRequest, TwoStepPexFromTargetsRequest, ) -from pants.core.goals.package import BuiltPackage, PackageFieldSet +from pants.core.goals.package import BuiltPackage, OutputPathField, PackageFieldSet from pants.engine.fs import Digest, MergeDigests from pants.engine.process import ProcessResult from pants.engine.rules import Get, collect_rules, rule @@ -33,8 +31,6 @@ from pants.option.global_options import GlobalOptions from pants.util.logging import LogLevel -logger = logging.getLogger(__name__) - @dataclass(frozen=True) class PythonAwsLambdaFieldSet(PackageFieldSet, AWSLambdaFieldSet): @@ -42,6 +38,7 @@ class PythonAwsLambdaFieldSet(PackageFieldSet, AWSLambdaFieldSet): handler: PythonAwsLambdaHandler runtime: PythonAwsLambdaRuntime + output_path: OutputPathField @dataclass(frozen=True) @@ -53,21 +50,13 @@ class LambdexSetup: async def create_python_awslambda( field_set: PythonAwsLambdaFieldSet, lambdex_setup: LambdexSetup, global_options: GlobalOptions ) -> CreatedAWSLambda: - # Lambdas typically use the .zip suffix, so we use that instead of .pex. - disambiguated_pex_filename = os.path.join( - field_set.address.spec_path.replace(os.sep, "."), f"{field_set.address.target_name}.zip" + output_filename = field_set.output_path.value_or_default( + field_set.address, + # Lambdas typically use the .zip suffix, so we use that instead of .pex. + file_ending="zip", + use_legacy_format=global_options.options.pants_distdir_legacy_paths, ) - if global_options.options.pants_distdir_legacy_paths: - pex_filename = f"{field_set.address.target_name}.zip" - logger.warning( - f"Writing to the legacy subpath: {pex_filename}, which may not be unique. An " - f"upcoming version of Pants will switch to writing to the fully-qualified subpath: " - f"{disambiguated_pex_filename}. You can effect that switch now (and silence this " - f"warning) by setting `pants_distdir_legacy_paths = false` in the [GLOBAL] section of " - f"pants.toml." - ) - else: - pex_filename = disambiguated_pex_filename + # We hardcode the platform value to the appropriate one for each AWS Lambda runtime. # (Running the "hello world" lambda in the example code will report the platform, and can be # used to verify correctness of these platform strings.) @@ -83,7 +72,7 @@ async def create_python_awslambda( addresses=[field_set.address], internal_only=False, entry_point=None, - output_filename=pex_filename, + output_filename=output_filename, platforms=PexPlatforms([platform]), additional_args=[ # Ensure we can resolve manylinux wheels in addition to any AMI-specific wheels. @@ -105,15 +94,15 @@ async def create_python_awslambda( ProcessResult, PexProcess( lambdex_setup.requirements_pex, - argv=("build", "-e", field_set.handler.value, pex_filename), + argv=("build", "-e", field_set.handler.value, output_filename), input_digest=input_digest, - output_files=(pex_filename,), - description=f"Setting up handler in {pex_filename}", + output_files=(output_filename,), + description=f"Setting up handler in {output_filename}", ), ) return CreatedAWSLambda( digest=result.output_digest, - zip_file_relpath=pex_filename, + zip_file_relpath=output_filename, runtime=field_set.runtime.value, # The AWS-facing handler function is always lambdex_handler.handler, which is the wrapper # injected by lambdex that manages invocation of the actual handler. diff --git a/src/python/pants/backend/python/goals/package_python_binary.py b/src/python/pants/backend/python/goals/package_python_binary.py index f435f39252f..7a8fbfa8c13 100644 --- a/src/python/pants/backend/python/goals/package_python_binary.py +++ b/src/python/pants/backend/python/goals/package_python_binary.py @@ -1,7 +1,6 @@ # Copyright 2019 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import logging import os from dataclasses import dataclass from typing import Tuple @@ -24,7 +23,7 @@ TwoStepPexFromTargetsRequest, ) from pants.core.goals.binary import BinaryFieldSet, CreatedBinary -from pants.core.goals.package import BuiltPackage, PackageFieldSet +from pants.core.goals.package import BuiltPackage, OutputPathField, PackageFieldSet from pants.core.goals.run import RunFieldSet from pants.engine.fs import PathGlobs, Paths from pants.engine.rules import Get, collect_rules, rule @@ -34,8 +33,6 @@ from pants.source.source_root import SourceRoot, SourceRootRequest from pants.util.logging import LogLevel -logger = logging.getLogger(__name__) - @dataclass(frozen=True) class PythonBinaryFieldSet(PackageFieldSet, BinaryFieldSet, RunFieldSet): @@ -44,6 +41,7 @@ class PythonBinaryFieldSet(PackageFieldSet, BinaryFieldSet, RunFieldSet): sources: PythonBinarySources entry_point: PythonEntryPoint + output_path: OutputPathField always_write_cache: PexAlwaysWriteCache emit_warnings: PexEmitWarnings ignore_errors: PexIgnoreErrors @@ -98,20 +96,11 @@ async def package_python_binary( os.path.relpath(entry_point_path, source_root.path) ) - disambiguated_output_filename = os.path.join( - field_set.address.spec_path.replace(os.sep, "."), f"{field_set.address.target_name}.pex" + output_filename = field_set.output_path.value_or_default( + field_set.address, + file_ending="pex", + use_legacy_format=global_options.options.pants_distdir_legacy_paths, ) - if global_options.options.pants_distdir_legacy_paths: - output_filename = f"{field_set.address.target_name}.pex" - logger.warning( - f"Writing to the legacy subpath: {output_filename}, which may not be unique. An " - f"upcoming version of Pants will switch to writing to the fully-qualified subpath: " - f"{disambiguated_output_filename}. You can effect that switch now (and silence this " - f"warning) by setting `pants_distdir_legacy_paths = false` in the [GLOBAL] section of " - f"pants.toml." - ) - else: - output_filename = disambiguated_output_filename two_step_pex = await Get( TwoStepPex, TwoStepPexFromTargetsRequest( diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index 6f2e02160a3..7fdebb34b08 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -12,6 +12,7 @@ from pants.backend.python.macros.python_artifact import PythonArtifact from pants.backend.python.subsystems.pytest import PyTest from pants.base.deprecated import warn_or_error +from pants.core.goals.package import OutputPathField from pants.engine.addresses import Address, AddressInput from pants.engine.rules import Get, MultiGet, collect_rules, rule from pants.engine.target import ( @@ -233,6 +234,7 @@ class PythonBinary(Target): alias = "python_binary" core_fields = ( *COMMON_PYTHON_FIELDS, + OutputPathField, PythonBinarySources, PythonBinaryDependencies, PythonEntryPoint, diff --git a/src/python/pants/core/goals/package.py b/src/python/pants/core/goals/package.py index 48ddbadfeb8..3198f657b1c 100644 --- a/src/python/pants/core/goals/package.py +++ b/src/python/pants/core/goals/package.py @@ -2,15 +2,22 @@ # Licensed under the Apache License, Version 2.0 (see LICENSE). import logging +import os from abc import ABCMeta from dataclasses import dataclass from typing import Optional from pants.core.util_rules.distdir import DistDir +from pants.engine.addresses import Address from pants.engine.fs import Digest, MergeDigests, Snapshot, Workspace from pants.engine.goal import Goal, GoalSubsystem from pants.engine.rules import Get, MultiGet, collect_rules, goal_rule -from pants.engine.target import FieldSet, TargetRootsToFieldSets, TargetRootsToFieldSetsRequest +from pants.engine.target import ( + FieldSet, + StringField, + TargetRootsToFieldSets, + TargetRootsToFieldSetsRequest, +) from pants.engine.unions import union logger = logging.getLogger(__name__) @@ -28,6 +35,43 @@ class BuiltPackage: extra_log_info: Optional[str] = None +class OutputPathField(StringField): + """Where the built asset should be located. + + If undefined, this will use the path to the the BUILD, followed by the target name. For + example, `src/python/project:app` would be `src.python.project/app.ext`. + + When running `./pants package`, this path will be prefixed by `--distdir` (e.g. `dist/`). + + Warning: setting this value risks naming collisions with other package targets you may have. + """ + + alias = "output_path" + + def value_or_default( + self, address: Address, *, file_ending: str, use_legacy_format: bool + ) -> str: + assert not file_ending.startswith("."), "`file_ending` should not start with `.`" + if self.value is not None: + return self.value + disambiguated = os.path.join( + address.spec_path.replace(os.sep, "."), f"{address.target_name}.{file_ending}" + ) + if use_legacy_format: + ambiguous_name = f"{address.target_name}.{file_ending}" + logger.warning( + f"Writing to the legacy subpath {repr(ambiguous_name)} for the target {address}. " + f"This location may not be unique. An upcoming version of Pants will switch to " + f"writing to the fully-qualified subpath: {disambiguated}.\n\nYou can make that " + "switch now (and silence this warning) by setting " + "`pants_distdir_legacy_paths = false` in the [GLOBAL] section " + "of pants.toml.\n\nAlternatively, you can set the field `output_path` on the " + f"target {address} to a hardcoded value." + ) + return ambiguous_name + return disambiguated + + class PackageSubsystem(GoalSubsystem): """Create a distributable package.""" diff --git a/src/python/pants/core/target_types.py b/src/python/pants/core/target_types.py index def2c8a1cc9..18f46ee41b4 100644 --- a/src/python/pants/core/target_types.py +++ b/src/python/pants/core/target_types.py @@ -1,11 +1,10 @@ # Copyright 2020 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import os from dataclasses import dataclass from typing import Tuple -from pants.core.goals.package import BuiltPackage, PackageFieldSet +from pants.core.goals.package import BuiltPackage, OutputPathField, PackageFieldSet from pants.core.util_rules.archive import ArchiveFormat, CreateArchive from pants.engine.addresses import AddressInput from pants.engine.fs import AddPrefix, Digest, MergeDigests, RemovePrefix, Snapshot @@ -26,6 +25,7 @@ WrappedTarget, ) from pants.engine.unions import UnionRule +from pants.option.global_options import GlobalOptions from pants.util.logging import LogLevel # ----------------------------------------------------------------------------------------------- @@ -267,7 +267,13 @@ class ArchiveTarget(Target): package`.""" alias = "archive" - core_fields = (*COMMON_TARGET_FIELDS, ArchivePackages, ArchiveFiles, ArchiveFormatField) + core_fields = ( + *COMMON_TARGET_FIELDS, + OutputPathField, + ArchivePackages, + ArchiveFiles, + ArchiveFormatField, + ) @dataclass(frozen=True) @@ -277,10 +283,13 @@ class ArchiveFieldSet(PackageFieldSet): packages: ArchivePackages files: ArchiveFiles format_field: ArchiveFormatField + output_path: OutputPathField @rule(level=LogLevel.DEBUG) -async def package_archive_target(field_set: ArchiveFieldSet) -> BuiltPackage: +async def package_archive_target( + field_set: ArchiveFieldSet, global_options: GlobalOptions +) -> BuiltPackage: package_targets = await MultiGet( Get( WrappedTarget, @@ -330,10 +339,11 @@ async def package_archive_target(field_set: ArchiveFieldSet) -> BuiltPackage: ) ), ) - file_ending = field_set.format_field.value - output_filename = os.path.join( - field_set.address.spec_path.replace(os.sep, "."), - f"{field_set.address.target_name}.{file_ending}", + + output_filename = field_set.output_path.value_or_default( + field_set.address, + file_ending=field_set.format_field.value, + use_legacy_format=global_options.options.pants_distdir_legacy_paths, ) archive = await Get( Digest, diff --git a/src/python/pants/core/target_types_test.py b/src/python/pants/core/target_types_test.py index 0bad7a0646b..e947df1c27b 100644 --- a/src/python/pants/core/target_types_test.py +++ b/src/python/pants/core/target_types_test.py @@ -197,6 +197,7 @@ def test_archive() -> None: packages=[":archive1"], files=["resources:relocated_files"], format="tar", + output_path="output/archive2.tar", ) """ ), @@ -227,6 +228,7 @@ def assert_archive1_is_valid(zip_bytes: bytes) -> None: assert_archive1_is_valid(archive1.content) archive2 = get_archive("archive2") + assert archive2.path == "output/archive2.tar" io = BytesIO() io.write(archive2.content) io.seek(0) diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index 34c9d4c1e6c..5149c996e7c 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -1012,7 +1012,7 @@ def compute_value(cls, raw_value: Optional[bool], *, address: Address) -> Option return value_or_default -class IntField(ScalarField, metaclass=ABCMeta): +class IntField(ScalarField[int], metaclass=ABCMeta): expected_type = int expected_type_description = "an integer" @@ -1021,7 +1021,7 @@ def compute_value(cls, raw_value: Optional[int], *, address: Address) -> Optiona return super().compute_value(raw_value, address=address) -class FloatField(ScalarField, metaclass=ABCMeta): +class FloatField(ScalarField[float], metaclass=ABCMeta): expected_type = float expected_type_description = "a float" @@ -1030,7 +1030,7 @@ def compute_value(cls, raw_value: Optional[float], *, address: Address) -> Optio return super().compute_value(raw_value, address=address) -class StringField(ScalarField, metaclass=ABCMeta): +class StringField(ScalarField[str], metaclass=ABCMeta): """A field whose value is a string. If you expect the string to only be one of several values, set the class property @@ -1100,10 +1100,7 @@ def compute_value( return tuple(value_or_default) -class StringSequenceField(SequenceField, metaclass=ABCMeta): - value: Optional[Tuple[str, ...]] - default: ClassVar[Optional[Tuple[str, ...]]] = None - +class StringSequenceField(SequenceField[str], metaclass=ABCMeta): expected_element_type = str expected_type_description = "an iterable of strings (e.g. a list of strings)" @@ -1114,7 +1111,7 @@ def compute_value( return super().compute_value(raw_value, address=address) -class StringOrStringSequenceField(SequenceField, metaclass=ABCMeta): +class StringOrStringSequenceField(SequenceField[str], metaclass=ABCMeta): """The raw_value may either be a string or be an iterable of strings. This is syntactic sugar that we use for certain fields to make BUILD files simpler when the user @@ -1123,9 +1120,6 @@ class StringOrStringSequenceField(SequenceField, metaclass=ABCMeta): Generally, this should not be used by any new Fields. This mechanism is a misfeature. """ - value: Optional[Tuple[str, ...]] - default: ClassVar[Optional[Tuple[str, ...]]] = None - expected_element_type = str expected_type_description = ( "either a single string or an iterable of strings (e.g. a list of strings)"