Add support for the pex `--executable` argument #20497

cognifloyd · 2024-02-06T21:31:02Z

This PR adds support for pex --executable <path to script> added in pex 2.1.93:
pex-tool/pex#1807

With this change, users should be able to do pants run bin/my-python-script.py.

cognifloyd · 2024-02-08T00:44:00Z

src/python/pants/backend/python/goals/run_pex_binary_integration_test.py

@@ -213,6 +213,52 @@ def test_local_dist() -> None:
        assert result.stdout == "LOCAL DIST\n"


+def test_local_dist_with_executable_main() -> None:


This is a copy of test_local_dist, but using an executable main instead of an entry point.

benjyw

LG! Just a few nits...

benjyw · 2024-02-08T14:55:30Z

src/python/pants/backend/python/goals/run_helper.py

@@ -67,7 +69,7 @@ async def _create_python_source_run_request(
                include_source_files=False,
                include_local_dists=True,
                # `PEX_EXTRA_SYS_PATH` should contain this entry_point's module.
-                main=console_script or entry_point.val,
+                main=executable or console_script or entry_point.val,


Why the different precedence here than above? Only one should be set, so it may not matter in practice, but it's good to be consistent.

This function always gets an entry_point, whereas the others can be None. So we check the Nones first.

Oddly enough, nothing in pants actually passes a ConsoleScript to this function. This PR adds a heuristic that might pass an Executable.

So, would you prefer:

Suggested change

main=executable or console_script or entry_point.val,

main=console_script or executable or entry_point.val,

I guess it's fine the way it is, it just scans oddly.

src/python/pants/backend/python/util_rules/pex.py

src/python/pants/backend/python/subsystems/python_tool_base.py

src/python/pants/backend/python/target_types.py

benjyw · 2024-02-08T21:30:12Z

src/python/pants/backend/python/goals/package_pex_binary.py

@@ -141,7 +143,7 @@ async def package_pex_binary(
    request = PexFromTargetsRequest(
        addresses=[field_set.address],
        internal_only=False,
-        main=resolved_entry_point.val or field_set.script.value,
+        main=resolved_entry_point.val or field_set.script.value or field_set.executable.value,


Re my question on precedence, does this always get an entry point? Can its .val be None? How is this case different than the one below?

In this case, yes, as this is the general purpose case and can be anything.

In the other file, it is a utility function only used for the run goal, so it always has an entry point.

Co-authored-by: Benjy Weinberger <benjyw@gmail.com>

cognifloyd · 2024-06-11T03:16:20Z

src/python/pants/backend/python/goals/run_python_source.py

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


Oops. self.source.value is relative to the BUILD file, but Executable needs to be relative to the workspace root, similar to the logic in PexExecutableField.

executable = Executable(os.path.join(self.address.spec_path, self.source.value).lstrip(os.path.sep))

The integration tests are only testing running a pex_binary, not python_source.
So, we need to add another parameter to this test to use app.py or my-app.py so it uses the Executable main:
https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/python/goals/run_python_source_integration_test.py#L114-L137

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.

…) (#21084) 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.

…) (#21085) 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.

…) (#21086) 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.

cognifloyd added 6 commits February 5, 2024 23:31

stub pex --executable support

088d7ff

add PexExecutableField validation test

2afda25

reword help_text to include executable field

7ebca2d

adjust comment on PythonToolBase

fa0fb1d

add test for pex_binary w/ executable param

8dd1e51

refactor

1682cff

cognifloyd added category:new feature backend: Python Python backend-related issues labels Feb 6, 2024

cognifloyd self-assigned this Feb 6, 2024

cognifloyd added 3 commits February 7, 2024 17:29

keep trying

02bbe33

fix source stripping of pex executable arg

fd75b52

more robust

1c17056

cognifloyd commented Feb 8, 2024

View reviewed changes

Merge branch 'main' into cognifloyd/pex-exe-arg

8908ba7

cognifloyd marked this pull request as ready for review February 8, 2024 00:48

cognifloyd added 2 commits February 7, 2024 19:05

fmt fix

97cb716

rules graph fix

df068ab

cognifloyd changed the title ~~plumb pex --executable arg~~ Add support for the pex --executable argument Feb 8, 2024

more fix

2a86881

cognifloyd requested review from benjyw, huonw and thejcannon February 8, 2024 03:14

benjyw approved these changes Feb 8, 2024

View reviewed changes

benjyw reviewed Feb 8, 2024

View reviewed changes

cognifloyd and others added 2 commits February 8, 2024 23:10

Fix comment typos

36aed98

Co-authored-by: Benjy Weinberger <benjyw@gmail.com>

use os.path.join

fdbf202

cognifloyd merged commit 882ffe6 into main Feb 9, 2024
24 checks passed

cognifloyd deleted the cognifloyd/pex-exe-arg branch February 9, 2024 16:20

purajit mentioned this pull request Feb 15, 2024

add more module mappings for popular packages #20551

Merged

cognifloyd mentioned this pull request May 18, 2024

Upgrade pantsbuild from 2.18 to 2.23.0.dev2 StackStorm/st2#6200

Draft

cognifloyd commented Jun 11, 2024

View reviewed changes

cognifloyd mentioned this pull request Jun 11, 2024

Fix running python_sources with pex --executable #21047

Merged

This was referenced Jun 18, 2024

Fix running python_sources with pex --executable (Cherry-pick of #21047) #21083

Closed

Fix running python_sources with pex --executable (Cherry-pick of #21047) #21084

Merged

cognifloyd mentioned this pull request Jun 18, 2024

Fix running python_sources with pex --executable (Cherry-pick of #21047) #21085

Merged

cognifloyd mentioned this pull request Jun 18, 2024

Fix running python_sources with pex --executable (Cherry-pick of #21047) #21086

Merged

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the pex `--executable` argument #20497

Add support for the pex `--executable` argument #20497

cognifloyd commented Feb 6, 2024 •

edited

Loading

cognifloyd Feb 8, 2024

benjyw left a comment

benjyw Feb 8, 2024

cognifloyd Feb 8, 2024

benjyw Feb 8, 2024

benjyw Feb 8, 2024

cognifloyd Feb 9, 2024

cognifloyd Jun 11, 2024

cognifloyd Jun 11, 2024

		@@ -213,6 +213,52 @@ def test_local_dist() -> None:
		assert result.stdout == "LOCAL DIST\n"


		def test_local_dist_with_executable_main() -> None:

	main=executable or console_script or entry_point.val,
	main=console_script or executable or entry_point.val,

Add support for the pex --executable argument #20497

Add support for the pex --executable argument #20497

Conversation

cognifloyd commented Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for the pex `--executable` argument #20497

Add support for the pex `--executable` argument #20497

cognifloyd commented Feb 6, 2024 •

edited

Loading