From 53cb4f7935e858712edea7b2b09ad8b1c75aacd0 Mon Sep 17 00:00:00 2001 From: Leo Neat Date: Tue, 4 Feb 2020 11:51:18 -0800 Subject: [PATCH] [Infra] Update execute function in utils.py (#3319) --- infra/build_specified_commit.py | 2 +- infra/repo_manager.py | 16 +++++------ infra/utils.py | 23 ++++++++++------ infra/utils_test.py | 47 +++++++++++++++++++++++++++++---- 4 files changed, 66 insertions(+), 22 deletions(-) diff --git a/infra/build_specified_commit.py b/infra/build_specified_commit.py index 823fd488d..13965f141 100644 --- a/infra/build_specified_commit.py +++ b/infra/build_specified_commit.py @@ -88,7 +88,7 @@ def detect_main_repo(project_name, repo_name=None, commit=None): command_to_run.extend(['--repo_name', repo_name]) else: command_to_run.extend(['--example_commit', commit]) - out, _ = utils.execute(command_to_run) + out, _, _ = utils.execute(command_to_run) match = re.search(r'\bDetected repo: ([^ ]+) ([^ ]+)', out.rstrip()) if match and match.group(1) and match.group(2): return match.group(1), match.group(2) diff --git a/infra/repo_manager.py b/infra/repo_manager.py index cf98ab944..53ac2df48 100644 --- a/infra/repo_manager.py +++ b/infra/repo_manager.py @@ -63,8 +63,8 @@ class RepoManager: if not os.path.exists(self.base_dir): os.makedirs(self.base_dir) self.remove_repo() - out, err = utils.execute(['git', 'clone', self.repo_url, self.repo_name], - location=self.base_dir) + out, _, _ = utils.execute(['git', 'clone', self.repo_url, self.repo_name], + location=self.base_dir) if not self._is_git_repo(): raise ValueError('%s is not a git repo' % self.repo_url) @@ -89,8 +89,8 @@ class RepoManager: if not commit.rstrip(): return False - _, err_code = utils.execute(['git', 'cat-file', '-e', commit], - self.repo_dir) + _, _, err_code = utils.execute(['git', 'cat-file', '-e', commit], + self.repo_dir) return not err_code def get_current_commit(self): @@ -99,9 +99,9 @@ class RepoManager: Returns: The current active commit SHA. """ - out, _ = utils.execute(['git', 'rev-parse', 'HEAD'], - self.repo_dir, - check_result=True) + out, _, _ = utils.execute(['git', 'rev-parse', 'HEAD'], + self.repo_dir, + check_result=True) return out.strip('\n') def get_commit_list(self, old_commit, new_commit): @@ -125,7 +125,7 @@ class RepoManager: raise ValueError('The new commit %s does not exist' % new_commit) if old_commit == new_commit: return [old_commit] - out, err_code = utils.execute( + out, _, err_code = utils.execute( ['git', 'rev-list', old_commit + '..' + new_commit], self.repo_dir) commits = out.split('\n') commits = [commit for commit in commits if commit] diff --git a/infra/utils.py b/infra/utils.py index 8f0e4e287..468438f4f 100644 --- a/infra/utils.py +++ b/infra/utils.py @@ -13,6 +13,7 @@ # limitations under the License. """Utilities for OSS-Fuzz infrastructure.""" +import logging import os import re import stat @@ -41,7 +42,7 @@ def execute(command, location=None, check_result=False): check_result: Should an exception be thrown on failed command. Returns: - The stdout of the command, the error code. + stdout, stderr, error code. Raises: RuntimeError: running a command resulted in an error. @@ -49,14 +50,20 @@ def execute(command, location=None, check_result=False): if not location: location = os.getcwd() - process = subprocess.Popen(command, stdout=subprocess.PIPE, cwd=location) + process = subprocess.Popen(command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + cwd=location) out, err = process.communicate() - if check_result and (process.returncode or err): - raise RuntimeError('Error: %s\n Command: %s\n Return code: %s\n Out: %s' % - (err, command, process.returncode, out)) - if out is not None: - out = out.decode('ascii').rstrip() - return out, process.returncode + out = out.decode('ascii') + err = err.decode('ascii') + if err: + logging.debug('Stderr of command \'%s\' is %s.', ' '.join(command), err) + if check_result and process.returncode: + raise RuntimeError( + 'Executing command \'{0}\' failed with error: {1}.'.format( + ' '.join(command), err)) + return out, err, process.returncode def get_fuzz_targets(path): diff --git a/infra/utils_test.py b/infra/utils_test.py index ab3d216fb..5898537c7 100644 --- a/infra/utils_test.py +++ b/infra/utils_test.py @@ -11,13 +11,10 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Test the functionality of the utils module's functions: -1. is_fuzz_target_local -2. get_fuzz_targets -3. get_env_var -""" +"""Test the functionality of the utils module's functions""" import os +import tempfile import unittest import utils @@ -98,5 +95,45 @@ class GetFuzzTargetsUnitTest(unittest.TestCase): self.assertFalse(fuzz_targets) +class ExecuteUnitTest(unittest.TestCase): + """Test execute function in the utils module.""" + + def test_valid_command(self): + """Tests that execute can produce valid output.""" + with tempfile.TemporaryDirectory() as tmp_dir: + out, err, err_code = utils.execute(['ls', '.'], + location=tmp_dir, + check_result=False) + self.assertEqual(err_code, 0) + self.assertEqual(err, '') + self.assertEqual(out, '') + out, err, err_code = utils.execute(['mkdir', 'tmp'], + location=tmp_dir, + check_result=False) + self.assertEqual(err_code, 0) + self.assertEqual(err, '') + self.assertEqual(out, '') + out, err, err_code = utils.execute(['ls', '.'], + location=tmp_dir, + check_result=False) + self.assertEqual(err_code, 0) + self.assertEqual(err, '') + self.assertEqual(out, 'tmp\n') + + def test_error_command(self): + """Tests that execute can correctly surface errors.""" + with tempfile.TemporaryDirectory() as tmp_dir: + out, err, err_code = utils.execute(['ls', 'notarealdir'], + location=tmp_dir, + check_result=False) + self.assertEqual(err_code, 2) + self.assertIsNotNone(err) + self.assertEqual(out, '') + with self.assertRaises(RuntimeError): + out, err, err_code = utils.execute(['ls', 'notarealdir'], + location=tmp_dir, + check_result=True) + + if __name__ == '__main__': unittest.main()