Added TempDir

expect_tests/tempdir.py

+"""Provides an automatically-cleaned temporary directory.
+
+All functions in this module are thread-safe.
+"""
+
+import glob
+import os
+import shutil
+import tempfile
+
+TEMP_PID_FILE = '.expect_tests_pidfile'
+TEMP_SUFFIX = '.expect_tests_temp'
+
+
+class TempDir(object):
+  """Context manager for a temporary directory.
+
+  Use tempfile.TemporaryDirectory instead with Python 3.
+  """
+  def __init__(self):
+    self.temp_dir = None
+
+  def __enter__(self):
+    self.temp_dir = set_temp_dir()
+    return self.temp_dir
+
+  def __exit__(self, _type, _value, _traceback):
+    clear_temp_dir(self.temp_dir)
+
+
+def set_temp_dir(suffix=TEMP_SUFFIX):

[dnj, 2014/09/12 17:31:56]

This function seems to be mixing application-level things (PIDfile, cleanup from
previous state) with basic directory creation. I think you should split the two
up. Make the PIDfile, and previous application/directory cleanup something
that's handled explicitly at the application level during startup (before this
is even invoked), not implicitly during a "set_temp_dir" call.

This also means you don't have to exempt the newly-created directory from the
cleanup list.

This will simplify things both for testing and also conceptually.

[pgervais, 2014/09/12 18:16:02]

I haven't written this code, so I'm just guessing the design decisions here: 

- This is all intermingled because there is no way to get the name of the
temporary directory other than what is done line 40:  os.path.dirname(temp_dir).
It means /creating/ a temp file is required to know the location where it's been
created.

- Cleaning up the temporary directory automatically will prove valuable on bots.
I agree we can do that in an external process but the fewer leftovers we have,
the better.

[dnj, 2014/09/12 23:22:10]

Acknowledged.

+  """Provides a temporary directory for tests.
+
+  This function also takes care of removing old temp directories from dead
+  processes.
+  """
+  temp_dir = tempfile.mkdtemp(suffix)
+
+  # Clean up obsolete directories.
+  for p in glob.glob(os.path.join(os.path.dirname(temp_dir), '*'+suffix)):
+    if p == temp_dir:
+      continue
+
+    pfile = os.path.join(p, TEMP_PID_FILE)
+    if os.path.exists(pfile):
+      with open(pfile, 'rb') as f:

[dnj, 2014/09/12 17:31:57]

I don't see any reason to use the 'b' flag here.

[pgervais, 2014/09/12 18:16:02]

Well, it doesn't hurt either.

[dnj, 2014/09/12 23:22:10]

It sort of implies that binary-level correctness is important *shrug*

+        try:
+          os.kill(int(f.read()), 0)

[dnj, 2014/09/12 17:31:57]

What's kill(..., 0) do? I'm used to killing w/ a signal. This also won't work on
Windows, will it?

I'm not sure I like this previous cleanup thing at all. Is it really
unreasonable to allow multiple 'expect_tests' to run at the same time? If not,
maybe just let them clean themselves up and leave it to the user to cleanup any
unlikely leakage, like other UNIX programs tend to do.

[pgervais, 2014/09/12 18:16:02]

Sure. Having multiple expect_tests running at the same time is useful. I guess
it's here for the same reason as the cleanup: removing stuck processes.

[pgervais, 2014/09/12 18:19:17]

Hm. os.kill(pid, 0) does nothing to the process, but raises an exception if it
doesn't exists. So it's just checking if the process is running.

[dnj, 2014/09/12 23:22:10]

Ah neat trick.

+          continue
+        except (OSError, TypeError):
+          pass
+
+    shutil.rmtree(p, ignore_errors=True)
+
+  # Create current PID file.
+  with open(os.path.join(temp_dir, TEMP_PID_FILE), 'wb') as f:

[dnj, 2014/09/12 17:31:56]

I don't see any reason to use the 'b' flag here.

+    f.write(str(os.getpid()))
+  return temp_dir
+
+
+def clear_temp_dir(temp_dir):
+  # Try to nuke the pidfile first.
+  pfile = os.path.join(temp_dir, TEMP_PID_FILE)
+  try:
+    os.unlink(pfile)
+  except OSError as e:
+    print >> sys.stderr, "Error removing %r: %s" % (pfile, e)
+  shutil.rmtree(temp_dir, ignore_errors=True)