From a94f6a0a9bf87525997b18fbac8485b77af27b15 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Wed, 15 Jun 2022 15:15:02 -0700 Subject: [PATCH 1/5] shell_tools.output_of - use `run` to execute the command Implement using shell_tools.run instead of run_cmd slated for removal. --- dev_tools/shell_tools.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dev_tools/shell_tools.py b/dev_tools/shell_tools.py index 440bec87dc9..b1badc8c124 100644 --- a/dev_tools/shell_tools.py +++ b/dev_tools/shell_tools.py @@ -290,19 +290,19 @@ def output_of(*cmd: Optional[str], **kwargs) -> str: Args: *cmd: Components of the command to execute, e.g. ["echo", "dog"]. - **kwargs: Extra arguments for asyncio.create_subprocess_shell, such as + Arguments set to None are disregarded in command execution. + + **kwargs: Extra arguments for the shell_tools.run function, such as a cwd (current working directory) argument. Returns: - A (captured output, captured error output, return code) triplet. The - captured outputs will be None if the out or err parameters were not set - to an instance of TeeCapture. + str: The standard output of the command with the last newline removed. Raises: subprocess.CalledProcessError: The process returned a non-zero error - code and raise_on_fail was set. + code and the `check` flag was True (default). """ - result = cast(str, run_cmd(*cmd, log_run_to_stderr=False, out=TeeCapture(), **kwargs).out) + result = run(cmd, log_run_to_stderr=False, stdout=subprocess.PIPE, **kwargs).stdout # Strip final newline. if result.endswith('\n'): From 263c90fa6c530237d2e7b39637a550b0c4e45ae8 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 16 Jun 2022 15:09:31 -0700 Subject: [PATCH 2/5] output_of - drop any arguments set to None --- dev_tools/shell_tools.py | 3 ++- dev_tools/shell_tools_test.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/dev_tools/shell_tools.py b/dev_tools/shell_tools.py index b1badc8c124..03aeb962de1 100644 --- a/dev_tools/shell_tools.py +++ b/dev_tools/shell_tools.py @@ -302,7 +302,8 @@ def output_of(*cmd: Optional[str], **kwargs) -> str: subprocess.CalledProcessError: The process returned a non-zero error code and the `check` flag was True (default). """ - result = run(cmd, log_run_to_stderr=False, stdout=subprocess.PIPE, **kwargs).stdout + cmdpure = [w for w in cmd if w is not None] + result = run(cmdpure, log_run_to_stderr=False, stdout=subprocess.PIPE, **kwargs).stdout # Strip final newline. if result.endswith('\n'): diff --git a/dev_tools/shell_tools_test.py b/dev_tools/shell_tools_test.py index 344ebd085fc..0cc314aabf8 100644 --- a/dev_tools/shell_tools_test.py +++ b/dev_tools/shell_tools_test.py @@ -111,4 +111,5 @@ def test_output_of(): with pytest.raises(subprocess.CalledProcessError): _ = shell_tools.output_of('false') assert shell_tools.output_of('echo', 'test') == 'test' + assert shell_tools.output_of('echo', 'test', None, 'duck') == 'test duck' assert shell_tools.output_of('pwd', cwd='/tmp') in ['/tmp', '/private/tmp'] From ad3e8351bd8cce18b7f3a92def26513c360b00f6 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 16 Jun 2022 16:43:01 -0700 Subject: [PATCH 3/5] docstring nitpicking - remove extra blank line --- dev_tools/shell_tools.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dev_tools/shell_tools.py b/dev_tools/shell_tools.py index 03aeb962de1..d4a2ab0fa96 100644 --- a/dev_tools/shell_tools.py +++ b/dev_tools/shell_tools.py @@ -291,7 +291,6 @@ def output_of(*cmd: Optional[str], **kwargs) -> str: Args: *cmd: Components of the command to execute, e.g. ["echo", "dog"]. Arguments set to None are disregarded in command execution. - **kwargs: Extra arguments for the shell_tools.run function, such as a cwd (current working directory) argument. From d0b0663e48108a8e698b5560f415e0e95f66a0bc Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Fri, 17 Jun 2022 10:01:17 -0700 Subject: [PATCH 4/5] output_of - unify argument interface with run() Pass command arguments to shell_tools.run unmodified. Remove filtering of the None items in command argument list; leave this up to the caller. --- dev_tools/shell_tools.py | 12 +++++++----- dev_tools/shell_tools_test.py | 6 ++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/dev_tools/shell_tools.py b/dev_tools/shell_tools.py index d4a2ab0fa96..db932b7fb4f 100644 --- a/dev_tools/shell_tools.py +++ b/dev_tools/shell_tools.py @@ -285,12 +285,15 @@ def run_shell( return result -def output_of(*cmd: Optional[str], **kwargs) -> str: +def output_of(args: Union[str, List[str]], **kwargs) -> str: """Invokes a subprocess and returns its output as a string. Args: - *cmd: Components of the command to execute, e.g. ["echo", "dog"]. - Arguments set to None are disregarded in command execution. + args: The arguments for launching the process. This may be a list + or a string, for example, ["echo", "dog"] or "pwd". The string + type may need to be used with ``shell=True`` to allow invocation + as a shell command, otherwise the string is used as a command name + with no arguments. **kwargs: Extra arguments for the shell_tools.run function, such as a cwd (current working directory) argument. @@ -301,8 +304,7 @@ def output_of(*cmd: Optional[str], **kwargs) -> str: subprocess.CalledProcessError: The process returned a non-zero error code and the `check` flag was True (default). """ - cmdpure = [w for w in cmd if w is not None] - result = run(cmdpure, log_run_to_stderr=False, stdout=subprocess.PIPE, **kwargs).stdout + result = run(args, log_run_to_stderr=False, stdout=subprocess.PIPE, **kwargs).stdout # Strip final newline. if result.endswith('\n'): diff --git a/dev_tools/shell_tools_test.py b/dev_tools/shell_tools_test.py index 0cc314aabf8..6c6f20ae470 100644 --- a/dev_tools/shell_tools_test.py +++ b/dev_tools/shell_tools_test.py @@ -110,6 +110,8 @@ def test_output_of(): assert shell_tools.output_of('true') == '' with pytest.raises(subprocess.CalledProcessError): _ = shell_tools.output_of('false') - assert shell_tools.output_of('echo', 'test') == 'test' - assert shell_tools.output_of('echo', 'test', None, 'duck') == 'test duck' + assert shell_tools.output_of(['echo', 'test']) == 'test' + # filtering of the None arguments was removed. check this now fails + with pytest.raises(TypeError): + _ = shell_tools.output_of(['echo', 'test', None, 'duck']) assert shell_tools.output_of('pwd', cwd='/tmp') in ['/tmp', '/private/tmp'] From ab24692e04538f7291ef09cfde14805eb945045f Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Fri, 17 Jun 2022 10:13:31 -0700 Subject: [PATCH 5/5] Update callers of shell_tools.output_of output_of requires command-line arguments in a list. Also do the filtering of an optional None items before the call. --- dev_tools/git_env_tools.py | 10 +++++----- dev_tools/incremental_coverage.py | 3 ++- dev_tools/prepared_env.py | 15 +++++++++------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/dev_tools/git_env_tools.py b/dev_tools/git_env_tools.py index 8ff4ba7f914..f4c0e1efecd 100644 --- a/dev_tools/git_env_tools.py +++ b/dev_tools/git_env_tools.py @@ -22,7 +22,7 @@ def get_repo_root() -> str: """Get the root of the git repository the cwd is within.""" - return shell_tools.output_of('git', 'rev-parse', '--show-toplevel') + return shell_tools.output_of(['git', 'rev-parse', '--show-toplevel']) def _git_fetch_for_comparison( @@ -60,7 +60,7 @@ def _git_fetch_for_comparison( depth_str, log_run_to_stderr=verbose, ) - actual_id = shell_tools.output_of('git', 'rev-parse', 'FETCH_HEAD') + actual_id = shell_tools.output_of(['git', 'rev-parse', 'FETCH_HEAD']) shell_tools.run_cmd( 'git', @@ -71,10 +71,10 @@ def _git_fetch_for_comparison( depth_str, log_run_to_stderr=verbose, ) - base_id = shell_tools.output_of('git', 'rev-parse', 'FETCH_HEAD') + base_id = shell_tools.output_of(['git', 'rev-parse', 'FETCH_HEAD']) try: - base_id = shell_tools.output_of('git', 'merge-base', actual_id, base_id) + base_id = shell_tools.output_of(['git', 'merge-base', actual_id, base_id]) break except subprocess.CalledProcessError: # No common ancestor. We need to dig deeper. @@ -170,7 +170,7 @@ def fetch_local_files(destination_directory: str, verbose: bool) -> prepared_env log_run_to_stderr=verbose, ) - cur_commit = shell_tools.output_of('git', 'rev-parse', 'HEAD') + cur_commit = shell_tools.output_of(['git', 'rev-parse', 'HEAD']) os.chdir(destination_directory) if verbose: diff --git a/dev_tools/incremental_coverage.py b/dev_tools/incremental_coverage.py index e0b9b42806d..a35b4d6459f 100644 --- a/dev_tools/incremental_coverage.py +++ b/dev_tools/incremental_coverage.py @@ -144,8 +144,9 @@ def get_incremental_uncovered_lines( if not os.path.isfile(abs_path): return [] + optional_actual_commit = [] if actual_commit is None else [actual_commit] unified_diff_lines_str = shell_tools.output_of( - 'git', 'diff', '--unified=0', base_commit, actual_commit, '--', abs_path + ['git', 'diff', '--unified=0', base_commit, *optional_actual_commit, '--', abs_path] ) unified_diff_lines = [e for e in unified_diff_lines_str.split('\n') if e.strip()] diff --git a/dev_tools/prepared_env.py b/dev_tools/prepared_env.py index aea65b34438..50cf66acee1 100644 --- a/dev_tools/prepared_env.py +++ b/dev_tools/prepared_env.py @@ -121,13 +121,16 @@ def get_changed_files(self) -> List[str]: List[str]: File paths of changed files, relative to the git repo root. """ + optional_actual_commit_id = [] if self.actual_commit_id is None else [self.actual_commit_id] out = shell_tools.output_of( - 'git', - 'diff', - '--name-only', - self.compare_commit_id, - self.actual_commit_id, - '--', + [ + 'git', + 'diff', + '--name-only', + self.compare_commit_id, + *optional_actual_commit_id, + '--', + ], cwd=self.destination_directory, ) return [e for e in out.split('\n') if e.strip()]