From 4bde8988be0762dcc24eb73c3a0c9279db7cb9a5 Mon Sep 17 00:00:00 2001 From: UebelAndre Date: Sat, 23 Dec 2023 18:00:32 -0800 Subject: [PATCH] Added type hints to `@rules_python//python/runfiles` --- .github/workflows/mypy.yaml | 32 ++++ python/runfiles/BUILD.bazel | 2 + python/runfiles/py.typed | 0 python/runfiles/runfiles.py | 266 +++++++++++++++----------------- tests/runfiles/runfiles_test.py | 53 ++++--- 5 files changed, 188 insertions(+), 165 deletions(-) create mode 100644 .github/workflows/mypy.yaml create mode 100644 python/runfiles/py.typed diff --git a/.github/workflows/mypy.yaml b/.github/workflows/mypy.yaml new file mode 100644 index 0000000000..0abd6749d8 --- /dev/null +++ b/.github/workflows/mypy.yaml @@ -0,0 +1,32 @@ +name: mypy + +on: + push: + branches: + - main + pull_request: + types: + - opened + - synchronize + +defaults: + run: + shell: bash + +jobs: + ci: + runs-on: ubuntu-20.04 + steps: + # Checkout the code + - uses: actions/checkout@v2 + - uses: jpetrucciani/mypy-check@master + with: + requirements: 1.6.0 + python_version: 3.11 + path: 'python/runfiles' + - uses: jpetrucciani/mypy-check@master + with: + requirements: 1.6.0 + python_version: 3.11 + path: 'tests/runfiles' + diff --git a/python/runfiles/BUILD.bazel b/python/runfiles/BUILD.bazel index c6cfc2fa94..55c25c870d 100644 --- a/python/runfiles/BUILD.bazel +++ b/python/runfiles/BUILD.bazel @@ -27,6 +27,7 @@ py_library( "__init__.py", "runfiles.py", ], + data = ["py.typed"], imports = [ # Add the repo root so `import python.runfiles.runfiles` works. This makes it agnostic # to the --experimental_python_import_all_repositories setting. @@ -49,6 +50,7 @@ py_wheel( dist_folder = "dist", distribution = "bazel_runfiles", homepage = "https://github.com/bazelbuild/rules_python", + python_requires = ">=3.7", strip_path_prefixes = ["python"], twine = "@publish_deps_twine//:pkg", # this can be replaced by building with --stamp --embed_label=1.2.3 diff --git a/python/runfiles/py.typed b/python/runfiles/py.typed new file mode 100644 index 0000000000..e69de29bb2 diff --git a/python/runfiles/runfiles.py b/python/runfiles/runfiles.py index 9bdb61b56a..88ea0a2682 100644 --- a/python/runfiles/runfiles.py +++ b/python/runfiles/runfiles.py @@ -20,76 +20,111 @@ import os import posixpath import sys +from typing import Dict, Optional, Tuple, Union -if False: - # Mypy needs these symbols imported, but since they only exist in python 3.5+, - # this import may fail at runtime. Luckily mypy can follow this conditional import. - from typing import Callable, Dict, Optional, Tuple, Union - - -def CreateManifestBased(manifest_path): - # type: (str) -> _Runfiles - return _Runfiles(_ManifestBased(manifest_path)) +class _ManifestBased: + """`Runfiles` strategy that parses a runfiles-manifest to look up runfiles.""" -def CreateDirectoryBased(runfiles_dir_path): - # type: (str) -> _Runfiles - return _Runfiles(_DirectoryBased(runfiles_dir_path)) + def __init__(self, path: str) -> None: + if not path: + raise ValueError() + if not isinstance(path, str): + raise TypeError() + self._path = path + self._runfiles = _ManifestBased._LoadRunfiles(path) + def RlocationChecked(self, path: str) -> Optional[str]: + """Returns the runtime path of a runfile.""" + exact_match = self._runfiles.get(path) + if exact_match: + return exact_match + # If path references a runfile that lies under a directory that + # itself is a runfile, then only the directory is listed in the + # manifest. Look up all prefixes of path in the manifest and append + # the relative path from the prefix to the looked up path. + prefix_end = len(path) + while True: + prefix_end = path.rfind("/", 0, prefix_end - 1) + if prefix_end == -1: + return None + prefix_match = self._runfiles.get(path[0:prefix_end]) + if prefix_match: + return prefix_match + "/" + path[prefix_end + 1 :] -def Create(env=None): - # type: (Optional[Dict[str, str]]) -> Optional[_Runfiles] - """Returns a new `Runfiles` instance. + @staticmethod + def _LoadRunfiles(path: str) -> Dict[str, str]: + """Loads the runfiles manifest.""" + result = {} + with open(path, "r") as f: + for line in f: + line = line.strip() + if line: + tokens = line.split(" ", 1) + if len(tokens) == 1: + result[line] = line + else: + result[tokens[0]] = tokens[1] + return result - The returned object is either: - - manifest-based, meaning it looks up runfile paths from a manifest file, or - - directory-based, meaning it looks up runfile paths under a given directory - path + def _GetRunfilesDir(self) -> str: + if self._path.endswith("/MANIFEST") or self._path.endswith("\\MANIFEST"): + return self._path[: -len("/MANIFEST")] + elif self._path.endswith(".runfiles_manifest"): + return self._path[: -len("_manifest")] + else: + return "" - If `env` contains "RUNFILES_MANIFEST_FILE" with non-empty value, this method - returns a manifest-based implementation. The object eagerly reads and caches - the whole manifest file upon instantiation; this may be relevant for - performance consideration. + def EnvVars(self) -> Dict[str, str]: + directory = self._GetRunfilesDir() + return { + "RUNFILES_MANIFEST_FILE": self._path, + "RUNFILES_DIR": directory, + # TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can + # pick up RUNFILES_DIR. + "JAVA_RUNFILES": directory, + } - Otherwise, if `env` contains "RUNFILES_DIR" with non-empty value (checked in - this priority order), this method returns a directory-based implementation. - If neither cases apply, this method returns null. +class _DirectoryBased: + """`Runfiles` strategy that appends runfiles paths to the runfiles root.""" - Args: - env: {string: string}; optional; the map of environment variables. If None, - this function uses the environment variable map of this process. - Raises: - IOError: if some IO error occurs. - """ - env_map = os.environ if env is None else env - manifest = env_map.get("RUNFILES_MANIFEST_FILE") - if manifest: - return CreateManifestBased(manifest) + def __init__(self, path: str) -> None: + if not path: + raise ValueError() + if not isinstance(path, str): + raise TypeError() + self._runfiles_root = path - directory = env_map.get("RUNFILES_DIR") - if directory: - return CreateDirectoryBased(directory) + def RlocationChecked(self, path: str) -> str: + # Use posixpath instead of os.path, because Bazel only creates a runfiles + # tree on Unix platforms, so `Create()` will only create a directory-based + # runfiles strategy on those platforms. + return posixpath.join(self._runfiles_root, path) - return None + def EnvVars(self) -> Dict[str, str]: + return { + "RUNFILES_DIR": self._runfiles_root, + # TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can + # pick up RUNFILES_DIR. + "JAVA_RUNFILES": self._runfiles_root, + } -class _Runfiles(object): +class Runfiles: """Returns the runtime location of runfiles. Runfiles are data-dependencies of Bazel-built binaries and tests. """ - def __init__(self, strategy): - # type: (Union[_ManifestBased, _DirectoryBased]) -> None + def __init__(self, strategy: Union[_ManifestBased, _DirectoryBased]) -> None: self._strategy = strategy self._python_runfiles_root = _FindPythonRunfilesRoot() self._repo_mapping = _ParseRepoMapping( strategy.RlocationChecked("_repo_mapping") ) - def Rlocation(self, path, source_repo=None): - # type: (str, Optional[str]) -> Optional[str] + def Rlocation(self, path: str, source_repo: Optional[str] = None) -> Optional[str]: """Returns the runtime path of a runfile. Runfiles are data-dependencies of Bazel-built binaries and tests. @@ -156,11 +191,13 @@ def Rlocation(self, path, source_repo=None): # target_repo is an apparent repository name. Look up the corresponding # canonical repository name with respect to the current repository, # identified by its canonical name. - target_canonical = self._repo_mapping[(source_repo, target_repo)] - return self._strategy.RlocationChecked(target_canonical + "/" + remainder) + if source_repo: + target_canonical = self._repo_mapping[(source_repo, target_repo)] + return self._strategy.RlocationChecked(target_canonical + "/" + remainder) - def EnvVars(self): - # type: () -> Dict[str, str] + return None + + def EnvVars(self) -> Dict[str, str]: """Returns environment variables for subprocesses. The caller should set the returned key-value pairs in the environment of @@ -173,8 +210,7 @@ def EnvVars(self): """ return self._strategy.EnvVars() - def CurrentRepository(self, frame=1): - # type: (int) -> str + def CurrentRepository(self, frame: int = 1) -> str: """Returns the canonical name of the caller's Bazel repository. For example, this function returns '' (the empty string) when called @@ -204,12 +240,11 @@ def CurrentRepository(self, frame=1): ValueError: if the caller cannot be determined or the caller's file path is not contained in the Python runfiles tree """ - # pylint:disable=protected-access # for sys._getframe - # pylint:disable=raise-missing-from # we're still supporting Python 2 try: + # pylint: disable-next=protected-access caller_path = inspect.getfile(sys._getframe(frame)) - except (TypeError, ValueError): - raise ValueError("failed to determine caller's file path") + except (TypeError, ValueError) as exc: + raise ValueError("failed to determine caller's file path") from exc caller_runfiles_path = os.path.relpath(caller_path, self._python_runfiles_root) if caller_runfiles_path.startswith(".." + os.path.sep): raise ValueError( @@ -233,8 +268,7 @@ def CurrentRepository(self, frame=1): return caller_runfiles_directory -def _FindPythonRunfilesRoot(): - # type: () -> str +def _FindPythonRunfilesRoot() -> str: """Finds the root of the Python runfiles tree.""" root = __file__ # Walk up our own runfiles path to the root of the runfiles tree from which @@ -246,8 +280,7 @@ def _FindPythonRunfilesRoot(): return root -def _ParseRepoMapping(repo_mapping_path): - # type: (Optional[str]) -> Dict[Tuple[str, str], str] +def _ParseRepoMapping(repo_mapping_path: Optional[str]) -> Dict[Tuple[str, str], str]: """Parses the repository mapping manifest.""" # If the repository mapping file can't be found, that is not an error: We # might be running without Bzlmod enabled or there may not be any runfiles. @@ -271,98 +304,45 @@ def _ParseRepoMapping(repo_mapping_path): return repo_mapping -class _ManifestBased(object): - """`Runfiles` strategy that parses a runfiles-manifest to look up runfiles.""" +def CreateManifestBased(manifest_path: str) -> Runfiles: + return Runfiles(_ManifestBased(manifest_path)) - def __init__(self, path): - # type: (str) -> None - if not path: - raise ValueError() - if not isinstance(path, str): - raise TypeError() - self._path = path - self._runfiles = _ManifestBased._LoadRunfiles(path) - def RlocationChecked(self, path): - # type: (str) -> Optional[str] - """Returns the runtime path of a runfile.""" - exact_match = self._runfiles.get(path) - if exact_match: - return exact_match - # If path references a runfile that lies under a directory that - # itself is a runfile, then only the directory is listed in the - # manifest. Look up all prefixes of path in the manifest and append - # the relative path from the prefix to the looked up path. - prefix_end = len(path) - while True: - prefix_end = path.rfind("/", 0, prefix_end - 1) - if prefix_end == -1: - return None - prefix_match = self._runfiles.get(path[0:prefix_end]) - if prefix_match: - return prefix_match + "/" + path[prefix_end + 1 :] +def CreateDirectoryBased(runfiles_dir_path: str) -> Runfiles: + return Runfiles(_DirectoryBased(runfiles_dir_path)) - @staticmethod - def _LoadRunfiles(path): - # type: (str) -> Dict[str, str] - """Loads the runfiles manifest.""" - result = {} - with open(path, "r") as f: - for line in f: - line = line.strip() - if line: - tokens = line.split(" ", 1) - if len(tokens) == 1: - result[line] = line - else: - result[tokens[0]] = tokens[1] - return result - def _GetRunfilesDir(self): - # type: () -> str - if self._path.endswith("/MANIFEST") or self._path.endswith("\\MANIFEST"): - return self._path[: -len("/MANIFEST")] - elif self._path.endswith(".runfiles_manifest"): - return self._path[: -len("_manifest")] - else: - return "" +def Create(env: Optional[Dict[str, str]] = None) -> Optional[Runfiles]: + """Returns a new `Runfiles` instance. - def EnvVars(self): - # type: () -> Dict[str, str] - directory = self._GetRunfilesDir() - return { - "RUNFILES_MANIFEST_FILE": self._path, - "RUNFILES_DIR": directory, - # TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can - # pick up RUNFILES_DIR. - "JAVA_RUNFILES": directory, - } + The returned object is either: + - manifest-based, meaning it looks up runfile paths from a manifest file, or + - directory-based, meaning it looks up runfile paths under a given directory + path + If `env` contains "RUNFILES_MANIFEST_FILE" with non-empty value, this method + returns a manifest-based implementation. The object eagerly reads and caches + the whole manifest file upon instantiation; this may be relevant for + performance consideration. -class _DirectoryBased(object): - """`Runfiles` strategy that appends runfiles paths to the runfiles root.""" + Otherwise, if `env` contains "RUNFILES_DIR" with non-empty value (checked in + this priority order), this method returns a directory-based implementation. - def __init__(self, path): - # type: (str) -> None - if not path: - raise ValueError() - if not isinstance(path, str): - raise TypeError() - self._runfiles_root = path + If neither cases apply, this method returns null. - def RlocationChecked(self, path): - # type: (str) -> str + Args: + env: {string: string}; optional; the map of environment variables. If None, + this function uses the environment variable map of this process. + Raises: + IOError: if some IO error occurs. + """ + env_map = os.environ if env is None else env + manifest = env_map.get("RUNFILES_MANIFEST_FILE") + if manifest: + return CreateManifestBased(manifest) - # Use posixpath instead of os.path, because Bazel only creates a runfiles - # tree on Unix platforms, so `Create()` will only create a directory-based - # runfiles strategy on those platforms. - return posixpath.join(self._runfiles_root, path) + directory = env_map.get("RUNFILES_DIR") + if directory: + return CreateDirectoryBased(directory) - def EnvVars(self): - # type: () -> Dict[str, str] - return { - "RUNFILES_DIR": self._runfiles_root, - # TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can - # pick up RUNFILES_DIR. - "JAVA_RUNFILES": self._runfiles_root, - } + return None diff --git a/tests/runfiles/runfiles_test.py b/tests/runfiles/runfiles_test.py index 5cc95688df..8cb4ce22b7 100644 --- a/tests/runfiles/runfiles_test.py +++ b/tests/runfiles/runfiles_test.py @@ -1,4 +1,3 @@ -# pylint: disable=g-bad-file-header # Copyright 2018 The Bazel Authors. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -13,17 +12,20 @@ # See the License for the specific language governing permissions and # limitations under the License. +# pylint: disable=g-bad-file-header + import os import tempfile import unittest +from typing import Any, List, Optional from python.runfiles import runfiles class RunfilesTest(unittest.TestCase): - # """Unit tests for `runfiles.Runfiles`.""" + """Unit tests for `rules_python.python.runfiles.Runfiles`.""" - def testRlocationArgumentValidation(self): + def testRlocationArgumentValidation(self) -> None: r = runfiles.Create({"RUNFILES_DIR": "whatever"}) self.assertRaises(ValueError, lambda: r.Rlocation(None)) self.assertRaises(ValueError, lambda: r.Rlocation("")) @@ -61,7 +63,7 @@ def testRlocationArgumentValidation(self): lambda: r.Rlocation("\\foo"), ) - def testCreatesManifestBasedRunfiles(self): + def testCreatesManifestBasedRunfiles(self) -> None: with _MockFile(contents=["a/b c/d"]) as mf: r = runfiles.Create( { @@ -73,7 +75,7 @@ def testCreatesManifestBasedRunfiles(self): self.assertEqual(r.Rlocation("a/b"), "c/d") self.assertIsNone(r.Rlocation("foo")) - def testManifestBasedRunfilesEnvVars(self): + def testManifestBasedRunfilesEnvVars(self) -> None: with _MockFile(name="MANIFEST") as mf: r = runfiles.Create( { @@ -126,7 +128,7 @@ def testManifestBasedRunfilesEnvVars(self): }, ) - def testCreatesDirectoryBasedRunfiles(self): + def testCreatesDirectoryBasedRunfiles(self) -> None: r = runfiles.Create( { "RUNFILES_DIR": "runfiles/dir", @@ -136,7 +138,7 @@ def testCreatesDirectoryBasedRunfiles(self): self.assertEqual(r.Rlocation("a/b"), "runfiles/dir/a/b") self.assertEqual(r.Rlocation("foo"), "runfiles/dir/foo") - def testDirectoryBasedRunfilesEnvVars(self): + def testDirectoryBasedRunfilesEnvVars(self) -> None: r = runfiles.Create( { "RUNFILES_DIR": "runfiles/dir", @@ -151,13 +153,13 @@ def testDirectoryBasedRunfilesEnvVars(self): }, ) - def testFailsToCreateManifestBasedBecauseManifestDoesNotExist(self): + def testFailsToCreateManifestBasedBecauseManifestDoesNotExist(self) -> None: def _Run(): runfiles.Create({"RUNFILES_MANIFEST_FILE": "non-existing path"}) self.assertRaisesRegex(IOError, "non-existing path", _Run) - def testFailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined(self): + def testFailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined(self) -> None: with _MockFile(contents=["a b"]) as mf: runfiles.Create( { @@ -175,7 +177,7 @@ def testFailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined(self): self.assertIsNone(runfiles.Create({"TEST_SRCDIR": "always ignored"})) self.assertIsNone(runfiles.Create({"FOO": "bar"})) - def testManifestBasedRlocation(self): + def testManifestBasedRlocation(self) -> None: with _MockFile( contents=[ "Foo/runfile1", @@ -205,7 +207,7 @@ def testManifestBasedRlocation(self): else: self.assertEqual(r.Rlocation("/foo"), "/foo") - def testManifestBasedRlocationWithRepoMappingFromMain(self): + def testManifestBasedRlocationWithRepoMappingFromMain(self) -> None: with _MockFile( contents=[ ",config.json,config.json~1.2.3", @@ -280,7 +282,7 @@ def testManifestBasedRlocationWithRepoMappingFromMain(self): self.assertIsNone(r.Rlocation("my_module", "")) self.assertIsNone(r.Rlocation("protobuf", "")) - def testManifestBasedRlocationWithRepoMappingFromOtherRepo(self): + def testManifestBasedRlocationWithRepoMappingFromOtherRepo(self) -> None: with _MockFile( contents=[ ",config.json,config.json~1.2.3", @@ -362,7 +364,7 @@ def testManifestBasedRlocationWithRepoMappingFromOtherRepo(self): self.assertIsNone(r.Rlocation("my_module", "protobuf~3.19.2")) self.assertIsNone(r.Rlocation("protobuf", "protobuf~3.19.2")) - def testDirectoryBasedRlocation(self): + def testDirectoryBasedRlocation(self) -> None: # The _DirectoryBased strategy simply joins the runfiles directory and the # runfile's path on a "/". This strategy does not perform any normalization, # nor does it check that the path exists. @@ -374,7 +376,7 @@ def testDirectoryBasedRlocation(self): else: self.assertEqual(r.Rlocation("/foo"), "/foo") - def testDirectoryBasedRlocationWithRepoMappingFromMain(self): + def testDirectoryBasedRlocationWithRepoMappingFromMain(self) -> None: with _MockFile( name="_repo_mapping", contents=[ @@ -441,7 +443,7 @@ def testDirectoryBasedRlocationWithRepoMappingFromMain(self): self.assertEqual(r.Rlocation("config.json", ""), dir + "/config.json") - def testDirectoryBasedRlocationWithRepoMappingFromOtherRepo(self): + def testDirectoryBasedRlocationWithRepoMappingFromOtherRepo(self) -> None: with _MockFile( name="_repo_mapping", contents=[ @@ -513,7 +515,7 @@ def testDirectoryBasedRlocationWithRepoMappingFromOtherRepo(self): r.Rlocation("config.json", "protobuf~3.19.2"), dir + "/config.json" ) - def testCurrentRepository(self): + def testCurrentRepository(self) -> None: # Under bzlmod, the current repository name is the empty string instead # of the name in the workspace file. if bool(int(os.environ["BZLMOD_ENABLED"])): @@ -525,28 +527,35 @@ def testCurrentRepository(self): ) @staticmethod - def IsWindows(): + def IsWindows() -> bool: return os.name == "nt" -class _MockFile(object): - def __init__(self, name=None, contents=None): +class _MockFile: + def __init__( + self, name: Optional[str] = None, contents: Optional[List[Any]] = None + ) -> None: self._contents = contents or [] self._name = name or "x" self._path = None - def __enter__(self): + def __enter__(self) -> None: tmpdir = os.environ.get("TEST_TMPDIR") self._path = os.path.join(tempfile.mkdtemp(dir=tmpdir), self._name) with open(self._path, "wt") as f: f.writelines(l + "\n" for l in self._contents) return self - def __exit__(self, exc_type, exc_value, traceback): + def __exit__( + self, + exc_type: Any, # pylint: disable=unused-argument + exc_value: Any, # pylint: disable=unused-argument + traceback: Any, # pylint: disable=unused-argument + ) -> None: os.remove(self._path) os.rmdir(os.path.dirname(self._path)) - def Path(self): + def Path(self) -> Optional[str]: return self._path