From 8b7d2d0f24c7125b5dafa857352ad24f33f3b06c Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 24 Oct 2017 21:32:30 +0200 Subject: [PATCH] 'spack install' can overwrite an existing installation (#5384) 'spack install' can now reinstall a spec even if it has dependents, via the --overwrite option. This option moves the current installation in a temporary directory. If the reinstallation is successful the temporary is removed, otherwise a rollback is performed. --- lib/spack/llnl/util/filesystem.py | 84 ++++++++++++++- lib/spack/spack/cmd/install.py | 136 ++++++++++++++++-------- lib/spack/spack/test/cmd/install.py | 37 +++++++ lib/spack/spack/test/util/filesystem.py | 61 +++++++++++ 4 files changed, 270 insertions(+), 48 deletions(-) create mode 100644 lib/spack/spack/test/util/filesystem.py diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index 56b96b3a32..1250319e41 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -24,6 +24,7 @@ ############################################################################## import collections import errno +import hashlib import fileinput import fnmatch import glob @@ -31,12 +32,13 @@ import os import re import shutil -import six import stat import subprocess import sys +import tempfile from contextlib import contextmanager +import six from llnl.util import tty from llnl.util.lang import dedupe @@ -282,6 +284,60 @@ def working_dir(dirname, **kwargs): os.chdir(orig_dir) +@contextmanager +def replace_directory_transaction(directory_name, tmp_root=None): + """Moves a directory to a temporary space. If the operations executed + within the context manager don't raise an exception, the directory is + deleted. If there is an exception, the move is undone. + + Args: + directory_name (path): absolute path of the directory name + tmp_root (path): absolute path of the parent directory where to create + the temporary + + Returns: + temporary directory where ``directory_name`` has been moved + """ + # Check the input is indeed a directory with absolute path. + # Raise before anything is done to avoid moving the wrong directory + assert os.path.isdir(directory_name), \ + '"directory_name" must be a valid directory' + assert os.path.isabs(directory_name), \ + '"directory_name" must contain an absolute path' + + directory_basename = os.path.basename(directory_name) + + if tmp_root is not None: + assert os.path.isabs(tmp_root) + + tmp_dir = tempfile.mkdtemp(dir=tmp_root) + tty.debug('TEMPORARY DIRECTORY CREATED [{0}]'.format(tmp_dir)) + + shutil.move(src=directory_name, dst=tmp_dir) + tty.debug('DIRECTORY MOVED [src={0}, dest={1}]'.format( + directory_name, tmp_dir + )) + + try: + yield tmp_dir + except (Exception, KeyboardInterrupt, SystemExit): + # Delete what was there, before copying back the original content + if os.path.exists(directory_name): + shutil.rmtree(directory_name) + shutil.move( + src=os.path.join(tmp_dir, directory_basename), + dst=os.path.dirname(directory_name) + ) + tty.debug('DIRECTORY RECOVERED [{0}]'.format(directory_name)) + + msg = 'the transactional move of "{0}" failed.' + raise RuntimeError(msg.format(directory_name)) + else: + # Otherwise delete the temporary directory + shutil.rmtree(tmp_dir) + tty.debug('TEMPORARY DIRECTORY DELETED [{0}]'.format(tmp_dir)) + + @contextmanager def hide_files(*file_list): try: @@ -294,6 +350,32 @@ def hide_files(*file_list): shutil.move(bak, f) +def hash_directory(directory): + """Hashes recursively the content of a directory. + + Args: + directory (path): path to a directory to be hashed + + Returns: + hash of the directory content + """ + assert os.path.isdir(directory), '"directory" must be a directory!' + + md5_hash = hashlib.md5() + + # Adapted from https://stackoverflow.com/a/3431835/771663 + for root, dirs, files in os.walk(directory): + for name in sorted(files): + filename = os.path.join(root, name) + # TODO: if caching big files becomes an issue, convert this to + # TODO: read in chunks. Currently it's used only for testing + # TODO: purposes. + with open(filename, 'rb') as f: + md5_hash.update(f.read()) + + return md5_hash.hexdigest() + + def touch(path): """Creates an empty file at the specified path.""" perms = (os.O_WRONLY | os.O_CREAT | os.O_NONBLOCK | os.O_NOCTTY) diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py index 7c014e0a92..f9b2e69d00 100644 --- a/lib/spack/spack/cmd/install.py +++ b/lib/spack/spack/cmd/install.py @@ -61,6 +61,9 @@ def setup_parser(subparser): subparser.add_argument( '-j', '--jobs', action='store', type=int, help="explicitly set number of make jobs. default is #cpus") + subparser.add_argument( + '--overwrite', action='store_true', + help="reinstall an existing spec, even if it has dependents") subparser.add_argument( '--keep-prefix', action='store_true', help="don't remove the install prefix if installation fails") @@ -84,7 +87,7 @@ def setup_parser(subparser): help="display verbose build output while installing") subparser.add_argument( '--fake', action='store_true', - help="fake install. just remove prefix and create a fake file") + help="fake install for debug purposes.") subparser.add_argument( '-f', '--file', action='store_true', help="install from file. Read specs to install from .yaml files") @@ -121,6 +124,7 @@ def setup_parser(subparser): default=None, help="filename for the log file. if not passed a default will be used" ) + arguments.add_common_arguments(subparser, ['yes_to_all']) # Needed for test cases @@ -314,6 +318,61 @@ def default_log_file(spec): return fs.join_path(dirname, basename) +def install_spec(cli_args, kwargs, spec): + + saved_do_install = PackageBase.do_install + decorator = lambda fn: fn + + # Check if we were asked to produce some log for dashboards + if cli_args.log_format is not None: + # Compute the filename for logging + log_filename = cli_args.log_file + if not log_filename: + log_filename = default_log_file(spec) + + # Create the test suite in which to log results + test_suite = TestSuite(spec) + + # Temporarily decorate PackageBase.do_install to monitor + # recursive calls. + decorator = junit_output(spec, test_suite) + + # Do the actual installation + try: + # decorate the install if necessary + PackageBase.do_install = decorator(PackageBase.do_install) + + if cli_args.things_to_install == 'dependencies': + # Install dependencies as-if they were installed + # for root (explicit=False in the DB) + kwargs['explicit'] = False + for s in spec.dependencies(): + p = spack.repo.get(s) + p.do_install(**kwargs) + else: + package = spack.repo.get(spec) + kwargs['explicit'] = True + package.do_install(**kwargs) + + except InstallError as e: + if cli_args.show_log_on_error: + e.print_context() + if not os.path.exists(e.pkg.build_log_path): + tty.error("'spack install' created no log.") + else: + sys.stderr.write('Full build log:\n') + with open(e.pkg.build_log_path) as log: + shutil.copyfileobj(log, sys.stderr) + raise + + finally: + PackageBase.do_install = saved_do_install + + # Dump test output if asked to + if cli_args.log_format is not None: + test_suite.dump(log_filename) + + def install(parser, args, **kwargs): if not args.package: tty.die("install requires at least one package argument") @@ -360,56 +419,39 @@ def install(parser, args, **kwargs): if len(specs) == 0: tty.error('The `spack install` command requires a spec to install.') - for spec in specs: - saved_do_install = PackageBase.do_install - decorator = lambda fn: fn + if args.overwrite: + # If we asked to overwrite an existing spec we must ensure that: + # 1. We have only one spec + # 2. The spec is already installed + assert len(specs) == 1, \ + "only one spec is allowed when overwriting an installation" - # Check if we were asked to produce some log for dashboards - if args.log_format is not None: - # Compute the filename for logging - log_filename = args.log_file - if not log_filename: - log_filename = default_log_file(spec) + spec = specs[0] + t = spack.store.db.query(spec) + assert len(t) == 1, "to overwrite a spec you must install it first" - # Create the test suite in which to log results - test_suite = TestSuite(spec) + # Give the user a last chance to think about overwriting an already + # existing installation + if not args.yes_to_all: + tty.msg('The following package will be reinstalled:\n') - # Temporarily decorate PackageBase.do_install to monitor - # recursive calls. - decorator = junit_output(spec, test_suite) + display_args = { + 'long': True, + 'show_flags': True, + 'variants': True + } - # Do the actual installation - try: - # decorate the install if necessary - PackageBase.do_install = decorator(PackageBase.do_install) + spack.cmd.display_specs(t, **display_args) + answer = tty.get_yes_or_no( + 'Do you want to proceed?', default=False + ) + if not answer: + tty.die('Reinstallation aborted.') - if args.things_to_install == 'dependencies': - # Install dependencies as-if they were installed - # for root (explicit=False in the DB) - kwargs['explicit'] = False - for s in spec.dependencies(): - p = spack.repo.get(s) - p.do_install(**kwargs) + with fs.replace_directory_transaction(specs[0].prefix): + install_spec(args, kwargs, specs[0]) - else: - package = spack.repo.get(spec) - kwargs['explicit'] = True - package.do_install(**kwargs) + else: - except InstallError as e: - if args.show_log_on_error: - e.print_context() - if not os.path.exists(e.pkg.build_log_path): - tty.error("'spack install' created no log.") - else: - sys.stderr.write('Full build log:\n') - with open(e.pkg.build_log_path) as log: - shutil.copyfileobj(log, sys.stderr) - raise - - finally: - PackageBase.do_install = saved_do_install - - # Dump test output if asked to - if args.log_format is not None: - test_suite.dump(log_filename) + for spec in specs: + install_spec(args, kwargs, spec) diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py index 124836ef9a..77fe39d58f 100644 --- a/lib/spack/spack/test/cmd/install.py +++ b/lib/spack/spack/test/cmd/install.py @@ -28,6 +28,8 @@ import pytest +import llnl.util.filesystem as fs + import spack import spack.cmd.install from spack.spec import Spec @@ -197,3 +199,38 @@ def test_show_log_on_error(builtin_mock, mock_archive, mock_fetch, errors = [line for line in out.split('\n') if 'configure: error: cannot run C compiled programs' in line] assert len(errors) == 2 + + +def test_install_overwrite( + builtin_mock, mock_archive, mock_fetch, config, install_mockery +): + # It's not possible to overwrite something that is not yet installed + with pytest.raises(AssertionError): + install('--overwrite', 'libdwarf') + + # --overwrite requires a single spec + with pytest.raises(AssertionError): + install('--overwrite', 'libdwarf', 'libelf') + + # Try to install a spec and then to reinstall it. + spec = Spec('libdwarf') + spec.concretize() + + install('libdwarf') + + assert os.path.exists(spec.prefix) + expected_md5 = fs.hash_directory(spec.prefix) + + # Modify the first installation to be sure the content is not the same + # as the one after we reinstalled + with open(os.path.join(spec.prefix, 'only_in_old'), 'w') as f: + f.write('This content is here to differentiate installations.') + + bad_md5 = fs.hash_directory(spec.prefix) + + assert bad_md5 != expected_md5 + + install('--overwrite', '-y', 'libdwarf') + assert os.path.exists(spec.prefix) + assert fs.hash_directory(spec.prefix) == expected_md5 + assert fs.hash_directory(spec.prefix) != bad_md5 diff --git a/lib/spack/spack/test/util/filesystem.py b/lib/spack/spack/test/util/filesystem.py new file mode 100644 index 0000000000..4ed65da857 --- /dev/null +++ b/lib/spack/spack/test/util/filesystem.py @@ -0,0 +1,61 @@ +############################################################################## +# Copyright (c) 2013-2017, Lawrence Livermore National Security, LLC. +# Produced at the Lawrence Livermore National Laboratory. +# +# This file is part of Spack. +# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved. +# LLNL-CODE-647188 +# +# For details, see https://github.com/llnl/spack +# Please also see the NOTICE and LICENSE files for our notice and the LGPL. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License (as +# published by the Free Software Foundation) version 2.1, February 1999. +# +# This program is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and +# conditions of the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +############################################################################## + +import llnl.util.filesystem as fs + + +def test_move_transaction_commit(tmpdir): + + fake_library = tmpdir.mkdir('lib').join('libfoo.so') + fake_library.write('Just some fake content.') + + old_md5 = fs.hash_directory(str(tmpdir)) + + with fs.replace_directory_transaction(str(tmpdir.join('lib'))): + fake_library = tmpdir.mkdir('lib').join('libfoo.so') + fake_library.write('Other content.') + new_md5 = fs.hash_directory(str(tmpdir)) + + assert old_md5 != fs.hash_directory(str(tmpdir)) + assert new_md5 == fs.hash_directory(str(tmpdir)) + + +def test_move_transaction_rollback(tmpdir): + + fake_library = tmpdir.mkdir('lib').join('libfoo.so') + fake_library.write('Just some fake content.') + + h = fs.hash_directory(str(tmpdir)) + + try: + with fs.replace_directory_transaction(str(tmpdir.join('lib'))): + assert h != fs.hash_directory(str(tmpdir)) + fake_library = tmpdir.mkdir('lib').join('libfoo.so') + fake_library.write('Other content.') + raise RuntimeError('') + except RuntimeError: + pass + + assert h == fs.hash_directory(str(tmpdir))