[update-checkout] finish adding type hints

Author: charles-zablit

utils/update_checkout/tests/test_locked_repository.py

@@ -16,7 +16,7 @@ def _update_arguments_with_fake_path(repo_name: str, path: str) -> UpdateArgumen
         reset_to_remote=False,
         clean=False,
         stash=False,
-        cross_repos_pr=False,
+        cross_repos_pr={},
         output_prefix="",
         verbose=False,
     )

utils/update_checkout/update_checkout/git_command.py

@@ -36,7 +36,7 @@ def __init__(
 
     def __str__(self):
         return (
-            f"[{self.repo_name}] {Git._quote_command(self.command)} "
+            f"[{self.repo_name}] '{Git._quote_command(self.command)}' "
             f"returned ({self.returncode}) with the following {self.stderr}."
         )
 

utils/update_checkout/update_checkout/parallel_runner.py

@@ -1,14 +1,14 @@
 import sys
 from multiprocessing import cpu_count
 import time
-from typing import Callable, List, Any, Tuple, Union
+from typing import Callable, List, Any, Optional, Tuple, Union
 from threading import Lock, Thread, Event
 from concurrent.futures import ThreadPoolExecutor
 import shutil
 
-from swift.utils.update_checkout.update_checkout.git_command import GitException
+from .git_command import GitException
 
-from .runner_arguments import RunnerArguments, AdditionalSwiftSourcesArguments
+from .runner_arguments import RunnerArguments, AdditionalSwiftSourcesArguments, UpdateArguments
 
 
 class TaskTracker:
@@ -52,32 +52,38 @@ def done_task_counter(self) -> int:
 class MonitoredFunction:
     def __init__(
         self,
-        fn: Callable,
+        fn: Callable[..., Union[Exception]],
         task_tracker: TaskTracker,
     ):
-        self.fn = fn
+        self._fn = fn
         self._task_tracker = task_tracker
 
     def __call__(self, *args: Union[RunnerArguments, AdditionalSwiftSourcesArguments]):
         task_name = args[0].repo_name
         self._task_tracker.mark_task_as_running(task_name)
         result = None
         try:
-            result = self.fn(*args)
+            result = self._fn(*args)
         except Exception as e:
             print(e)
         finally:
             self._task_tracker.mark_task_as_done(task_name)
             return result
 
 
-class ParallelRunner:
+class ParallelRunner():
     def __init__(
         self,
-        fn: Callable,
-        pool_args: List[Union[RunnerArguments, AdditionalSwiftSourcesArguments]],
+        fn: Callable[..., None],
+        pool_args: Union[List[UpdateArguments], List[AdditionalSwiftSourcesArguments]],
         n_threads: int = 0,
     ):
+        def run_safely(*args, **kwargs):
+            try:
+                fn(*args, **kwargs)
+            except GitException as e:
+                return e
+
         if n_threads == 0:
             # Limit the number of threads as the performance regresses if the
             # number is too high.
@@ -86,7 +92,8 @@ def __init__(
         self._monitor_polling_period = 0.1
         self._terminal_width = shutil.get_terminal_size().columns
         self._pool_args = pool_args
-        self._fn = fn
+        self._fn_name = fn.__name__
+        self._fn = run_safely
         self._output_prefix = pool_args[0].output_prefix
         self._nb_repos = len(pool_args)
         self._stop_event = Event()
@@ -95,8 +102,8 @@ def __init__(
             self._task_tracker = TaskTracker()
             self._monitored_fn = MonitoredFunction(self._fn, self._task_tracker)
 
-    def run(self) -> List[Any]:
-        print(f"Running ``{self._fn.__name__}`` with up to {self._n_threads} processes.")
+    def run(self) -> List[Union[None, Exception]]:
+        print(f"Running ``{self._fn_name}`` with up to {self._n_threads} processes.")
         if self._verbose:
             with ThreadPoolExecutor(max_workers=self._n_threads) as pool:
                 results = list(pool.map(self._fn, self._pool_args, timeout=1800))
@@ -131,7 +138,9 @@ def _monitor(self):
         sys.stdout.flush()
 
     @staticmethod
-    def check_results(results, operation: str) -> int:
+    def check_results(
+        results: Optional[List[Union[GitException, Exception, Any]]], operation: str
+    ) -> int:
         """Check the results of ParallelRunner and print the failures."""
 
         fail_count = 0
@@ -143,11 +152,8 @@ def check_results(results, operation: str) -> int:
             if fail_count == 0:
                 print(f"======{operation} FAILURES======")
             fail_count += 1
-            if isinstance(r, str):
+            if isinstance(r, (GitException, Exception)):
                 print(r)
                 continue
-            if isinstance(r, GitException):
-                print(str(r))
-                continue
             print(r)
         return fail_count

utils/update_checkout/update_checkout/runner_arguments.py

@@ -1,5 +1,5 @@
 from dataclasses import dataclass
-from typing import Any, Dict, List
+from typing import Any, Dict, List, Optional
 
 from .cli_arguments import CliArguments
 
@@ -15,12 +15,12 @@ class UpdateArguments(RunnerArguments):
     source_root: str
     config: Dict[str, Any]
     scheme_map: Any
-    tag: str
+    tag: Optional[str]
     timestamp: Any
     reset_to_remote: bool
     clean: bool
     stash: bool
-    cross_repos_pr: bool
+    cross_repos_pr: Dict[str, str]
 
 @dataclass
 class AdditionalSwiftSourcesArguments(RunnerArguments):

utils/update_checkout/update_checkout/update_checkout.py

@@ -15,10 +15,10 @@
 import sys
 import traceback
 from multiprocessing import freeze_support
-from typing import Any, Dict, Optional, Set, List, Union
+from typing import Any, Dict, Hashable, Optional, Set, List, Union
 
 from .cli_arguments import CliArguments
-from .git_command import Git
+from .git_command import Git, GitException
 from .runner_arguments import AdditionalSwiftSourcesArguments, UpdateArguments
 from .parallel_runner import ParallelRunner
 
@@ -74,7 +74,7 @@ def get_branch_for_repo(
     config: Dict[str, Any],
     repo_name: str,
     scheme_name: str,
-    scheme_map: Dict[str, str],
+    scheme_map: Optional[Dict[str, str]],
     cross_repos_pr: Dict[str, str],
 ):
     """Infer, fetch, and return a branch corresponding to a given PR, otherwise
@@ -85,7 +85,7 @@ def get_branch_for_repo(
         config (Dict[str, Any]): deserialized `update-checkout-config.json`
         repo_name (str): name of the repository for checking out the branch
         scheme_name (str): name of the scheme to look up in the config
-        scheme_map (Dict[str, str]): map of repo names to branches to check out
+        scheme_map (Dict[str, str] | None): map of repo names to branches to check out
         cross_repos_pr (Dict[str, str]): map of repo ids to PRs to check out
 
     Returns:
@@ -239,8 +239,8 @@ def run_for_repo_and_each_submodule_rec(args: List[str]):
             # Otherwise there was some other error, and we need to handle
             # it like other command errors.
             Git.run(repo_path, ["symbolic-ref", "-q", "HEAD"])
-        except Exception as e:
-            if e.ret == 1:
+        except GitException as e:
+            if e.returncode == 1:
                 detached_head = True
             else:
                 raise  # Pass this error up the chain.
@@ -268,19 +268,17 @@ def run_for_repo_and_each_submodule_rec(args: List[str]):
             prefix=prefix,
         )
     except Exception:
-        (type, value, tb) = sys.exc_info()
         if verbose:
             print('Error on repo "%s": %s' % (repo_path, traceback.format_exc()))
-        return value
+        raise
 
 
-def get_timestamp_to_match(match_timestamp, source_root):
-    # type: (str | None, str) -> str | None
+def get_timestamp_to_match(match_timestamp: bool, source_root: str):
     """Computes a timestamp of the last commit on the current branch in
     the `swift` repository.
 
     Args:
-        match_timestamp (str | None): value of `--match-timestamp` to check.
+        match_timestamp (bool): value of `--match-timestamp` to check.
         source_root (str): directory that contains sources of the Swift project.
 
     Returns:
@@ -295,7 +293,7 @@ def get_timestamp_to_match(match_timestamp, source_root):
     return output
 
 
-def get_scheme_map(config: Dict[str, Any], scheme_name: str):
+def get_scheme_map(config: Dict[str, Any], scheme_name: str) -> Optional[Dict[str, str]]:
     """Find a mapping from repository IDs to branches in the config.
 
     Args:
@@ -342,7 +340,7 @@ def _is_any_repository_locked(pool_args: List[UpdateArguments]) -> Set[str]:
                 locked_repositories.add(repo_name)
     return locked_repositories
 
-def _move_llvm_project_to_first_index(pool_args: List[Union[UpdateArguments, AdditionalSwiftSourcesArguments]]):
+def _move_llvm_project_to_first_index(pool_args: Union[List[UpdateArguments], List[AdditionalSwiftSourcesArguments]]):
     llvm_project_idx = None
     for i in range(len(pool_args)):
         if pool_args[i].repo_name == "llvm-project":
@@ -351,7 +349,13 @@ def _move_llvm_project_to_first_index(pool_args: List[Union[UpdateArguments, Add
     if llvm_project_idx is not None:
         pool_args.insert(0, pool_args.pop(llvm_project_idx))
 
-def update_all_repositories(args: CliArguments, config, scheme_name, scheme_map, cross_repos_pr):
+def update_all_repositories(
+    args: CliArguments,
+    config: Dict[str, Any],
+    scheme_name: str,
+    scheme_map: Optional[Dict[str, Any]],
+    cross_repos_pr: Dict[str, str],
+):
     pool_args: List[UpdateArguments] = []
     timestamp = get_timestamp_to_match(args.match_timestamp, args.source_root)
     for repo_name in config['repos'].keys():
@@ -392,7 +396,7 @@ def update_all_repositories(args: CliArguments, config, scheme_name, scheme_map,
     locked_repositories: set[str] = _is_any_repository_locked(pool_args)
     if len(locked_repositories) > 0:
         return [
-            f"'{repo_name}' is locked by git. Cannot update it."
+            Exception(f"'{repo_name}' is locked by git. Cannot update it.")
             for repo_name in locked_repositories
         ]
     _move_llvm_project_to_first_index(pool_args)
@@ -484,7 +488,7 @@ def obtain_all_additional_swift_sources(
             else:
                 remote = config['https-clone-pattern'] % remote_repo_id
 
-        repo_branch = None
+        repo_branch: Optional[str] = None
         repo_not_in_scheme = False
         if scheme_name:
             for v in config['branch-schemes'].values():
@@ -500,6 +504,9 @@ def obtain_all_additional_swift_sources(
                 repo_branch = scheme_name
         if repo_not_in_scheme:
             continue
+        
+        if repo_branch is None:
+            raise RuntimeError("repo_branch is None")
 
         new_args = AdditionalSwiftSourcesArguments(
             args=args,
@@ -570,7 +577,7 @@ def print_repo_hashes(args: CliArguments, config: Dict[str, Any]):
         print("{:<35}: {:<35}".format(repo_name, repo_hash))
 
 
-def merge_no_duplicates(a: dict, b: dict) -> dict:
+def merge_no_duplicates(a: Dict[Hashable, Any], b: Dict[Hashable, Any]) -> Dict[Hashable, Any]:
     result = {**a}
     for key, value in b.items():
         if key in a:
@@ -580,7 +587,7 @@ def merge_no_duplicates(a: dict, b: dict) -> dict:
     return result
 
 
-def merge_config(config: dict, new_config: dict) -> dict:
+def merge_config(config: Dict[str, Any], new_config: Dict[str, Any]) -> Dict[str, Any]:
     """
     Merge two configs, with a 'last-wins' strategy.
 
@@ -619,7 +626,7 @@ def validate_config(config: Dict[str, Any]):
                                'too.'.format(scheme_name))
 
     # Then make sure the alias names used by our branches are unique.
-    seen = dict()
+    seen: Dict[str, Any] = dict()
     for (scheme_name, scheme) in config['branch-schemes'].items():
         aliases = scheme['aliases']
         for alias in aliases:
@@ -631,7 +638,7 @@ def validate_config(config: Dict[str, Any]):
                 seen[alias] = scheme_name
 
 
-def full_target_name(repo_path, repository, target):
+def full_target_name(repo_path: str, repository: str, target: str) -> str:
     tag, _, _ = Git.run(repo_path, ["tag", "-l", target], fatal=True)
     if tag == target:
         return tag
@@ -645,13 +652,13 @@ def full_target_name(repo_path, repository, target):
     raise RuntimeError('Cannot determine if %s is a branch or a tag' % target)
 
 
-def skip_list_for_platform(config: Dict[str, Any], all_repos: List[str]) -> List[str]:
+def skip_list_for_platform(config: Dict[str, Any], all_repos: bool) -> List[str]:
     """Computes a list of repositories to skip when updating or cloning, if not
     overridden by `--all-repositories` CLI argument.
 
     Args:
         config (Dict[str, Any]): deserialized `update-checkout-config.json`
-        all_repos (List[str]): repositories not required for current platform.
+        all_repos (bool): include all repositories.
 
     Returns:
         List[str]: a resulting list of repositories to skip or empty list if
@@ -677,7 +684,7 @@ def skip_list_for_platform(config: Dict[str, Any], all_repos: List[str]) -> List
     return skip_list
 
 
-def main():
+def main() -> int:
     freeze_support()
     args = CliArguments.parse_args()
 
@@ -704,7 +711,7 @@ def main():
             config = merge_config(config, json.load(f))
     validate_config(config)
 
-    cross_repos_pr = {}
+    cross_repos_pr: Dict[str, str] = {}
     if args.github_comment:
         regex_pr = r'(apple/[-a-zA-Z0-9_]+/pull/\d+'\
             r'|apple/[-a-zA-Z0-9_]+#\d+'\
@@ -755,11 +762,11 @@ def main():
 
     if args.dump_hashes:
         dump_repo_hashes(args, config)
-        return (None, None)
+        return 0
 
     if args.dump_hashes_config:
         dump_repo_hashes(args, config, args.dump_hashes_config)
-        return (None, None)
+        return 0
 
     # Quick check whether somebody is calling update in an empty directory
     directory_contents = os.listdir(args.source_root)