Warn about files() in PEXes in one place (pantsbuild#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 pantsbuild#19022, and restores the warning for
`layout="zip"`, since I didn't implement it there.

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

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

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)

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

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)

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()
 

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