From f09449467bc86f6f90dd9aee727bc2a9258d5e40 Mon Sep 17 00:00:00 2001 From: Charles Zablit Date: Tue, 21 Oct 2025 17:50:30 -0700 Subject: [PATCH] [update-checkout] refactor shell invocations --- .../swift_build_support/shell.py | 47 ------- utils/update_checkout/tests/test_clone.py | 5 +- .../update_checkout/git_command.py | 115 +++++++++++++----- .../update_checkout/parallel_runner.py | 2 +- .../update_checkout/update_checkout.py | 46 +++---- 5 files changed, 106 insertions(+), 109 deletions(-) diff --git a/utils/swift_build_support/swift_build_support/shell.py b/utils/swift_build_support/swift_build_support/shell.py index 2045f4c5a6cd3..719acf73f92d2 100644 --- a/utils/swift_build_support/swift_build_support/shell.py +++ b/utils/swift_build_support/swift_build_support/shell.py @@ -209,50 +209,3 @@ def remove(path, dry_run=None, echo=True): if dry_run: return os.remove(path) - - -# Initialized later -lock = None - - -def run(*args, **kwargs): - repo_path = kwargs.pop('repo_path', os.getcwd()) - echo_output = kwargs.pop('echo', False) - dry_run = kwargs.pop('dry_run', False) - env = kwargs.get('env', None) - prefix = kwargs.pop('prefix', '') - if dry_run: - _echo_command(dry_run, *args, env=env, prompt="{0}+ ".format(prefix)) - return (None, 0, args) - - my_pipe = subprocess.Popen( - *args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, - universal_newlines=True, - encoding='utf-8', - **kwargs) - (output, _) = my_pipe.communicate() - output = output.encode(encoding='ascii', errors='replace') - ret = my_pipe.wait() - - if lock: - lock.acquire() - if echo_output: - sys.stdout.flush() - sys.stderr.flush() - _echo_command(dry_run, *args, env=env, prompt="{0}+ ".format(prefix)) - if output: - for line in output.splitlines(): - print("{0}{1}".format(prefix, line.decode('utf-8', errors='replace'))) - sys.stdout.flush() - sys.stderr.flush() - if lock: - lock.release() - - if ret != 0: - eout = Exception(f"[{repo_path}] '{args}' failed with '{output.decode('utf-8')}'") - eout.ret = ret - eout.arguments = args - eout.repo_path = repo_path - eout.stderr = output - raise eout - return (output, 0, args) diff --git a/utils/update_checkout/tests/test_clone.py b/utils/update_checkout/tests/test_clone.py index 27da77f035135..7799be778615e 100644 --- a/utils/update_checkout/tests/test_clone.py +++ b/utils/update_checkout/tests/test_clone.py @@ -41,10 +41,7 @@ def test_clone_with_additional_scheme(self): '--verbose']) # Test that we're actually checking out the 'extra' scheme based on the output - self.assertIn( - f"git -C {os.path.join(self.source_root, 'repo1')} checkout refs/heads/main", - output.decode("utf-8"), - ) + self.assertIn("git checkout refs/heads/main", output.decode("utf-8")) def test_manager_not_called_on_long_socket(self): fake_tmpdir = '/tmp/very/' + '/long' * 20 + '/tmp' diff --git a/utils/update_checkout/update_checkout/git_command.py b/utils/update_checkout/update_checkout/git_command.py index 33906f5b4a41a..e6c09fb9cdf0d 100644 --- a/utils/update_checkout/update_checkout/git_command.py +++ b/utils/update_checkout/update_checkout/git_command.py @@ -1,37 +1,90 @@ -from typing import List, Any, Optional, Union - -from swift_build_support.swift_build_support import shell +import shlex +import subprocess +import sys +from typing import List, Any, Optional, Dict class Git: @staticmethod - def run(repo_path: Optional[str], args: List[str], **kwargs): - cmd = ["git"] - if repo_path is not None: - cmd += ["-C", repo_path] - kwargs["repo_path"] = repo_path - # FIXME: The way we are passing args below is broken. shell.run takes - # *args as list arguments and sometimes treats them as one positional - # argument or as a list of arguments. - return shell.run(cmd+args, **kwargs) - - @staticmethod - def capture( + def run( repo_path: str, args: List[str], - stderr=None, - env=None, - dry_run=None, - echo=True, - optional=False, - allow_non_zero_exit=False, - ) -> Union[str, Any, None]: - return shell.capture( - ["git", "-C", repo_path] + args, - stderr=stderr, - env=env, - dry_run=dry_run, - echo=echo, - optional=optional, - allow_non_zero_exit=allow_non_zero_exit, - ) + echo: bool = False, + env: Optional[Dict[str, Any]] = None, + prefix: str = "", + allow_non_zero_exit: bool = False, + fatal: bool = False, + **kwargs, + ): + command = Git._build_command(args) + + try: + result = subprocess.run( + command, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + universal_newlines=True, + encoding="utf-8", + env=env, + cwd=repo_path, + **kwargs, + ) + output = result.stdout + if echo: + Git._echo_command(command, output, env, prefix) + if not allow_non_zero_exit: + result.check_returncode() + except subprocess.CalledProcessError as e: + if fatal: + sys.exit( + f"command `{command}` terminated with a non-zero exit " + f"status {str(e.returncode)}, aborting") + eout = Exception( + f"[{repo_path}] '{Git._quote_command(command)}' failed with '{output}'" + ) + eout.ret = e.returncode + eout.arguments = command + eout.repo_path = repo_path + eout.stderr = output + raise eout + except OSError as e: + if fatal: + sys.exit( + f"could not execute '{Git._quote_command(command)}': " + f"{e.strerror}" + ) + return (output.strip(), result.returncode, command) + + @staticmethod + def _echo_command( + command: List[str], + output: Optional[str] = None, + env: Optional[Dict[str, Any]] = None, + prefix: str = "", + ): + sys.stdout.flush() + sys.stderr.flush() + command_str = [] + if env is not None: + command_str += ["env"] + [ + Git._quote(f"{k}={v}") for (k, v) in sorted(env.items()) + ] + command_str.append(Git._quote_command(command)) + print(f"{prefix}+ {' '.join(command_str)}", file=sys.stderr) + if output: + for line in output.splitlines(): + print(prefix+line) + sys.stdout.flush() + sys.stderr.flush() + + @staticmethod + def _build_command(args: List[str]) -> List[str]: + return ["git"] + args + + @staticmethod + def _quote(arg: Any) -> str: + return shlex.quote(str(arg)) + + @staticmethod + def _quote_command(command: Any) -> str: + return " ".join(Git._quote(arg) for arg in command) diff --git a/utils/update_checkout/update_checkout/parallel_runner.py b/utils/update_checkout/update_checkout/parallel_runner.py index 78b7631aeb2c6..d5bcbd953ca38 100644 --- a/utils/update_checkout/update_checkout/parallel_runner.py +++ b/utils/update_checkout/update_checkout/parallel_runner.py @@ -31,7 +31,7 @@ def mark_task_as_done(self, task_name: str): self._done_task_counter += 1 self._lock.release() - def status(self) -> Tuple[List[str], int]: + def status(self) -> Tuple[str, int]: self._lock.acquire() running_tasks_str = ", ".join(self.running_tasks) done_tasks = self.done_task_counter diff --git a/utils/update_checkout/update_checkout/update_checkout.py b/utils/update_checkout/update_checkout/update_checkout.py index f3bfadf11c81d..c5594c55f8f8f 100755 --- a/utils/update_checkout/update_checkout/update_checkout.py +++ b/utils/update_checkout/update_checkout/update_checkout.py @@ -42,13 +42,13 @@ def confirm_tag_in_repo(repo_path: str, tag: str, repo_name: str) -> Optional[st exist. """ - tag_exists = Git.capture( - repo_path, ["ls-remote", "--tags", "origin", tag], echo=False + tag_exists, _, _ = Git.run( + repo_path, ["ls-remote", "--tags", "origin", tag], fatal=True ) if not tag_exists: print("Tag '" + tag + "' does not exist for '" + repo_name + "', just updating regularly") - tag = None + return None return tag @@ -61,7 +61,7 @@ def find_rev_by_timestamp(repo_path: str, timestamp: str, repo_name: str, refspe args = ["log", "-1", "--format=%H", "--first-parent", "--before=" + timestamp] if refspec_exists: args.append(refspec) - rev = Git.capture(repo_path, args).strip() + rev, _, _ = Git.run(repo_path, args, fatal=True) if rev: return rev else: @@ -98,7 +98,7 @@ def get_branch_for_repo(repo_path, config, repo_name, scheme_name, scheme_map, pr_id = cross_repos_pr[remote_repo_id] repo_branch = "ci_pr_{0}".format(pr_id) Git.run(repo_path, ["checkout", scheme_branch], echo=True) - Git.capture(repo_path, ["branch", "-D", repo_branch], echo=True, allow_non_zero_exit=True) + Git.run(repo_path, ["branch", "-D", repo_branch], echo=True, allow_non_zero_exit=True, fatal=True) Git.run(repo_path, ["fetch", "origin", "pull/{0}/merge:{1}".format(pr_id, repo_branch), "--tags"], echo=True) return repo_branch, cross_repo @@ -172,7 +172,7 @@ def run_for_repo_and_each_submodule_rec(args: List[str]): pass if checkout_target: - Git.run(repo_path, ['status', '--porcelain', '-uno'], echo=False) + Git.run(repo_path, ['status', '--porcelain', '-uno']) # Some of the projects switch branches/tags when they # are updated. Local checkout might not have that tag/branch @@ -199,8 +199,7 @@ def run_for_repo_and_each_submodule_rec(args: List[str]): ) except Exception: try: - result = Git.run(repo_path, ["rev-parse", checkout_target]) - revision = result[0].strip() + revision, _, _ = Git.run(repo_path, ["rev-parse", checkout_target]) Git.run( repo_path, ["checkout", revision], echo=verbose, prefix=prefix ) @@ -233,7 +232,7 @@ def run_for_repo_and_each_submodule_rec(args: List[str]): # This git command returns error code 1 if HEAD is detached. # Otherwise there was some other error, and we need to handle # it like other command errors. - Git.run(repo_path, ["symbolic-ref", "-q", "HEAD"], echo=False) + Git.run(repo_path, ["symbolic-ref", "-q", "HEAD"]) except Exception as e: if e.ret == 1: detached_head = True @@ -286,9 +285,8 @@ def get_timestamp_to_match(match_timestamp, source_root): if not match_timestamp: return None swift_repo_path = os.path.join(source_root, "swift") - return Git.capture( - swift_repo_path, ["log", "-1", "--format=%cI"], echo=False - ).strip() + output, _, _ = Git.run(swift_repo_path, ["log", "-1", "--format=%cI"], fatal=True) + return output def get_scheme_map(config, scheme_name): @@ -410,22 +408,19 @@ def obtain_additional_swift_sources(pool_args: AdditionalSwiftSourcesArguments): print("Cloning '" + pool_args.repo_name + "'") if pool_args.skip_history: - Git.run(None, ['clone', '--recursive', '--depth', '1', + Git.run(args.source_root, ['clone', '--recursive', '--depth', '1', '--branch', repo_branch, remote, repo_name] + (['--no-tags'] if skip_tags else []), - cwd=args.source_root, env=env, echo=verbose) elif pool_args.use_submodules: - Git.run(None, ['submodule', 'add', remote, repo_name] + + Git.run(args.source_root, ['submodule', 'add', remote, repo_name] + (['--no-tags'] if skip_tags else []), - cwd=args.source_root, env=env, echo=verbose) else: - Git.run(None, ['clone', '--recursive', remote, repo_name] + + Git.run(args.source_root, ['clone', '--recursive', remote, repo_name] + (['--no-tags'] if skip_tags else []), - cwd=args.source_root, env=env, echo=verbose) @@ -433,12 +428,11 @@ def obtain_additional_swift_sources(pool_args: AdditionalSwiftSourcesArguments): if pool_args.scheme_name: src_path = os.path.join(repo_path, ".git") Git.run( - None, + args.source_root, ["--git-dir", src_path, "--work-tree", repo_path, "checkout", repo_branch], env=env, - echo=False, ) - Git.run(repo_path, ["submodule", "update", "--recursive"], env=env, echo=False) + Git.run(repo_path, ["submodule", "update", "--recursive"], env=env) def obtain_all_additional_swift_sources(args, config, with_ssh, scheme_name, @@ -455,8 +449,7 @@ def obtain_all_additional_swift_sources(args, config, with_ssh, scheme_name, if use_submodules: repo_exists = False - submodules_status = Git.capture(repo_path, ['submodule', 'status'], - echo=False) + submodules_status, _, _ = Git.run(repo_path, ['submodule', 'status'], fatal=True) if submodules_status: for line in submodules_status.splitlines(): if line[0].endswith(repo_name): @@ -557,7 +550,7 @@ def repo_hashes(args, config): for repo_name, _ in sorted(config['repos'].items(), key=lambda x: x[0]): repo_path = os.path.join(args.source_root, repo_name) if os.path.exists(repo_path): - h = Git.capture(repo_path, ["rev-parse", "HEAD"], echo=False).strip() + h, _, _ = Git.run(repo_path, ["rev-parse", "HEAD"], fatal=True) else: h = "skip" repos[repo_name] = str(h) @@ -633,11 +626,12 @@ def validate_config(config: Dict[str, Any]): def full_target_name(repo_path, repository, target): - tag = Git.capture(repo_path, ["tag", "-l", target], echo=False).strip() + tag, _, _ = Git.run(repo_path, ["tag", "-l", target], fatal=True) if tag == target: return tag - branch = Git.capture(repo_path, ["branch", "--list", target], echo=False).strip().replace("* ", "") + branch, _, _ = Git.run(repo_path, ["branch", "--list", target], fatal=True) + branch = branch.replace("* ", "") if branch == target: name = "%s/%s" % (repository, target) return name