diff --git a/presubmit_support.py b/presubmit_support.py index 08bf5ac1a..d80985c1f 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -447,6 +447,8 @@ class InputApi(object): # We carry the canned checks so presubmit scripts can easily use them. self.canned_checks = presubmit_canned_checks + # Temporary files we must manually remove at the end of a run. + self._named_temporary_files = [] # TODO(dpranke): figure out a list of all approved owners for a repo # in order to be able to handle wildcard OWNERS files? @@ -577,6 +579,40 @@ class InputApi(object): raise IOError('Access outside the repository root is denied.') return gclient_utils.FileRead(file_item, mode) + def CreateTemporaryFile(self, **kwargs): + """Returns a named temporary file that must be removed with a call to + RemoveTemporaryFiles(). + + All keyword arguments are forwarded to tempfile.NamedTemporaryFile(), + except for |delete|, which is always set to False. + + Presubmit checks that need to create a temporary file and pass it for + reading should use this function instead of NamedTemporaryFile(), as + Windows fails to open a file that is already open for writing. + + with input_api.CreateTemporaryFile() as f: + f.write('xyz') + f.close() + input_api.subprocess.check_output(['script-that', '--reads-from', + f.name]) + + + Note that callers of CreateTemporaryFile() should not worry about removing + any temporary file; this is done transparently by the presubmit handling + code. + """ + if 'delete' in kwargs: + # Prevent users from passing |delete|; we take care of file deletion + # ourselves and this prevents unintuitive error messages when we pass + # delete=False and 'delete' is also in kwargs. + raise TypeError('CreateTemporaryFile() does not take a "delete" ' + 'argument, file deletion is handled automatically by ' + 'the same presubmit_support code that creates InputApi ' + 'objects.') + temp_file = self.tempfile.NamedTemporaryFile(delete=False, **kwargs) + self._named_temporary_files.append(temp_file.name) + return temp_file + @property def tbr(self): """Returns if a change is TBR'ed.""" @@ -1269,10 +1305,13 @@ class PresubmitExecuter(object): else: function_name = 'CheckChangeOnUpload' if function_name in context: - context['__args'] = (input_api, OutputApi(self.committing)) - logging.debug('Running %s in %s', function_name, presubmit_path) - result = eval(function_name + '(*__args)', context) - logging.debug('Running %s done.', function_name) + try: + context['__args'] = (input_api, OutputApi(self.committing)) + logging.debug('Running %s in %s', function_name, presubmit_path) + result = eval(function_name + '(*__args)', context) + logging.debug('Running %s done.', function_name) + finally: + map(os.remove, input_api._named_temporary_files) if not (isinstance(result, types.TupleType) or isinstance(result, types.ListType)): raise PresubmitFailure( diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index ece0e3039..fbafe3a80 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -41,6 +41,18 @@ presubmit_canned_checks = presubmit.presubmit_canned_checks # pylint: disable=protected-access +class MockTemporaryFile(object): + """Simple mock for files returned by tempfile.NamedTemporaryFile().""" + def __init__(self, name): + self.name = name + + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + class PresubmitTestsBase(SuperMoxTestBase): """Sets up and tears down the mocks but doesn't test anything as-is.""" presubmit_text = """ @@ -551,6 +563,40 @@ class PresubmitUnittest(PresubmitTestsBase): ' return ["foo"]', fake_presubmit) + def testExecPresubmitScriptTemporaryFilesRemoval(self): + self.mox.StubOutWithMock(presubmit.tempfile, 'NamedTemporaryFile') + presubmit.tempfile.NamedTemporaryFile(delete=False).AndReturn( + MockTemporaryFile('baz')) + presubmit.tempfile.NamedTemporaryFile(delete=False).AndReturn( + MockTemporaryFile('quux')) + presubmit.os.remove('baz') + presubmit.os.remove('quux') + self.mox.ReplayAll() + + fake_presubmit = presubmit.os.path.join(self.fake_root_dir, 'PRESUBMIT.py') + executer = presubmit.PresubmitExecuter( + self.fake_change, False, None, False) + + self.assertEqual((), executer.ExecPresubmitScript( + ('def CheckChangeOnUpload(input_api, output_api):\n' + ' if len(input_api._named_temporary_files):\n' + ' return (output_api.PresubmitError("!!"),)\n' + ' return ()\n'), + fake_presubmit + )) + + result = executer.ExecPresubmitScript( + ('def CheckChangeOnUpload(input_api, output_api):\n' + ' with input_api.CreateTemporaryFile():\n' + ' pass\n' + ' with input_api.CreateTemporaryFile():\n' + ' pass\n' + ' return [output_api.PresubmitResult(None, f)\n' + ' for f in input_api._named_temporary_files]\n'), + fake_presubmit + ) + self.assertEqual(['baz', 'quux'], [r._items for r in result]) + def testDoPresubmitChecksNoWarningsOrErrors(self): haspresubmit_path = presubmit.os.path.join( self.fake_root_dir, 'haspresubmit', 'PRESUBMIT.py') @@ -957,6 +1003,7 @@ class InputApiUnittest(PresubmitTestsBase): 'AffectedTextFiles', 'DEFAULT_BLACK_LIST', 'DEFAULT_WHITE_LIST', + 'CreateTemporaryFile', 'FilterSourceFile', 'LocalPaths', 'Command', @@ -1323,6 +1370,32 @@ class InputApiUnittest(PresubmitTestsBase): None, False) input_api.ReadFile(fileobj, 'x') + def testCreateTemporaryFile(self): + input_api = presubmit.InputApi( + self.fake_change, + presubmit_path='foo/path/PRESUBMIT.py', + is_committing=False, rietveld_obj=None, verbose=False) + input_api.tempfile.NamedTemporaryFile = self.mox.CreateMock( + input_api.tempfile.NamedTemporaryFile) + input_api.tempfile.NamedTemporaryFile( + delete=False).AndReturn(MockTemporaryFile('foo')) + input_api.tempfile.NamedTemporaryFile( + delete=False).AndReturn(MockTemporaryFile('bar')) + self.mox.ReplayAll() + + self.assertEqual(0, len(input_api._named_temporary_files)) + with input_api.CreateTemporaryFile(): + self.assertEqual(1, len(input_api._named_temporary_files)) + self.assertEqual(['foo'], input_api._named_temporary_files) + with input_api.CreateTemporaryFile(): + self.assertEqual(2, len(input_api._named_temporary_files)) + self.assertEqual(2, len(input_api._named_temporary_files)) + self.assertEqual(['foo', 'bar'], input_api._named_temporary_files) + + self.assertRaises(TypeError, input_api.CreateTemporaryFile, delete=True) + self.assertRaises(TypeError, input_api.CreateTemporaryFile, delete=False) + self.assertEqual(['foo', 'bar'], input_api._named_temporary_files) + class OutputApiUnittest(PresubmitTestsBase): """Tests presubmit.OutputApi."""