Fix run not working with foo:main entry point. (pantsbuild#10821)

Previously we assumed that the entry point, if explicit,
was a module. However it may be module:file. This change
delegates interpreting the entry point string to Pex itself.

It also addresses a TODO in computing the implicit entry
point from sources. Now we just do it by path manipulation,
without snapshotting, as we now have the ability to do so.

[ci skip-rust]

[ci skip-build-wheels]

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

@@ -25,12 +25,12 @@
 )
 from pants.core.goals.binary import BinaryFieldSet, CreatedBinary
 from pants.core.goals.run import RunFieldSet
-from pants.core.util_rules.source_files import SourceFiles
-from pants.core.util_rules.stripped_source_files import StrippedSourceFiles
+from pants.engine.fs import PathGlobs, Paths
 from pants.engine.rules import Get, collect_rules, rule
-from pants.engine.target import HydratedSources, HydrateSourcesRequest
+from pants.engine.target import InvalidFieldException
 from pants.engine.unions import UnionRule
-from pants.option.global_options import GlobalOptions
+from pants.option.global_options import FilesNotFoundBehavior, GlobalOptions
+from pants.source.source_root import SourceRoot, SourceRootRequest
 from pants.util.logging import LogLevel
 
 logger = logging.getLogger(__name__)
@@ -78,14 +78,23 @@ async def create_python_binary(
 ) -> CreatedBinary:
     entry_point = field_set.entry_point.value
     if entry_point is None:
