From 92ebffa35573d57b454c0f00faa279e07fe2e368 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Wed, 25 Dec 2019 03:52:13 +0000 Subject: [PATCH] add SingleFileExecutable to make it easier to consume Snapshots of executables (#8860) ### Problem We use the same boilerplate in `cloc.py` and `download_pex_bin.py` to wrap a `Snapshot` consumed by a `UrlToFetch`, e.g.: https://github.com/pantsbuild/pants/blob/43ffe73ada2fcbc2571172193c30b906d2de944f/src/python/pants/backend/python/rules/download_pex_bin.py#L64-L65 ### Solution - Create `SingleFileExecutable` dataclass in `engine/fs.py`, which validates that the input has only one file, and has a convenience method `.exe_filename` to return a relative path to the wrapped executable which can be used in a hermetic process execution. - Use `SingleFileExecutable` for both `cloc` and `pex`. ### Result Along with #8859, this helps to simplify the experience of fetching an executable via `UrlToFetch`! --- .../backend/python/rules/download_pex_bin.py | 15 +++++-- src/python/pants/engine/BUILD | 1 + src/python/pants/engine/fs.py | 34 ++++++++++++++- src/python/pants/fs/fs_test.py | 42 +++++++++++++++++++ src/python/pants/rules/core/cloc.py | 14 +++++-- src/python/pants/util/dirutil.py | 12 ++++++ tests/python/pants_test/util/test_dirutil.py | 16 +++++++ 7 files changed, 126 insertions(+), 8 deletions(-) diff --git a/src/python/pants/backend/python/rules/download_pex_bin.py b/src/python/pants/backend/python/rules/download_pex_bin.py index fcfad279b9f..36917609df2 100644 --- a/src/python/pants/backend/python/rules/download_pex_bin.py +++ b/src/python/pants/backend/python/rules/download_pex_bin.py @@ -8,7 +8,7 @@ from pants.backend.python.subsystems.python_native_code import PexBuildEnvironment from pants.backend.python.subsystems.python_setup import PythonSetup from pants.backend.python.subsystems.subprocess_environment import SubprocessEncodingEnvironment -from pants.engine.fs import Digest, Snapshot, UrlToFetch +from pants.engine.fs import Digest, SingleFileExecutable, Snapshot, UrlToFetch from pants.engine.isolated_process import ExecuteProcessRequest from pants.engine.rules import rule from pants.engine.selectors import Get @@ -16,8 +16,15 @@ @dataclass(frozen=True) class DownloadedPexBin(HermeticPex): - executable: str - directory_digest: Digest + exe: SingleFileExecutable + + @property + def executable(self) -> str: + return self.exe.exe_filename + + @property + def directory_digest(self) -> Digest: + return self.exe.directory_digest def create_execute_request( # type: ignore[override] self, @@ -63,7 +70,7 @@ async def download_pex_bin() -> DownloadedPexBin: url = 'https://github.com/pantsbuild/pex/releases/download/v1.6.12/pex' digest = Digest('ce64cb72cd23d2123dd48126af54ccf2b718d9ecb98c2ed3045ed1802e89e7e1', 1842359) snapshot = await Get[Snapshot](UrlToFetch(url, digest)) - return DownloadedPexBin(executable=snapshot.files[0], directory_digest=snapshot.directory_digest) + return DownloadedPexBin(SingleFileExecutable(snapshot)) def rules(): diff --git a/src/python/pants/engine/BUILD b/src/python/pants/engine/BUILD index e4aac543264..49d4a311b09 100644 --- a/src/python/pants/engine/BUILD +++ b/src/python/pants/engine/BUILD @@ -55,6 +55,7 @@ python_library( ':selectors', 'src/python/pants/base:project_tree', 'src/python/pants/option', + 'src/python/pants/util:dirutil', 'src/python/pants/util:meta', ], tags = {'partially_type_checked'}, diff --git a/src/python/pants/engine/fs.py b/src/python/pants/engine/fs.py index 65a67aebd49..80d386dcb26 100644 --- a/src/python/pants/engine/fs.py +++ b/src/python/pants/engine/fs.py @@ -10,7 +10,12 @@ from pants.engine.rules import RootRule from pants.option.custom_types import GlobExpansionConjunction as GlobExpansionConjunction from pants.option.global_options import GlobMatchErrorBehavior -from pants.util.dirutil import maybe_read_file, safe_delete, safe_file_dump +from pants.util.dirutil import ( + ensure_relative_file_name, + maybe_read_file, + safe_delete, + safe_file_dump, +) from pants.util.meta import frozen_after_init @@ -239,6 +244,33 @@ def materialize_directories( ) +@frozen_after_init +@dataclass(unsafe_hash=True) +class SingleFileExecutable: + """Wraps a `Snapshot` and ensures that it only contains a single file.""" + _exe_filename: Path + directory_digest: Digest + + @property + def exe_filename(self) -> str: + return ensure_relative_file_name(self._exe_filename) + + class ValidationError(ValueError): pass + + @classmethod + def _raise_validation_error(cls, snapshot: Snapshot, should_message: str) -> None: + raise cls.ValidationError(f'snapshot {snapshot} used for {cls} should {should_message}') + + def __init__(self, snapshot: Snapshot) -> None: + if len(snapshot.files) != 1: + self._raise_validation_error(snapshot, 'have exactly 1 file!') + if snapshot.directory_digest == EMPTY_DIRECTORY_DIGEST: + self._raise_validation_error(snapshot, 'have a non-empty digest!') + + self._exe_filename = Path(snapshot.files[0]) + self.directory_digest = snapshot.directory_digest + + def create_fs_rules(): """Creates rules that consume the intrinsic filesystem types.""" return [ diff --git a/src/python/pants/fs/fs_test.py b/src/python/pants/fs/fs_test.py index dfec4320d42..3d806fb8979 100644 --- a/src/python/pants/fs/fs_test.py +++ b/src/python/pants/fs/fs_test.py @@ -6,12 +6,15 @@ from pants.engine.console import Console from pants.engine.fs import ( + EMPTY_DIRECTORY_DIGEST, Digest, DirectoryToMaterialize, FileContent, InputFilesContent, MaterializeDirectoriesResult, MaterializeDirectoryResult, + SingleFileExecutable, + Snapshot, Workspace, ) from pants.engine.goal import Goal, GoalSubsystem @@ -106,3 +109,42 @@ def test_is_child_of(self): assert not is_child_of(Path("/other/random/directory/root/dist/dir"), mock_build_root) assert not is_child_of(Path("../not_root/dist/dir"), mock_build_root) + + +class SingleFileExecutableTest(TestBase): + + def test_raises_with_multiple_files(self): + input_files_content = InputFilesContent(( + FileContent(path='a.txt', content=b'test file contents'), + FileContent(path='b.txt', content=b'more test file contents'), + )) + + snapshot = self.request_single_product(Snapshot, input_files_content) + + with self.assertRaisesWithMessage( + SingleFileExecutable.ValidationError, + f'snapshot {snapshot} used for {SingleFileExecutable} should have exactly 1 file!'): + SingleFileExecutable(snapshot) + + def test_raises_empty_digest(self): + snapshot = Snapshot(EMPTY_DIRECTORY_DIGEST, files=('a.txt',), dirs=()) + + with self.assertRaisesWithMessage( + SingleFileExecutable.ValidationError, + f'snapshot {snapshot} used for {SingleFileExecutable} should have a non-empty digest!'): + SingleFileExecutable(snapshot) + + def test_accepts_single_file_snapshot(self): + input_files_content = InputFilesContent(( + FileContent(path='subdir/a.txt', content=b'test file contents'), + )) + snapshot = self.request_single_product(Snapshot, input_files_content) + + assert SingleFileExecutable(snapshot).exe_filename == './subdir/a.txt' + + input_files_content = InputFilesContent(( + FileContent(path='some_silly_file_name', content=b'test file contents'), + )) + snapshot = self.request_single_product(Snapshot, input_files_content) + + assert SingleFileExecutable(snapshot).exe_filename == './some_silly_file_name' diff --git a/src/python/pants/rules/core/cloc.py b/src/python/pants/rules/core/cloc.py index 1d74d224ab2..41999270b07 100644 --- a/src/python/pants/rules/core/cloc.py +++ b/src/python/pants/rules/core/cloc.py @@ -12,6 +12,7 @@ FileContent, FilesContent, InputFilesContent, + SingleFileExecutable, Snapshot, UrlToFetch, ) @@ -25,8 +26,15 @@ @dataclass(frozen=True) class DownloadedClocScript: """Cloc script as downloaded from the pantsbuild binaries repo.""" - script_path: str - digest: Digest + exe: SingleFileExecutable + + @property + def script_path(self) -> str: + return self.exe.exe_filename + + @property + def digest(self) -> Digest: + return self.exe.directory_digest #TODO(#7790) - We can't call this feature-complete with the v1 version of cloc @@ -37,7 +45,7 @@ async def download_cloc_script() -> DownloadedClocScript: sha_256 = "2b23012b1c3c53bd6b9dd43cd6aa75715eed4feb2cb6db56ac3fbbd2dffeac9d" digest = Digest(sha_256, 546279) snapshot = await Get[Snapshot](UrlToFetch(url, digest)) - return DownloadedClocScript(script_path=snapshot.files[0], digest=snapshot.directory_digest) + return DownloadedClocScript(SingleFileExecutable(snapshot)) class CountLinesOfCodeOptions(GoalSubsystem): diff --git a/src/python/pants/util/dirutil.py b/src/python/pants/util/dirutil.py index 9820c4daddd..5928c5f8506 100644 --- a/src/python/pants/util/dirutil.py +++ b/src/python/pants/util/dirutil.py @@ -11,6 +11,7 @@ import uuid from collections import defaultdict from contextlib import contextmanager +from pathlib import Path from typing import ( Any, Callable, @@ -74,6 +75,17 @@ def fast_relpath_optional(path: str, start: str) -> Optional[str]: return None +def ensure_relative_file_name(path: Path) -> str: + """Return a string representing the `path`, with a leading './'. + + This ensures that the returned string can be used as the executable file when executing a + subprocess, without putting the executable file on the PATH. + """ + if path.is_absolute(): + raise ValueError(f'path {path} is expected to be relative!') + return f'./{path}' + + def safe_mkdir(directory: str, clean: bool = False) -> None: """Ensure a directory is present. diff --git a/tests/python/pants_test/util/test_dirutil.py b/tests/python/pants_test/util/test_dirutil.py index 0e3df354154..3477dd67ec6 100644 --- a/tests/python/pants_test/util/test_dirutil.py +++ b/tests/python/pants_test/util/test_dirutil.py @@ -8,6 +8,7 @@ import unittest.mock from contextlib import contextmanager from dataclasses import dataclass +from pathlib import Path from typing import Iterator, Tuple, Union from pants.util import dirutil @@ -18,6 +19,7 @@ _mkdtemp_unregister_cleaner, absolute_symlink, check_no_overlapping_paths, + ensure_relative_file_name, fast_relpath, get_basedir, longest_dir_prefix, @@ -92,6 +94,20 @@ def test_fast_relpath_invalid(self) -> None: with self.assertRaises(ValueError): fast_relpath('/a/baseball', '/a/b') + def test_ensure_relative_file_name(self) -> None: + path = Path('./subdir/a.txt') + assert str(path) == 'subdir/a.txt' + assert ensure_relative_file_name(path) == './subdir/a.txt' + + path = Path('./a.txt') + assert ensure_relative_file_name(path) == './a.txt' + + path = Path('some_exe') + assert ensure_relative_file_name(path) == './some_exe' + + path = Path('./leading_slash') + assert ensure_relative_file_name(path) == './leading_slash' + @strict_patch('atexit.register') @strict_patch('os.getpid') @strict_patch('pants.util.dirutil.safe_rmtree')