From 5aadb6df19ea6d7575ee5f5e5f18d92934245d98 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Tue, 8 Mar 2016 02:50:26 -0800 Subject: [PATCH] Fixes #502. Create install prefix before build, clean up do_install. - Fix bug introduced during merge of stage refactor. - install prefix was not created before build_environment.fork() - build_environment.fork() calls setup_dependent_environment - python's setup_dependent_environment can inadvertently create the install prefix before directory_layout expects it. - Clean up Package.do_install: - simplify control flow: parent process now entirely responsible for creating/destroying the install prefix. cleanup is now in one place. - Hoisting cleanup out of the child improves nesting of try/catch in `real_work`. - `real_work` renamed to `build_process` --- lib/spack/spack/directory_layout.py | 56 +++++++++----- lib/spack/spack/package.py | 115 +++++++++++++--------------- 2 files changed, 91 insertions(+), 80 deletions(-) diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py index 08c23627f4..242eb1afa0 100644 --- a/lib/spack/spack/directory_layout.py +++ b/lib/spack/spack/directory_layout.py @@ -85,6 +85,16 @@ def create_install_directory(self, spec): raise NotImplementedError() + def check_installed(self, spec): + """Checks whether a spec is installed. + + Return the spec's prefix, if it is installed, None otherwise. + + Raise an exception if the install is inconsistent or corrupt. + """ + raise NotImplementedError() + + def extension_map(self, spec): """Get a dict of currently installed extension packages for a spec. @@ -246,26 +256,36 @@ def build_packages_path(self, spec): def create_install_directory(self, spec): _check_concrete(spec) + prefix = self.check_installed(spec) + if prefix: + raise InstallDirectoryAlreadyExistsError(prefix) + + mkdirp(self.metadata_path(spec)) + self.write_spec(spec, self.spec_file_path(spec)) + + + def check_installed(self, spec): + _check_concrete(spec) path = self.path_for_spec(spec) spec_file_path = self.spec_file_path(spec) - if os.path.isdir(path): - if not os.path.isfile(spec_file_path): - raise InconsistentInstallDirectoryError( - 'No spec file found at path %s' % spec_file_path) + if not os.path.isdir(path): + return None - installed_spec = self.read_spec(spec_file_path) - if installed_spec == self.spec: - raise InstallDirectoryAlreadyExistsError(path) + if not os.path.isfile(spec_file_path): + raise InconsistentInstallDirectoryError( + 'Inconsistent state: install prefix exists but contains no spec.yaml:', + " " + path) - if spec.dag_hash() == installed_spec.dag_hash(): - raise SpecHashCollisionError(installed_hash, spec_hash) - else: - raise InconsistentInstallDirectoryError( - 'Spec file in %s does not match hash!' % spec_file_path) + installed_spec = self.read_spec(spec_file_path) + if installed_spec == spec: + return path - mkdirp(self.metadata_path(spec)) - self.write_spec(spec, spec_file_path) + if spec.dag_hash() == installed_spec.dag_hash(): + raise SpecHashCollisionError(installed_hash, spec_hash) + else: + raise InconsistentInstallDirectoryError( + 'Spec file in %s does not match hash!' % spec_file_path) def all_specs(self): @@ -399,8 +419,8 @@ def remove_extension(self, spec, ext_spec): class DirectoryLayoutError(SpackError): """Superclass for directory layout errors.""" - def __init__(self, message): - super(DirectoryLayoutError, self).__init__(message) + def __init__(self, message, long_msg=None): + super(DirectoryLayoutError, self).__init__(message, long_msg) class SpecHashCollisionError(DirectoryLayoutError): @@ -422,8 +442,8 @@ def __init__(self, installed_spec, prefix, error): class InconsistentInstallDirectoryError(DirectoryLayoutError): """Raised when a package seems to be installed to the wrong place.""" - def __init__(self, message): - super(InconsistentInstallDirectoryError, self).__init__(message) + def __init__(self, message, long_msg=None): + super(InconsistentInstallDirectoryError, self).__init__(message, long_msg) class InstallDirectoryAlreadyExistsError(DirectoryLayoutError): diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index d4acbf5024..972a0410b9 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -845,7 +845,8 @@ def do_install(self, if not self.spec.concrete: raise ValueError("Can only install concrete packages.") - if os.path.exists(self.prefix): + # Ensure package is not already installed + if spack.install_layout.check_installed(self.spec): tty.msg("%s is already installed in %s" % (self.name, self.prefix)) return @@ -857,18 +858,11 @@ def do_install(self, keep_prefix=keep_prefix, keep_stage=keep_stage, ignore_deps=ignore_deps, fake=fake, skip_patch=skip_patch, verbose=verbose, make_jobs=make_jobs) - def cleanup(): - """Handles removing install prefix on error.""" - if not keep_prefix: - self.remove_prefix() - else: - tty.warn("Keeping install prefix in place despite error.", - "Spack will think this package is installed. " + - "Manually remove this directory to fix:", - self.prefix, wrap=True) + # Set parallelism before starting build. + self.make_jobs = make_jobs # Then install the package itself. - def real_work(): + def build_process(): """Forked for each build. Has its own process and python module space set up by build_environment.fork().""" start_time = time.time() @@ -878,30 +872,24 @@ def real_work(): else: self.do_stage() - # create the install directory. The install layout - # handles this in case so that it can use whatever - # package naming scheme it likes. - spack.install_layout.create_install_directory(self.spec) + tty.msg("Building %s" % self.name) - try: - tty.msg("Building %s" % self.name) + self.stage.keep = keep_stage + with self.stage: + # Run the pre-install hook in the child process after + # the directory is created. + spack.hooks.pre_install(self) - self.stage.keep = keep_stage - with self.stage: - # Run the pre-install hook in the child process after - # the directory is created. - spack.hooks.pre_install(self) + if fake: + self.do_fake_install() + else: + # Do the real install in the source directory. + self.stage.chdir_to_source() - if fake: - self.do_fake_install() - - else: - # Do the real install in the source directory. - self.stage.chdir_to_source() - - # Save the build environment in a file before building. - env_path = join_path(os.getcwd(), 'spack-build.env') + # Save the build environment in a file before building. + env_path = join_path(os.getcwd(), 'spack-build.env') + try: # Redirect I/O to a build log (and optionally to the terminal) log_path = join_path(os.getcwd(), 'spack-build.out') log_file = open(log_path, 'w') @@ -909,43 +897,46 @@ def real_work(): dump_environment(env_path) self.install(self.spec, self.prefix) - # Ensure that something was actually installed. - self._sanity_check_install() + except ProcessError as e: + # Annotate ProcessErrors with the location of the build log. + e.build_log = log_path + raise e - # Copy provenance into the install directory on success - log_install_path = spack.install_layout.build_log_path(self.spec) - env_install_path = spack.install_layout.build_env_path(self.spec) - packages_dir = spack.install_layout.build_packages_path(self.spec) + # Ensure that something was actually installed. + self._sanity_check_install() - install(log_path, log_install_path) - install(env_path, env_install_path) - dump_packages(self.spec, packages_dir) + # Copy provenance into the install directory on success + log_install_path = spack.install_layout.build_log_path(self.spec) + env_install_path = spack.install_layout.build_env_path(self.spec) + packages_dir = spack.install_layout.build_packages_path(self.spec) - # Stop timer. - self._total_time = time.time() - start_time - build_time = self._total_time - self._fetch_time + install(log_path, log_install_path) + install(env_path, env_install_path) + dump_packages(self.spec, packages_dir) - tty.msg("Successfully installed %s" % self.name, - "Fetch: %s. Build: %s. Total: %s." - % (_hms(self._fetch_time), _hms(build_time), _hms(self._total_time))) - print_pkg(self.prefix) + # Stop timer. + self._total_time = time.time() - start_time + build_time = self._total_time - self._fetch_time - except ProcessError as e: - # Annotate with location of build log. - e.build_log = log_path - cleanup() - raise e + tty.msg("Successfully installed %s" % self.name, + "Fetch: %s. Build: %s. Total: %s." + % (_hms(self._fetch_time), _hms(build_time), _hms(self._total_time))) + print_pkg(self.prefix) - except: - # other exceptions just clean up and raise. - cleanup() - raise - - # Set parallelism before starting build. - self.make_jobs = make_jobs - - # Do the build. - spack.build_environment.fork(self, real_work) + try: + # Create the install prefix and fork the build process. + spack.install_layout.create_install_directory(self.spec) + spack.build_environment.fork(self, build_process) + except: + # remove the install prefix if anything went wrong during install. + if not keep_prefix: + self.remove_prefix() + else: + tty.warn("Keeping install prefix in place despite error.", + "Spack will think this package is installed. " + + "Manually remove this directory to fix:", + self.prefix, wrap=True) + raise # note: PARENT of the build process adds the new package to # the database, so that we don't need to re-read from file.