-        # TODO: This is overkill? We don't need to hydrate the sources and strip snapshots,
-        #  we only need the path relative to the source root.
-        binary_sources = await Get(HydratedSources, HydrateSourcesRequest(field_set.sources))
-        stripped_binary_sources = await Get(
-            StrippedSourceFiles, SourceFiles(binary_sources.snapshot, ())
+        binary_source_paths = await Get(
+            Paths, PathGlobs, field_set.sources.path_globs(FilesNotFoundBehavior.error)
+        )
+        if len(binary_source_paths.files) != 1:
+            raise InvalidFieldException(
+                "No `entry_point` was set for the target "
+                f"{repr(field_set.address)}, so it must have exactly one source, but it has "
+                f"{len(binary_source_paths.files)}"
+            )
+        entry_point_path = binary_source_paths.files[0]
+        source_root = await Get(
+            SourceRoot,
+            SourceRootRequest,
+            SourceRootRequest.for_file(entry_point_path),
         )
         entry_point = PythonBinarySources.translate_source_file_to_entry_point(
-            stripped_binary_sources.snapshot.files
+            os.path.relpath(entry_point_path, source_root.path)
         )
 
     disambiguated_output_filename = os.path.join(

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

@@ -13,18 +13,13 @@
     PythonSourceFilesRequest,
 )
 from pants.core.goals.run import RunFieldSet, RunRequest
-from pants.core.util_rules.source_files import SourceFiles
-from pants.core.util_rules.stripped_source_files import StrippedSourceFiles
 from pants.engine.addresses import Addresses
-from pants.engine.fs import Digest, MergeDigests
+from pants.engine.fs import Digest, MergeDigests, PathGlobs, Paths
 from pants.engine.rules import Get, MultiGet, collect_rules, rule
-from pants.engine.target import (
-    HydratedSources,
-    HydrateSourcesRequest,
-    InvalidFieldException,
-    TransitiveTargets,
-)
+from pants.engine.target import InvalidFieldException, TransitiveTargets
 from pants.engine.unions import UnionRule
+from pants.option.global_options import FilesNotFoundBehavior
+from pants.source.source_root import SourceRoot, SourceRootRequest
 from pants.util.logging import LogLevel
 
 
@@ -36,21 +31,24 @@ async def create_python_binary_run_request(
 ) -> RunRequest:
     entry_point = field_set.entry_point.value
     if entry_point is None:
-        # TODO: This is overkill? We don't need to hydrate the sources and strip snapshots,
-        #  we only need the path relative to the source root.
-        binary_sources = await Get(HydratedSources, HydrateSourcesRequest(field_set.sources))
-        stripped_binary_sources = await Get(
-            StrippedSourceFiles, SourceFiles(binary_sources.snapshot, ())
+        binary_source_paths = await Get(
+            Paths, PathGlobs, field_set.sources.path_globs(FilesNotFoundBehavior.error)
         )
-        entry_point = PythonBinarySources.translate_source_file_to_entry_point(
-            stripped_binary_sources.snapshot.files
+        if len(binary_source_paths.files) != 1:
+            raise InvalidFieldException(
+                "No `entry_point` was set for the target "
+                f"{repr(field_set.address)}, so it must have exactly one source, but it has "
+                f"{len(binary_source_paths.files)}"
+            )
+        entry_point_path = binary_source_paths.files[0]
+        source_root = await Get(
+            SourceRoot,
+            SourceRootRequest,
+            SourceRootRequest.for_file(entry_point_path),
         )
-    if entry_point is None:
-        raise InvalidFieldException(
-            "You must either specify `sources` or `entry_point` for the target "
-            f"{repr(field_set.address)} in order to run it, but both fields were undefined."
+        entry_point = PythonBinarySources.translate_source_file_to_entry_point(
+            os.path.relpath(entry_point_path, source_root.path)
         )
-
     transitive_targets = await Get(TransitiveTargets, Addresses([field_set.address]))
 
     # Note that we get an intermediate PexRequest here (instead of going straight to a Pex)
@@ -75,6 +73,9 @@ async def create_python_binary_run_request(
             interpreter_constraints=requirements_pex_request.interpreter_constraints,
             additional_args=field_set.generate_additional_args(python_binary_defaults),
             internal_only=True,
+            # Note that the entry point file is not in the Pex itself, but on the
+            # PEX_PATH. This works fine!
+            entry_point=entry_point,
         ),
     )
 
@@ -92,9 +93,7 @@ async def create_python_binary_run_request(
     def in_chroot(relpath: str) -> str:
         return os.path.join("{chroot}", relpath)
 
-    args = pex_env.create_argv(
-        in_chroot(runner_pex.name), "-m", entry_point, python=runner_pex.python
-    )
+    args = pex_env.create_argv(in_chroot(runner_pex.name), python=runner_pex.python)
 
     chrooted_source_roots = [in_chroot(sr) for sr in sources.source_roots]
     extra_env = {

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

@@ -3,10 +3,20 @@
 
 from textwrap import dedent
 
+import pytest
+
 from pants.testutil.pants_integration_test import run_pants, setup_tmpdir
 
 
-def test_run_sample_script() -> None:
+@pytest.mark.parametrize(
+    "tgt_content",
+    [
+        "python_binary(sources=['app.py'])",
+        "python_binary(sources=['app.py'], entry_point='project.app')",
+        "python_binary(sources=['app.py'], entry_point='project.app:main')",
+    ],
+)
+def test_run_sample_script(tgt_content: str) -> None:
     """Test that we properly run a `python_binary` target.
 
     This checks a few things:
@@ -21,13 +31,16 @@ def test_run_sample_script() -> None:
             from utils.strutil import upper_case
 
 
-            if __name__ == "__main__":
+            def main():
                 print(upper_case("Hello world."))
                 print("Hola, mundo.", file=sys.stderr)
                 sys.exit(23)
+
+            if __name__ == "__main__":
+              main()
             """
         ),
-        "src_root1/project/BUILD": "python_binary(sources=['app.py'])",
+        "src_root1/project/BUILD": tgt_content,
         "src_root2/utils/strutil.py": dedent(
             """\
             def upper_case(s):

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

@@ -4,7 +4,7 @@
 import collections.abc
 import os.path
 from textwrap import dedent
-from typing import Iterable, Optional, Sequence, Tuple, Union, cast
+from typing import Iterable, Optional, Tuple, Union, cast
 
 from pkg_resources import Requirement
 
@@ -110,11 +110,8 @@ class PythonBinarySources(PythonSources):
     expected_num_files = range(0, 2)
 
     @staticmethod
-    def translate_source_file_to_entry_point(stripped_sources: Sequence[str]) -> Optional[str]:
-        # We assume we have 0-1 sources, which is enforced by PythonBinarySources.
-        if len(stripped_sources) != 1:
-            return None
-        module_base, _ = os.path.splitext(stripped_sources[0])
+    def translate_source_file_to_entry_point(stripped_source_path: str) -> str:
+        module_base, _ = os.path.splitext(stripped_source_path)
         return module_base.replace(os.path.sep, ".")
 
 

src/python/pants/backend/python/target_types_test.py

@@ -54,16 +54,13 @@ def assert_timeout_calculated(
 
 def test_translate_source_file_to_entry_point() -> None:
     assert (
-        PythonBinarySources.translate_source_file_to_entry_point(["example/app.py"])
-        == "example.app"
+        PythonBinarySources.translate_source_file_to_entry_point("example/app.py") == "example.app"
     )
     # NB: the onus is on the call site to strip the source roots before calling this method.
     assert (
-        PythonBinarySources.translate_source_file_to_entry_point(["src/python/app.py"])
+        PythonBinarySources.translate_source_file_to_entry_point("src/python/app.py")
         == "src.python.app"
     )
-    assert PythonBinarySources.translate_source_file_to_entry_point([]) is None
-    assert PythonBinarySources.translate_source_file_to_entry_point(["f1.py", "f2.py"]) is None
 
 
 def test_requirements_field() -> None: