Merge pull request #7 in SCALE/spack from bugfix/SPACK-10-fork-on-install to develop

# By Todd Gamblin
# Via Todd Gamblin
* commit 'b4fddad7eff448adf701fc9e88cf02cd6e582f15':
  Fix for SPACK-10: Spack now forks before install()

lib/spack/spack/directory_layout.py

@@ -86,8 +86,11 @@ def remove_path_for_spec(self, spec):
             shutil.rmtree(path, True)
 
         path = os.path.dirname(path)
-        while not os.listdir(path) and path != self.root:
-            os.rmdir(path)
+        while path != self.root:
+            if os.path.isdir(path):
+                if os.listdir(path):
+                    return
+                os.rmdir(path)
             path = os.path.dirname(path)
 
 

lib/spack/spack/package.py

@@ -33,9 +33,9 @@
 rundown on spack and how it differs from homebrew, look at the
 README.
 """
-import inspect
 import os
 import re
+import inspect
 import subprocess
 import platform as py_platform
 import multiprocessing
@@ -387,8 +387,9 @@ def default_version(self):
         try:
             return url.parse_version(self.__class__.url)
         except UndetectableVersionError:
-            tty.die("Couldn't extract a default version from %s. You " +
-                    "must specify it explicitly in the package." % self.url)
+            raise PackageError(
+                "Couldn't extract a default version from %s." % self.url,
+                " You must specify it explicitly in the package file.")
 
 
     @property
@@ -490,7 +491,7 @@ def virtual_dependencies(self, visited=None):
 
     @property
     def installed(self):
-        return os.path.exists(self.prefix)
+        return os.path.isdir(self.prefix)
 
 
     @property
@@ -544,10 +545,11 @@ def do_fetch(self):
             raise ValueError("Can only fetch concrete packages.")
 
         if spack.do_checksum and not self.version in self.versions:
-            tty.die("Cannot fetch %s@%s safely; there is no checksum on file for this "
-                    "version." % (self.name, self.version),
-                    "Add a checksum to the package file, or use --no-checksum to "
-                    "skip this check.")
+            raise ChecksumError(
+                "Cannot fetch %s safely; there is no checksum on file for version %s."
+                % self.version,
+                "Add a checksum to the package file, or use --no-checksum to "
+                "skip this check.")
 
         self.stage.fetch()
 
@@ -557,8 +559,9 @@ def do_fetch(self):
             if checker.check(self.stage.archive_file):
                 tty.msg("Checksum passed for %s" % self.name)
             else:
-                tty.die("%s checksum failed for %s.  Expected %s but got %s."
-                        % (checker.hash_name, self.name, digest, checker.sum))
+                raise ChecksumError(
+                    "%s checksum failed for %s." % checker.hash_name,
+                    "Expected %s but got %s." % (self.name, digest, checker.sum))
 
 
     def do_stage(self):
@@ -640,32 +643,56 @@ def do_install(self):
 
         self.do_patch()
 
-        # create the install directory (allow the layout to handle this in
-        # case it needs to add extra files)
-        spack.install_layout.make_path_for_spec(self.spec)
-
-        tty.msg("Building %s." % self.name)
+        # Fork a child process to do the build.  This allows each
+        # package authors to have full control over their environment,
+        # etc. without offecting other builds that might be executed
+        # in the same spack call.
         try:
+            pid = os.fork()
+        except OSError, e:
+            raise InstallError("Unable to fork build process: %s" % e)
+
+        if pid == 0:
+            tty.msg("Building %s." % self.name)
+
+            # create the install directory (allow the layout to handle
+            # this in case it needs to add extra files)
+            spack.install_layout.make_path_for_spec(self.spec)
+
+            # Set up process's build environment before running install.
             build_env.set_build_environment_variables(self)
             build_env.set_module_variables_for_package(self)
 
-            self.install(self.spec, self.prefix)
-            if not os.path.isdir(self.prefix):
-                tty.die("Install failed for %s.  No install dir created." % self.name)
+            try:
+                # Subclasses implement install() to do the build &
+                # install work.
+                self.install(self.spec, self.prefix)
 
-            tty.msg("Successfully installed %s" % self.name)
-            print_pkg(self.prefix)
+                if not os.listdir(self.prefix):
+                    raise InstallError(
+                        "Install failed for %s.  Nothing was installed!"
+                        % self.name)
 
-        except Exception, e:
-            self.remove_prefix()
-            raise
-
-        finally:
-            # Once the install is done, destroy the stage where we built it,
-            # unless the user wants it kept around.
-            if not self.dirty:
+                # On successful install, remove the stage.
+                # Leave if if there is an error
                 self.stage.destroy()
 
+                tty.msg("Successfully installed %s" % self.name)
+                print_pkg(self.prefix)
+
+                sys.exit(0)
+
+            except Exception, e:
+                self.remove_prefix()
+                raise
+
+        # Parent process just waits for the child to complete.  If the
+        # child exited badly, assume it already printed an appropriate
+        # message.  Just make the parent exit with an error code.
+        pid, returncode = os.waitpid(pid, 0)
+        if returncode != 0:
+            sys.exit(1)
+
 
     def do_install_dependencies(self):
         # Pass along paths of dependencies here
@@ -684,16 +711,16 @@ def module(self):
 
     def install(self, spec, prefix):
         """Package implementations override this with their own build configuration."""
-        tty.die("Packages must provide an install method!")
+        raise InstallError("Package %s provides no install method!" % self.name)
 
 
     def do_uninstall(self):
         if not self.installed:
-            tty.die(self.name + " is not installed.")
+            raise InstallError(self.name + " is not installed.")
 
         if not self.ignore_dependencies:
             deps = self.installed_dependents
-            if deps: tty.die(
+            if deps: raise InstallError(
                 "Cannot uninstall %s. The following installed packages depend on it: %s"
                 % (self.name, deps))
 
@@ -817,7 +844,32 @@ def print_pkg(message):
     print message
 
 
-class InvalidPackageDependencyError(spack.error.SpackError):
+
+class FetchError(spack.error.SpackError):
+    """Raised when something goes wrong during fetch."""
+    def __init__(self, message, long_msg=None):
+        super(FetchError, self).__init__(message, long_msg)
+
+
+class ChecksumError(FetchError):
+    """Raised when archive fails to checksum."""
+    def __init__(self, message, long_msg):
+        super(ChecksumError, self).__init__(message, long_msg)
+
+
+class InstallError(spack.error.SpackError):
+    """Raised when something goes wrong during install or uninstall."""
+    def __init__(self, message, long_msg=None):
+        super(InstallError, self).__init__(message, long_msg)
+
+
+class PackageError(spack.error.SpackError):
+    """Raised when something is wrong with a package definition."""
+    def __init__(self, message, long_msg=None):
+        super(PackageError, self).__init__(message, long_msg)
+
+
+class InvalidPackageDependencyError(PackageError):
     """Raised when package specification is inconsistent with requirements of
        its dependencies."""
     def __init__(self, message):

lib/spack/spack/test/install.py

@@ -93,6 +93,6 @@ def test_install_and_uninstall(self):
         try:
             pkg.do_install()
             pkg.do_uninstall()
-        except:
+        except Exception, e:
             if pkg: pkg.remove_prefix()
             raise