recipe engine: add tempfile and shutil modules

recipe_modules/tempfile/api.py

Index: recipe_modules/tempfile/api.py
diff --git a/recipe_modules/tempfile/api.py b/recipe_modules/tempfile/api.py
new file mode 100644
index 0000000000000000000000000000000000000000..1bbbf407013c9e5fb6e6efdf5533277b4fcc0144
--- /dev/null
+++ b/recipe_modules/tempfile/api.py
@@ -0,0 +1,19 @@
+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import contextlib
+
+from recipe_engine import recipe_api
+
+
+class TempfileApi(recipe_api.RecipeApi):
+  @contextlib.contextmanager
+  def temp_dir(self, prefix):

[martiniss, 2016/04/07 17:35:22]

This (roughly) already exists at
https://github.com/luci/recipes-py/blob/master/recipe_modules/path/api.py#L198

If you want a context manager, adding one to the path class would probably be
preferable. Although it's a bit strange that the mkdtemp function is part of the
path api module.

[Paweł Hajdan Jr., 2016/04/07 20:39:04]

Yes, in fact this method calls path.mkdtemp .
That was my first choice. However, then path module would have to depend on
shutil (for rmtree), and that leads to circular dependencies.

To me what's done in this patch makes sense. mkdtemp also registers existence of
the path for testing. Other logic doesn't need that, so can be in a separate
recipe module.

If you have an alternative way to implement it (which is implementable - watch
out for things like circular dependencies I mentioned above), please share.

Also let me know if you'd be OK with having an EMEA reviewer stamp the
implementation provided it'd be consistent with some guidelines we can agree on
(so that we don't need so many round-trips).

[martiniss, 2016/04/07 21:34:02]

I mis-read this function, sorry.
shutil doesn't depend on anything, though? How would it be circular.

tempfile -> path -> shutil
tempfile -> shutil

correct?
circular dependencies are allowed in the recipe engine AFAIK, I believe on
purpose. It might not always be the best idea, but it is technically feasible.
I would want a recipe engine owner to stamp any CLs which add a recipe module to
the engine itself, so I'm not sure who besides you in EMEA time would be able to
review this. I'm not sure who out there knows a lot about recipes and best
practices around them.

[iannucci, 2016/04/08 00:19:20]

This is fine for now, but this is really the wrong way to think about tempdirs
in general. Crashes will cause them to stick around, and nothing will clean them
up.

A better way to do this, I think would be to basically implement the
long-standing crbug.com/361343 . Basically, recipe engine manages TEMP/TMP
envvars and does:
  * at startup, allocate a new tempfolder with some recognizable patter with a
.pid file for ourselves inside
  * hunt for any stale temp folders matching that pattern and delete them
  * each step automatically gets it's own subdir of the overall recipe run's
temp folder, with correctly adjusted TEMP/TMP
  * api.path.mkdtemp would generate a random tempdir inside of this engine-wide
tempdir.
  * cleanups are handled by the engine at the end of a scope (duration of
context manager for path.mkdtemp or duration of the step). Ideally we could also
print an optional leaked-files report on the step so we can e.g. catch messy
tests.

[Paweł Hajdan Jr., 2016/04/08 07:46:15]

The circular dependency was path -> python -> step -> path ; by extracting
shutil and tempfile we get tempfile -> shutil -> python -> step -> path (no
cycle).
That's new to me. I got an actual error with above circular dependency, and
that's in recipe engine.

+    path = None
+    try:
+      path = self.m.path.mkdtemp(prefix)
+      yield path
+    finally:
+      if path:
+        self.m.shutil.rmtree(path, infra_step=True)