From add787fc9b2450f38c311d5eb3e3683a24d99264 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 25 May 2023 14:46:36 +1000 Subject: [PATCH] Warn about `files()` in PEXes in one place (#19027) There's several places that check whether a PEX transitively depends on any `files()` targets, because those are (currently) not included in the PEX. This moves the warning into the "pex from targets" builder, to not have to reimplement it everywhere. This is follow up to #19022, and restores the warning for `layout="zip"`, since I didn't implement it there. --- .../backend/awslambda/python/rules_test.py | 3 +- .../python/rules_test.py | 2 +- .../python/goals/package_pex_binary.py | 48 ++-------------- .../package_pex_binary_integration_test.py | 2 +- .../pants/backend/python/util_rules/faas.py | 25 ++------- .../python/util_rules/pex_from_targets.py | 55 ++++++++++++++++++- .../util_rules/pex_from_targets_test.py | 39 +++++++++++++ 7 files changed, 104 insertions(+), 70 deletions(-) diff --git a/src/python/pants/backend/awslambda/python/rules_test.py b/src/python/pants/backend/awslambda/python/rules_test.py index 26312caffb9..9f8afe48f9e 100644 --- a/src/python/pants/backend/awslambda/python/rules_test.py +++ b/src/python/pants/backend/awslambda/python/rules_test.py @@ -252,7 +252,8 @@ def handler(event, context): assert caplog.records assert "src.py.project/lambda.zip" == zip_file_relpath assert ( - "The `python_awslambda` target src/py/project:lambda transitively depends on" in caplog.text + "The target src/py/project:lambda (`python_awslambda`) transitively depends on" + in caplog.text ) assert "assets/f.txt:files" in caplog.text assert "assets:relocated" in caplog.text diff --git a/src/python/pants/backend/google_cloud_function/python/rules_test.py b/src/python/pants/backend/google_cloud_function/python/rules_test.py index e74336ce26c..7db7d99b5b0 100644 --- a/src/python/pants/backend/google_cloud_function/python/rules_test.py +++ b/src/python/pants/backend/google_cloud_function/python/rules_test.py @@ -237,7 +237,7 @@ def handler(event, context): assert caplog.records assert "src.py.project/lambda.zip" == zip_file_relpath assert ( - "The `python_google_cloud_function` target src/py/project:lambda transitively depends on" + "The target src/py/project:lambda (`python_google_cloud_function`) transitively depends on" in caplog.text ) assert "assets/f.txt:files" in caplog.text diff --git a/src/python/pants/backend/python/goals/package_pex_binary.py b/src/python/pants/backend/python/goals/package_pex_binary.py index d98788a3150..d8944bef8c0 100644 --- a/src/python/pants/backend/python/goals/package_pex_binary.py +++ b/src/python/pants/backend/python/goals/package_pex_binary.py @@ -40,19 +40,11 @@ PackageFieldSet, ) from pants.core.goals.run import RunFieldSet, RunInSandboxBehavior -from pants.core.target_types import FileSourceField from pants.core.util_rules.environments import EnvironmentField -from pants.engine.rules import Get, MultiGet, collect_rules, rule -from pants.engine.target import ( - TransitiveTargets, - TransitiveTargetsRequest, - targets_with_sources_types, -) -from pants.engine.unions import UnionMembership, UnionRule -from pants.util.docutil import doc_url +from pants.engine.rules import Get, collect_rules, rule +from pants.engine.unions import UnionRule from pants.util.frozendict import FrozenDict from pants.util.logging import LogLevel -from pants.util.strutil import softwrap logger = logging.getLogger(__name__) @@ -131,44 +123,11 @@ class PexFromTargetsRequestForBuiltPackage: async def package_pex_binary( field_set: PexBinaryFieldSet, pex_binary_defaults: PexBinaryDefaults, - union_membership: UnionMembership, ) -> PexFromTargetsRequestForBuiltPackage: - resolved_pex_entry_point_get = Get( + resolved_entry_point = await Get( ResolvedPexEntryPoint, ResolvePexEntryPointRequest(field_set.entry_point) ) - # If sources are included, then we need to check dependencies for `files` targets. - if not field_set.include_sources.value: - resolved_entry_point = await resolved_pex_entry_point_get - else: - resolved_entry_point, transitive_targets = await MultiGet( - resolved_pex_entry_point_get, - Get(TransitiveTargets, TransitiveTargetsRequest([field_set.address])), - ) - - # Warn if users depend on `files` targets, which won't be included in the PEX and is a common - # gotcha. - file_tgts = targets_with_sources_types( - [FileSourceField], transitive_targets.dependencies, union_membership - ) - if file_tgts: - files_addresses = sorted(tgt.address.spec for tgt in file_tgts) - logger.warning( - softwrap( - f""" - The `pex_binary` target {field_set.address} transitively depends on the below `files` - targets, but Pants will not include them in the PEX. Filesystem APIs like `open()` - are not able to load files within the binary itself; instead, they read from the - current working directory. - - Instead, use `resources` targets or wrap this `pex_binary` in an `archive`. - See {doc_url('resources')}. - - Files targets dependencies: {files_addresses} - """ - ) - ) - output_filename = field_set.output_path.value_or_default(file_ending="pex") complete_platforms = await Get( @@ -189,6 +148,7 @@ async def package_pex_binary( include_requirements=field_set.include_requirements.value, include_source_files=field_set.include_sources.value, include_local_dists=True, + warn_for_transitive_files_targets=True, ) return PexFromTargetsRequestForBuiltPackage(request) diff --git a/src/python/pants/backend/python/goals/package_pex_binary_integration_test.py b/src/python/pants/backend/python/goals/package_pex_binary_integration_test.py index 70f6918b86e..321ca3b177d 100644 --- a/src/python/pants/backend/python/goals/package_pex_binary_integration_test.py +++ b/src/python/pants/backend/python/goals/package_pex_binary_integration_test.py @@ -105,7 +105,7 @@ def test_warn_files_targets(rule_runner: PythonRuleRunner, caplog) -> None: assert not caplog.records result = rule_runner.request(BuiltPackage, [field_set]) assert caplog.records - assert f"The `pex_binary` target {tgt.address} transitively depends on" in caplog.text + assert f"The target {tgt.address} (`pex_binary`) transitively depends on" in caplog.text assert "assets/f.txt:files" in caplog.text assert "assets:relocated" in caplog.text assert "assets:resources" not in caplog.text diff --git a/src/python/pants/backend/python/util_rules/faas.py b/src/python/pants/backend/python/util_rules/faas.py index 21ff0ecd99b..d9eae29346b 100644 --- a/src/python/pants/backend/python/util_rules/faas.py +++ b/src/python/pants/backend/python/util_rules/faas.py @@ -40,7 +40,6 @@ from pants.backend.python.util_rules.pex_venv import PexVenv, PexVenvLayout, PexVenvRequest from pants.backend.python.util_rules.pex_venv import rules as pex_venv_rules from pants.core.goals.package import BuiltPackage, BuiltPackageArtifact, OutputPathField -from pants.core.target_types import FileSourceField from pants.engine.addresses import Address, UnparsedAddressInputs from pants.engine.fs import ( CreateDigest, @@ -66,12 +65,11 @@ StringField, TransitiveTargets, TransitiveTargetsRequest, - targets_with_sources_types, ) -from pants.engine.unions import UnionMembership, UnionRule +from pants.engine.unions import UnionRule from pants.source.filespec import Filespec from pants.source.source_root import SourceRoot, SourceRootRequest -from pants.util.docutil import bin_name, doc_url +from pants.util.docutil import bin_name from pants.util.strutil import help_text logger = logging.getLogger(__name__) @@ -312,7 +310,6 @@ async def build_lambdex( request: BuildLambdexRequest, lambdex: Lambdex, platform: Platform, - union_membership: UnionMembership, ) -> BuiltPackage: if platform.is_macos: logger.warning( @@ -354,6 +351,7 @@ async def build_lambdex( complete_platforms=complete_platforms, additional_args=additional_pex_args, additional_lockfile_args=additional_pex_args, + warn_for_transitive_files_targets=True, ) lambdex_request = lambdex.to_pex_request() @@ -364,22 +362,6 @@ async def build_lambdex( Get(TransitiveTargets, TransitiveTargetsRequest([request.address])), ) - # Warn if users depend on `files` targets, which won't be included in the PEX and is a common - # gotcha. - file_tgts = targets_with_sources_types( - [FileSourceField], transitive_targets.dependencies, union_membership - ) - if file_tgts: - files_addresses = sorted(tgt.address.spec for tgt in file_tgts) - logger.warning( - f"The `{request.target_name}` target {request.address} transitively depends " - "on the below `files` targets, but Pants will not include them in the built package. " - "Filesystem APIs like `open()` are not able to load files within the binary " - "itself; instead, they read from the current working directory." - f"\n\nInstead, use `resources` targets. See {doc_url('resources')}." - f"\n\nFiles targets dependencies: {files_addresses}" - ) - lambdex_args = ["build", "-e", f"{handler.module}:{handler.func}", output_filename] if request.script_handler: lambdex_args.extend(("-H", request.script_handler)) @@ -478,6 +460,7 @@ async def build_python_faas( additional_args=additional_pex_args, additional_lockfile_args=additional_pex_args, additional_sources=additional_sources, + warn_for_transitive_files_targets=True, ) pex_result = await Get(Pex, PexFromTargetsRequest, pex_request) diff --git a/src/python/pants/backend/python/util_rules/pex_from_targets.py b/src/python/pants/backend/python/util_rules/pex_from_targets.py index b9fa367653e..43bbf8761e2 100644 --- a/src/python/pants/backend/python/util_rules/pex_from_targets.py +++ b/src/python/pants/backend/python/util_rules/pex_from_targets.py @@ -46,11 +46,19 @@ ) from pants.backend.python.util_rules.python_sources import rules as python_sources_rules from pants.core.goals.generate_lockfiles import NoCompatibleResolveException +from pants.core.target_types import FileSourceField from pants.engine.addresses import Address, Addresses from pants.engine.collection import DeduplicatedCollection from pants.engine.fs import Digest, DigestContents, GlobMatchErrorBehavior, MergeDigests, PathGlobs from pants.engine.rules import Get, MultiGet, collect_rules, rule -from pants.engine.target import Target, TransitiveTargets, TransitiveTargetsRequest +from pants.engine.target import ( + Target, + Targets, + TransitiveTargets, + TransitiveTargetsRequest, + targets_with_sources_types, +) +from pants.engine.unions import UnionMembership from pants.util.docutil import doc_url from pants.util.frozendict import FrozenDict from pants.util.logging import LogLevel @@ -78,6 +86,7 @@ class PexFromTargetsRequest: additional_sources: Digest | None additional_inputs: Digest | None hardcoded_interpreter_constraints: InterpreterConstraints | None + warn_for_transitive_files_targets: bool # This field doesn't participate in comparison (and therefore hashing), as it doesn't affect # the result. description: str | None = dataclasses.field(compare=False) @@ -103,6 +112,7 @@ def __init__( additional_inputs: Digest | None = None, hardcoded_interpreter_constraints: InterpreterConstraints | None = None, description: str | None = None, + warn_for_transitive_files_targets: bool = False, ) -> None: """Request to create a Pex from the transitive closure of the given addresses. @@ -140,6 +150,8 @@ def __init__( constraints from the input. :param description: A human-readable description to render in the dynamic UI when building the Pex. + :param warn_for_transitive_files_targets: If True (and include_source_files is also true), + emit a warning if the pex depends on any `files` targets, since they won't be included. """ object.__setattr__(self, "addresses", Addresses(addresses)) object.__setattr__(self, "output_filename", output_filename) @@ -161,6 +173,9 @@ def __init__( self, "hardcoded_interpreter_constraints", hardcoded_interpreter_constraints ) object.__setattr__(self, "description", description) + object.__setattr__( + self, "warn_for_transitive_files_targets", warn_for_transitive_files_targets + ) self.__post_init__() @@ -473,9 +488,40 @@ async def _determine_requirements_for_pex_from_targets( return dataclasses.replace(requirements, from_superset=repository_pex.maybe_pex), () +async def _warn_about_any_files_targets( + addresses: Addresses, transitive_targets: TransitiveTargets, union_membership: UnionMembership +) -> None: + # Warn if users depend on `files` targets, which won't be included in the PEX and is a common + # gotcha. + file_tgts = targets_with_sources_types( + [FileSourceField], transitive_targets.dependencies, union_membership + ) + if file_tgts: + # make it easier for the user to find which targets are problematic by including the alias + targets = await Get(Targets, Addresses, addresses) + formatted_addresses = ", ".join( + f"{a} (`{tgt.alias}`)" for a, tgt in zip(addresses, targets) + ) + + files_addresses = sorted(tgt.address.spec for tgt in file_tgts) + targets_text, depend_text = ( + ("target", "depends") if len(addresses) == 1 else ("targets", "depend") + ) + logger.warning( + f"The {targets_text} {formatted_addresses} transitively {depend_text} " + "on the below `files` targets, but Pants will not include them in the built package. " + "Filesystem APIs like `open()` may be not able to load files within the binary " + "itself; instead, they read from the current working directory." + f"\n\nInstead, use `resources` targets. See {doc_url('resources')}." + f"\n\nFiles targets dependencies: {files_addresses}" + ) + + @rule(level=LogLevel.DEBUG) async def create_pex_from_targets( - request: PexFromTargetsRequest, python_setup: PythonSetup + request: PexFromTargetsRequest, + python_setup: PythonSetup, + union_membership: UnionMembership, ) -> PexRequest: requirements, additional_pexes = await _determine_requirements_for_pex_from_targets( request, python_setup @@ -495,6 +541,11 @@ async def create_pex_from_targets( TransitiveTargets, TransitiveTargetsRequest(request.addresses) ) sources = await Get(PythonSourceFiles, PythonSourceFilesRequest(transitive_targets.closure)) + + if request.warn_for_transitive_files_targets: + await _warn_about_any_files_targets( + request.addresses, transitive_targets, union_membership + ) else: sources = PythonSourceFiles.empty() diff --git a/src/python/pants/backend/python/util_rules/pex_from_targets_test.py b/src/python/pants/backend/python/util_rules/pex_from_targets_test.py index 1c2af220341..6bf0738f76b 100644 --- a/src/python/pants/backend/python/util_rules/pex_from_targets_test.py +++ b/src/python/pants/backend/python/util_rules/pex_from_targets_test.py @@ -56,6 +56,7 @@ from pants.backend.python.util_rules.pex_test_utils import get_all_data from pants.build_graph.address import Address from pants.core.goals.generate_lockfiles import NoCompatibleResolveException +from pants.core.target_types import FileTarget, ResourceTarget from pants.engine.addresses import Addresses from pants.engine.fs import Snapshot from pants.testutil.option_util import create_subsystem @@ -85,6 +86,8 @@ def rule_runner() -> PythonRuleRunner: PythonRequirementTarget, PythonSourceTarget, PythonTestTarget, + FileTarget, + ResourceTarget, ], ) @@ -892,3 +895,39 @@ def test_lockfile_requirements_selection( assert mode == ResolveMode.pex assert isinstance(result.requirements.from_superset, Resolve) assert result.requirements.from_superset.name == "myresolve" + + +def test_warn_about_files_targets(rule_runner: PythonRuleRunner, caplog) -> None: + rule_runner.write_files( + { + "app.py": "", + "file.txt": "", + "resource.txt": "", + "BUILD": dedent( + """ + file(name="file_target", source="file.txt") + resource(name="resource_target", source="resource.txt") + python_sources(name="app", dependencies=[":file_target", ":resource_target"]) + """ + ), + } + ) + + rule_runner.request( + PexRequest, + [ + PexFromTargetsRequest( + [Address("", target_name="app")], + output_filename="app.pex", + internal_only=True, + warn_for_transitive_files_targets=True, + ) + ], + ) + + assert "The target //:app (`python_source`) transitively depends on" in caplog.text + # files are not fine: + assert "//:file_target" in caplog.text + # resources are fine: + assert "resource_target" not in caplog.text + assert "resource.txt" not in caplog.text