Skip to content

Commit

Permalink
Warn about files() in PEXes in one place
Browse files Browse the repository at this point in the history
  • Loading branch information
huonw committed May 17, 2023
1 parent 8087903 commit 337bcc5
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 71 deletions.
4 changes: 1 addition & 3 deletions src/python/pants/backend/awslambda/python/rules_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,7 @@ 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
)
assert "The target src/py/project:lambda 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,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"
in caplog.text
)
assert "The target src/py/project:lambda 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
Expand Down
45 changes: 4 additions & 41 deletions src/python/pants/backend/python/goals/package_pex_binary.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -131,43 +123,13 @@ class PexFromTargetsRequestForBuiltPackage:
async def package_pex_binary(
field_set: PexBinaryFieldSet,
pex_binary_defaults: PexBinaryDefaults,
union_membership: UnionMembership,
) -> PexFromTargetsRequestForBuiltPackage:
resolved_pex_entry_point_get = 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}
"""
)
)
resolved_entry_point = await resolved_pex_entry_point_get

output_filename = field_set.output_path.value_or_default(file_ending="pex")

Expand All @@ -189,6 +151,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)
Expand Down
25 changes: 4 additions & 21 deletions src/python/pants/backend/python/util_rules/faas.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,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,
Expand All @@ -69,12 +68,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__)
Expand Down Expand Up @@ -343,7 +341,6 @@ async def build_lambdex(
request: BuildLambdexRequest,
lambdex: Lambdex,
platform: Platform,
union_membership: UnionMembership,
) -> BuiltPackage:
if platform.is_macos:
logger.warning(
Expand Down Expand Up @@ -384,6 +381,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()

Expand All @@ -394,22 +392,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))
Expand Down Expand Up @@ -509,6 +491,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)
Expand Down
45 changes: 43 additions & 2 deletions src/python/pants/backend/python/util_rules/pex_from_targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,18 @@
)
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,
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
Expand Down Expand Up @@ -78,6 +85,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)
Expand All @@ -103,6 +111,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.
Expand Down Expand Up @@ -140,6 +149,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)
Expand All @@ -161,6 +172,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__()

Expand Down Expand Up @@ -469,9 +483,33 @@ async def _determine_requirements_for_pex_from_targets(
return dataclasses.replace(requirements, from_superset=repository_pex.maybe_pex), ()


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:
files_addresses = sorted(tgt.address.spec for tgt in file_tgts)
formatted_addresses = ", ".join(str(a) for a in addresses)
targets, depend = ("target", "depends") if len(addresses) == 1 else ("targets", "depend")
logger.warning(
f"The {targets} {formatted_addresses} transitively {depend} "
"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
Expand All @@ -491,6 +529,9 @@ 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:
_warn_about_any_files_targets(request.addresses, transitive_targets, union_membership)
else:
sources = PythonSourceFiles.empty()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,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
Expand Down Expand Up @@ -83,6 +84,8 @@ def rule_runner() -> PythonRuleRunner:
PythonRequirementTarget,
PythonSourceTarget,
PythonTestTarget,
FileTarget,
ResourceTarget,
],
)

Expand Down Expand Up @@ -885,3 +888,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 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

0 comments on commit 337bcc5

Please sign in to comment.