presubmit: Add InputApi.CreateTemporaryFile()

Sometimes, InputApi users need to create temporary files, write to them and
pass them to another process, like this:

  with input_api.tempfile.NamedTemporaryFile() as f:
    f.write('foo')
    input_api.subprocess.check_output(['/path/to/script',
        '--reading-from', f.name])

While this works fine on Unix, on Windows subprocess cannot open and read
the file while we have it open for writing.

To work around this, we now offer a CreateTemporaryFile() that wraps a call
to tempfile.NamedTemporaryFile(delete=False), and we then take care of
removing all files created this way at the end of a presubmit run.

The idea is for users to do something like this:

  with input_api.CreateTemporaryFile() as f:
    f.write('foo')
    f.close()
    input_api.subprocess.check_output(['/path/to/script',
        '--reading-from', f.name])

with the temporary file being removed automatically in a transparent fashion
later.

Bug: 780629
Change-Id: I0d705a5d52928a43f39a51f94a2c48d277bd5ced
Reviewed-on: https://chromium-review.googlesource.com/758637
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Aaron Gable <agable@chromium.org>

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(

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."""