Skip to content

Commit

Permalink
Fix running python_sources with pex --executable (#21047)
Browse files Browse the repository at this point in the history
This fixes the feature added in #20497 as it broke using `pants run` on
a python_source if the file name has `-` or other invalid characters.
Bug reported here:

https://pantsbuild.slack.com/archives/C046T6T9U/p1717624913138789?thread_ts=1717624913.138789&cid=C046T6T9U

The other integration test for the pex --executable feature only tested
running pex_binary, not python_source. So, I missed applying some of the
path logic to both cases.
  • Loading branch information
cognifloyd committed Jun 18, 2024
1 parent dc987b9 commit 2982711
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 16 deletions.
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/goals/run_python_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def _executable_main(self) -> Optional[Executable]:
if not all(part.isidentifier() for part in source_name.split(".")):
# If the python source is not importable (python modules can't be named with '-'),
# then it must be an executable script.
executable = Executable(self.source.value)
executable = Executable.create(self.address, self.source.value)
else:
# The module is importable, so entry_point will do the heavy lifting instead.
executable = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,27 +112,37 @@ def run_run_request(


@pytest.mark.parametrize(
"global_default_value, field_value, run_uses_sandbox",
"global_default_value, field_value, run_uses_sandbox, importable_script_name",
[
# Nothing set -> True
(None, None, True),
(None, None, True, True),
(None, None, True, False),
# Field set -> use field value
(None, True, True),
(None, False, False),
(None, True, True, True),
(None, True, True, False),
(None, False, False, True),
(None, False, False, False),
# Global default set -> use default
(True, None, True),
(False, None, False),
(True, None, True, True),
(True, None, True, False),
(False, None, False, True),
(False, None, False, False),
# Both set -> use field
(True, True, True),
(True, False, False),
(False, True, True),
(False, False, False),
(True, True, True, True),
(True, True, True, False),
(True, False, False, True),
(True, False, False, False),
(False, True, True, True),
(False, True, True, False),
(False, False, False, True),
(False, False, False, False),
],
)
def test_run_sample_script(
global_default_value: bool | None,
field_value: bool | None,
run_uses_sandbox: bool,
importable_script_name: bool,
rule_runner: PythonRuleRunner,
) -> None:
"""Test that we properly run a `python_source` target.
Expand All @@ -141,9 +151,11 @@ def test_run_sample_script(
- We can handle source roots.
- We run in-repo when requested, and handle codegen correctly.
- We propagate the error code.
- We can handle scripts without importable file name.
"""
script_name = "app.py" if importable_script_name else "my-executable.py"
sources = {
"src_root1/project/app.py": dedent(
f"src_root1/project/{script_name}": dedent(
"""\
import sys
from utils.strutil import my_file
Expand Down Expand Up @@ -200,7 +212,7 @@ def my_file():
),
]
rule_runner.set_options(args, env_inherit={"PATH", "PYENV_ROOT", "HOME"})
target = rule_runner.get_target(Address("src_root1/project", relative_file_path="app.py"))
target = rule_runner.get_target(Address("src_root1/project", relative_file_path=script_name))
exit_code, stdout, stderr = run_run_request(rule_runner, target)

assert "Hola, mundo.\n" in stderr
Expand Down
10 changes: 7 additions & 3 deletions src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,12 @@ def spec(self) -> str:
class Executable(MainSpecification):
executable: str

@classmethod
def create(cls, address: Address, filename: str) -> Executable:
# spec_path is relative to the workspace. The rule is responsible for
# stripping the source root as needed.
return cls(os.path.join(address.spec_path, filename).lstrip(os.path.sep))

def iter_pex_args(self) -> Iterator[str]:
yield "--executable"
# We do NOT yield self.executable or self.spec
Expand Down Expand Up @@ -409,9 +415,7 @@ def compute_value(cls, raw_value: Optional[str], address: Address) -> Optional[E
return None
if not isinstance(value, str):
raise InvalidFieldTypeException(address, cls.alias, value, expected_type="a string")
# spec_path is relative to the workspace. The rule is responsible for
# stripping the source root as needed.
return Executable(os.path.join(address.spec_path, value).lstrip(os.path.sep))
return Executable.create(address, value)


class PexArgsField(StringSequenceField):
Expand Down
13 changes: 13 additions & 0 deletions src/python/pants/backend/python/util_rules/pex_from_targets.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

from pants.backend.python.subsystems.setup import PythonSetup
from pants.backend.python.target_types import (
Executable,
MainSpecification,
PexLayout,
PythonRequirementsField,
Expand Down Expand Up @@ -49,6 +50,7 @@
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.internals.graph import Owners, OwnersRequest
from pants.engine.rules import Get, MultiGet, collect_rules, rule
from pants.engine.target import (
Target,
Expand Down Expand Up @@ -554,6 +556,17 @@ async def create_pex_from_targets(
await _warn_about_any_files_targets(
request.addresses, transitive_targets, union_membership
)
elif isinstance(request.main, Executable):
# The source for an --executable main must be embedded in the pex even if request.include_source_files is False.
# If include_source_files is True, the executable source should be included in the (transitive) dependencies.
owners = await Get(
Owners,
OwnersRequest(
(request.main.spec,), owners_not_found_behavior=GlobMatchErrorBehavior.error
),
)
owning_targets = await Get(Targets, Addresses(owners))
sources = await Get(PythonSourceFiles, PythonSourceFilesRequest(owning_targets))
else:
sources = PythonSourceFiles.empty()

Expand Down

0 comments on commit 2982711

Please sign in to comment.