From 7970d308719df22b2cb483e950fc64b0315a4242 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Tue, 18 Jun 2024 13:03:55 -0500 Subject: [PATCH] Fix running python_sources with pex --executable (#21047) 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. --- docs/notes/2.23.x.md | 2 ++ .../backend/python/goals/run_python_source.py | 2 +- .../run_python_source_integration_test.py | 36 ++++++++++++------- .../pants/backend/python/target_types.py | 10 ++++-- .../python/util_rules/pex_from_targets.py | 13 +++++++ 5 files changed, 47 insertions(+), 16 deletions(-) diff --git a/docs/notes/2.23.x.md b/docs/notes/2.23.x.md index 16be4a09a17..a34e2fbd33f 100644 --- a/docs/notes/2.23.x.md +++ b/docs/notes/2.23.x.md @@ -74,6 +74,8 @@ The default version of the `ruff` tool has been updated from 0.4.4 to 0.4.9. The default version of the pex tool has been updated from 2.3.1 to 2.3.3. +Fix running python source files that have dashes in them (bug introduced in 2.20). For example: `pants run path/to/some-executable.py` + #### Terraform The `tfsec` linter now works on all supported platforms without extra config. diff --git a/src/python/pants/backend/python/goals/run_python_source.py b/src/python/pants/backend/python/goals/run_python_source.py index c0ba1f4a33b..2820cdd70e3 100644 --- a/src/python/pants/backend/python/goals/run_python_source.py +++ b/src/python/pants/backend/python/goals/run_python_source.py @@ -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 diff --git a/src/python/pants/backend/python/goals/run_python_source_integration_test.py b/src/python/pants/backend/python/goals/run_python_source_integration_test.py index f19330be6ad..d10b76cbe97 100644 --- a/src/python/pants/backend/python/goals/run_python_source_integration_test.py +++ b/src/python/pants/backend/python/goals/run_python_source_integration_test.py @@ -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. @@ -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 @@ -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 diff --git a/src/python/pants/backend/python/target_types.py b/src/python/pants/backend/python/target_types.py index 4b7f5f86fbe..5c3ffe3c954 100644 --- a/src/python/pants/backend/python/target_types.py +++ b/src/python/pants/backend/python/target_types.py @@ -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 @@ -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): 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 915525c0b90..3eb040a230b 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 @@ -12,6 +12,7 @@ from pants.backend.python.subsystems.setup import PythonSetup from pants.backend.python.target_types import ( + Executable, MainSpecification, PexLayout, PythonRequirementsField, @@ -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, @@ -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()