-
-
Notifications
You must be signed in to change notification settings - Fork 615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for the pex --executable
argument
#20497
Changes from 9 commits
088d7ff
2afda25
7ebca2d
fa0fb1d
8dd1e51
1682cff
02bbe33
fd75b52
1c17056
8908ba7
97cb716
df068ab
2a86881
36aed98
fdbf202
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,7 @@ | |||||
from pants.backend.python.subsystems.debugpy import DebugPy | ||||||
from pants.backend.python.target_types import ( | ||||||
ConsoleScript, | ||||||
Executable, | ||||||
PexEntryPointField, | ||||||
ResolvedPexEntryPoint, | ||||||
ResolvePexEntryPointRequest, | ||||||
|
@@ -43,6 +44,7 @@ async def _create_python_source_run_request( | |||||
run_in_sandbox: bool, | ||||||
pex_path: Iterable[Pex] = (), | ||||||
console_script: Optional[ConsoleScript] = None, | ||||||
executable: Optional[Executable] = None, | ||||||
) -> RunRequest: | ||||||
addresses = [address] | ||||||
entry_point, transitive_targets = await MultiGet( | ||||||
|
@@ -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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's fine the way it is, it just scans oddly. |
||||||
additional_args=( | ||||||
# N.B.: Since we cobble together the runtime environment via PEX_EXTRA_SYS_PATH | ||||||
# below, it's important for any app that re-executes itself that these environment | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -213,6 +213,52 @@ def test_local_dist() -> None: | |
assert result.stdout == "LOCAL DIST\n" | ||
|
||
|
||
def test_local_dist_with_executable_main() -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a copy of |
||
sources = { | ||
"foo/bar.py": "BAR = 'LOCAL DIST'", | ||
"foo/setup.py": dedent( | ||
"""\ | ||
from setuptools import setup # pants: no-infer-dep | ||
|
||
# Double-brace the package_dir to avoid setup_tmpdir treating it as a format. | ||
setup(name="foo", version="9.8.7", packages=["foo"], package_dir={{"foo": "."}},) | ||
""" | ||
), | ||
"foo/foo-bar-main": "from foo.bar import BAR; print(BAR)", | ||
"foo/BUILD": dedent( | ||
"""\ | ||
python_sources(name="lib", sources=["bar.py", "setup.py"]) | ||
|
||
python_sources(name="main_exe", sources=["foo-bar-main"]) | ||
|
||
python_distribution( | ||
name="dist", | ||
dependencies=[":lib"], | ||
provides=python_artifact(name="foo", version="9.8.7"), | ||
sdist=False, | ||
generate_setup=False, | ||
) | ||
|
||
pex_binary( | ||
name="bin", | ||
executable="foo-bar-main", | ||
# Force-exclude any dep on bar.py, so the only way to consume it is via the dist. | ||
dependencies=[":main_exe", ":dist", "!!:lib"]) | ||
""" | ||
), | ||
} | ||
with setup_tmpdir(sources) as tmpdir: | ||
args = [ | ||
"--backend-packages=pants.backend.python", | ||
f"--source-root-patterns=['/{tmpdir}']", | ||
"run", | ||
f"{tmpdir}/foo:bin", | ||
] | ||
result = run_pants(args) | ||
result.assert_success() | ||
assert result.stdout == "LOCAL DIST\n" | ||
|
||
|
||
def test_run_script_from_3rdparty_dist_issue_13747() -> None: | ||
sources = { | ||
"src/BUILD": dedent( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from dataclasses import dataclass | ||
from pathlib import PurePath | ||
from typing import Optional | ||
|
||
from pants.backend.python.goals.run_helper import ( | ||
_create_python_source_run_dap_request, | ||
|
@@ -10,6 +12,7 @@ | |
from pants.backend.python.subsystems.debugpy import DebugPy | ||
from pants.backend.python.subsystems.setup import PythonSetup | ||
from pants.backend.python.target_types import ( | ||
Executable, | ||
InterpreterConstraintsField, | ||
PexEntryPointField, | ||
PythonRunGoalUseSandboxField, | ||
|
@@ -48,6 +51,18 @@ def should_use_sandbox(self, python_setup: PythonSetup) -> bool: | |
return python_setup.default_run_goal_use_sandbox | ||
return self._run_goal_use_sandbox.value | ||
|
||
def _executable_main(self) -> Optional[Executable]: | ||
source = PurePath(self.source.value) | ||
source_name = source.stem if source.suffix == ".py" else source.name | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The integration tests are only testing running a |
||
else: | ||
# The module is importable, so entry_point will do the heavy lifting instead. | ||
executable = None | ||
return executable | ||
|
||
|
||
@rule(level=LogLevel.DEBUG) | ||
async def create_python_source_run_request( | ||
|
@@ -56,6 +71,7 @@ async def create_python_source_run_request( | |
return await _create_python_source_run_request( | ||
field_set.address, | ||
entry_point_field=PexEntryPointField(field_set.source.value, field_set.address), | ||
executable=field_set._executable_main(), | ||
pex_env=pex_env, | ||
run_in_sandbox=field_set.should_use_sandbox(python_setup), | ||
) | ||
|
@@ -70,6 +86,7 @@ async def create_python_source_run_in_sandbox_request( | |
run_request = await _create_python_source_run_request( | ||
field_set.address, | ||
entry_point_field=PexEntryPointField(field_set.source.value, field_set.address), | ||
executable=field_set._executable_main(), | ||
pex_env=pex_env, | ||
run_in_sandbox=True, | ||
) | ||
|
@@ -101,6 +118,7 @@ async def create_python_source_debug_adapter_request( | |
run_request = await _create_python_source_run_request( | ||
field_set.address, | ||
entry_point_field=PexEntryPointField(field_set.source.value, field_set.address), | ||
executable=field_set._executable_main(), | ||
pex_env=pex_env, | ||
pex_path=[debugpy_pex], | ||
run_in_sandbox=run_in_sandbox, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.