Fixes #524

- Had attempted to add more functionality by assigning different
  meanign None, True, and False values "keep_stage" (where False was
  "always delete").

- Turns out that's not really worth the complexity.  Having the third
  "always delete" sense is hardly ever useful but makes the code hard
  to understand.

3 files changed, 49 insertions, 17 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 972a0410b9..ca9e9c4bd1 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -825,7 +825,7 @@ class Package(object):
 
 
     def do_install(self,
-                   keep_prefix=False,  keep_stage=None, ignore_deps=False,
+                   keep_prefix=False,  keep_stage=False, ignore_deps=False,
                    skip_patch=False, verbose=False, make_jobs=None, fake=False):
         """Called by commands to install a package and its dependencies.
 
@@ -834,8 +834,9 @@ class Package(object):
 
         Args:
         keep_prefix -- Keep install prefix on failure. By default, destroys it.
-        keep_stage  -- Set to True or false to always keep or always delete stage.
-                       By default, stage is destroyed only if there are no exceptions.
+        keep_stage  -- By default, stage is destroyed only if there are no
+                       exceptions during build. Set to True to keep the stage
+                       even with exceptions.
         ignore_deps -- Do not install dependencies before installing this package.
         fake        -- Don't really build -- install fake stub files instead.
         skip_patch  -- Skip patch stage of build if True.
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 5354135e6a..f88f82fc2d 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -88,7 +88,8 @@ class Stage(object):
     similar, and are intended to persist for only one run of spack.
     """
 
-    def __init__(self, url_or_fetch_strategy, name=None, mirror_path=None, keep=None):
+    def __init__(self, url_or_fetch_strategy,
+                 name=None, mirror_path=None, keep=False):
         """Create a stage object.
            Parameters:
              url_or_fetch_strategy
@@ -108,10 +109,9 @@ class Stage(object):
 
              keep
                  By default, when used as a context manager, the Stage
-                 is cleaned up when everything goes well, and it is
-                 kept intact when an exception is raised. You can
-                 override this behavior by setting keep to True
-                 (always keep) or False (always delete).
+                 is deleted on exit when no exceptions are raised.
+                 Pass True to keep the stage intact even if no
+                 exceptions are raised.
         """
         # TODO: fetch/stage coupling needs to be reworked -- the logic
         # TODO: here is convoluted and not modular enough.
@@ -166,12 +166,8 @@ class Stage(object):
         Returns:
             Boolean
         """
-        if self.keep is None:
-            # Default: delete when there are no exceptions.
-            if exc_type is None: self.destroy()
-
-        elif not self.keep:
-            # Overridden. Either always keep or always delete.
+        # Delete when there are no exceptions, unless asked to keep.
+        if exc_type is None and not self.keep:
             self.destroy()
 
 
@@ -195,8 +191,8 @@ class Stage(object):
             real_tmp = os.path.realpath(self.tmp_root)
 
             if spack.use_tmp_stage:
-                # If we're using a tmp dir, it's a link, and it points at the right spot,
-                # then keep it.
+                # If we're using a tmp dir, it's a link, and it points at the
+                # right spot, then keep it.
                 if (real_path.startswith(real_tmp) and os.path.exists(real_path)):
                     return False
                 else:
@@ -441,7 +437,7 @@ class StageComposite:
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         for item in reversed(self):
-            item.keep = getattr(self, 'keep', None)
+            item.keep = getattr(self, 'keep', False)
             item.__exit__(exc_type, exc_val, exc_tb)
 
     #
diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py
index dbcf89d864..ea425127c4 100644
--- a/lib/spack/spack/test/stage.py
+++ b/lib/spack/spack/test/stage.py
@@ -277,3 +277,38 @@ class StageTest(unittest.TestCase):
             self.check_chdir_to_source(stage, stage_name)
             self.assertFalse('foobar' in os.listdir(stage.source_path))
         self.check_destroy(stage, stage_name)
+
+
+    def test_no_keep_without_exceptions(self):
+        with Stage(archive_url, name=stage_name, keep=False) as stage:
+            pass
+        self.check_destroy(stage, stage_name)
+
+
+    def test_keep_without_exceptions(self):
+        with Stage(archive_url, name=stage_name, keep=True) as stage:
+            pass
+        path = self.get_stage_path(stage, stage_name)
+        self.assertTrue(os.path.isdir(path))
+
+
+    def test_no_keep_with_exceptions(self):
+        try:
+            with Stage(archive_url, name=stage_name, keep=False) as stage:
+                raise Exception()
+
+            path = self.get_stage_path(stage, stage_name)
+            self.assertTrue(os.path.isdir(path))
+        except:
+            pass # ignore here.
+
+
+    def test_keep_exceptions(self):
+        try:
+            with Stage(archive_url, name=stage_name, keep=True) as stage:
+                raise Exception()
+
+            path = self.get_stage_path(stage, stage_name)
+            self.assertTrue(os.path.isdir(path))
+        except:
+            pass # ignore here.