From 0ea6e0f8176db1ec28f1d65aa26c932f1ed055e7 Mon Sep 17 00:00:00 2001 From: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com> Date: Mon, 19 Aug 2019 10:31:24 -0700 Subject: [PATCH] features: Remove stage symlinks (#12072) Fixes #11163 The goal of this work is to simplify stage directory structures by eliminating use of symbolic links. This means, among other things, that` $spack/var/spack/stage` will no longer be the core staging directory. Instead, the first accessible `config:build_stage` path will be used. Spack will no longer automatically append `spack-stage` (or the like) to configured build stage directories so the onus of distinguishing the directory from other work -- so the other work is not automatically removed with a `spack clean` operation -- falls on the user. --- etc/spack/defaults/config.yaml | 26 +- lib/spack/docs/config_yaml.rst | 31 +- lib/spack/llnl/util/filesystem.py | 20 +- lib/spack/spack/cmd/location.py | 3 +- lib/spack/spack/compilers/clang.py | 3 +- lib/spack/spack/config.py | 1 + lib/spack/spack/package.py | 4 +- lib/spack/spack/patch.py | 5 +- lib/spack/spack/paths.py | 1 - lib/spack/spack/stage.py | 178 ++++----- lib/spack/spack/test/cmd/location.py | 3 +- lib/spack/spack/test/conftest.py | 54 ++- lib/spack/spack/test/data/config.yaml | 2 +- lib/spack/spack/test/llnl/util/filesystem.py | 15 + lib/spack/spack/test/patch.py | 18 +- lib/spack/spack/test/stage.py | 374 +++++++++++------- .../spack/test/util/spack_lock_wrapper.py | 4 + 17 files changed, 422 insertions(+), 320 deletions(-) diff --git a/etc/spack/defaults/config.yaml b/etc/spack/defaults/config.yaml index 37db99f4c9a..33a37bbdfa3 100644 --- a/etc/spack/defaults/config.yaml +++ b/etc/spack/defaults/config.yaml @@ -37,18 +37,28 @@ config: # Temporary locations Spack can try to use for builds. # - # Spack will use the first one it finds that exists and is writable. - # You can use $tempdir to refer to the system default temp directory - # (as returned by tempfile.gettempdir()). + # Recommended options are given below. # - # A value of $spack/var/spack/stage indicates that Spack should run - # builds directly inside its install directory without staging them in + # Builds can be faster in temporary directories on some (e.g., HPC) systems. + # Specifying `$tempdir` will ensure use of the system default temporary + # directory (as returned by `tempfile.gettempdir()`). Spack will append + # `spack-stage` and, if the username is not already in the path, the value + # of `$user` to the path. The latter is used to avoid conflicts between + # users in shared temporary spaces. + # + # Another option that prevents conflicts and potential permission issues is + # to specify `~/.spack/stage`, which ensures each user builds in their home + # directory. + # + # A more traditional path uses the value of `$spack/var/spack/stage`, which + # builds directly inside Spack's instance without staging them in a # temporary space. # - # The build stage can be purged with `spack clean --stage`. + # The build stage can be purged with `spack clean --stage` and + # `spack clean -a`, so it is important that the specified directory uniquely + # identifies Spack staging to avoid accidentally wiping out non-Spack work. build_stage: - - $tempdir - - $spack/var/spack/stage + - $tempdir/spack-stage # Cache directory for already downloaded source tarballs and archived diff --git a/lib/spack/docs/config_yaml.rst b/lib/spack/docs/config_yaml.rst index 7e2759349b5..ef3ad1d03a2 100644 --- a/lib/spack/docs/config_yaml.rst +++ b/lib/spack/docs/config_yaml.rst @@ -85,38 +85,39 @@ See :ref:`modules` for details. Spack is designed to run out of a user home directory, and on many systems the home directory is a (slow) network file system. On most systems, -building in a temporary file system results in faster builds than building -in the home directory. Usually, there is also more space available in -the temporary location than in the home directory. So, Spack tries to -create build stages in temporary space. +building in a temporary file system is faster. Usually, there is also more +space available in the temporary location than in the home directory. If the +username is not already in the path, Spack will also append the value of +`$user` to the path. + +.. warning:: We highly recommend appending `spack-stage` to `$tempdir` in order + to ensure `spack clean` does not delete everything in `$tempdir`. By default, Spack's ``build_stage`` is configured like this: .. code-block:: yaml build_stage: - - $tempdir - - $spack/var/spack/stage + - $tempdir/spack-stage -This is an ordered list of paths that Spack should search when trying to +This can be an ordered list of paths that Spack should search when trying to find a temporary directory for the build stage. The list is searched in order, and Spack will use the first directory to which it has write access. -See :ref:`config-file-variables` for more on ``$tempdir`` and ``$spack``. +Specifying `~/.spack/stage` first will ensure each user builds in their home +directory. The historic Spack stage path `$spack/var/spack/stage` will build +directly inside the Spack instance. See :ref:`config-file-variables` for more +on ``$tempdir`` and ``$spack``. When Spack builds a package, it creates a temporary directory within the -``build_stage``, and it creates a symbolic link to that directory in -``$spack/var/spack/stage``. This is used to track the temporary -directory. After the package is successfully installed, Spack deletes +``build_stage``. After the package is successfully installed, Spack deletes the temporary directory it used to build. Unsuccessful builds are not deleted, but you can manually purge them with :ref:`spack clean --stage `. .. note:: - The last item in the list is ``$spack/var/spack/stage``. If this is the - only writable directory in the ``build_stage`` list, Spack will build - *directly* in ``$spack/var/spack/stage`` and will not link to temporary - space. + The build will fail if there is no writable directory in the ``build_stage`` + list. -------------------- ``source_cache`` diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index e58f657aead..19feb1686fe 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -347,12 +347,22 @@ def copy_tree(src, dest, symlinks=True, ignore=None, _permissions=False): else: tty.debug('Copying {0} to {1}'.format(src, dest)) + abs_src = os.path.abspath(src) + if not abs_src.endswith(os.path.sep): + abs_src += os.path.sep + abs_dest = os.path.abspath(dest) + if not abs_dest.endswith(os.path.sep): + abs_dest += os.path.sep + + # Stop early to avoid unnecessary recursion if being asked to copy from a + # parent directory. + if abs_dest.startswith(abs_src): + raise ValueError('Cannot copy ancestor directory {0} into {1}'. + format(abs_src, abs_dest)) + mkdirp(dest) - src = os.path.abspath(src) - dest = os.path.abspath(dest) - - for s, d in traverse_tree(src, dest, order='pre', + for s, d in traverse_tree(abs_src, abs_dest, order='pre', follow_symlinks=not symlinks, ignore=ignore, follow_nonexisting=True): @@ -361,7 +371,7 @@ def copy_tree(src, dest, symlinks=True, ignore=None, _permissions=False): if symlinks: target = os.readlink(s) if os.path.isabs(target): - new_target = re.sub(src, dest, target) + new_target = re.sub(abs_src, abs_dest, target) if new_target != target: tty.debug("Redirecting link {0} to {1}" .format(target, new_target)) diff --git a/lib/spack/spack/cmd/location.py b/lib/spack/spack/cmd/location.py index 38d1eab1749..d6716233f99 100644 --- a/lib/spack/spack/cmd/location.py +++ b/lib/spack/spack/cmd/location.py @@ -14,6 +14,7 @@ import spack.environment import spack.paths import spack.repo +import spack.stage description = "print out locations of packages and spack directories" section = "basic" @@ -76,7 +77,7 @@ def location(parser, args): print(spack.repo.path.first_repo().root) elif args.stages: - print(spack.paths.stage_path) + print(spack.stage.get_stage_root()) else: specs = spack.cmd.parse_specs(args.spec) diff --git a/lib/spack/spack/compilers/clang.py b/lib/spack/spack/compilers/clang.py index e4b109588b2..e53332363bd 100644 --- a/lib/spack/spack/compilers/clang.py +++ b/lib/spack/spack/compilers/clang.py @@ -12,6 +12,7 @@ import llnl.util.tty as tty import spack.paths +import spack.stage from spack.compiler import Compiler, UnsupportedCompilerFlag from spack.util.executable import Executable from spack.version import ver @@ -279,7 +280,7 @@ def setup_custom_environment(self, pkg, env): raise OSError(msg) real_root = os.path.dirname(os.path.dirname(real_root)) - developer_root = os.path.join(spack.paths.stage_path, + developer_root = os.path.join(spack.stage.get_stage_root(), 'xcode-select', self.name, str(self.version)) diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index 8114ec21b47..c73b5047f53 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -101,6 +101,7 @@ 'checksum': True, 'dirty': False, 'build_jobs': min(16, multiprocessing.cpu_count()), + 'build_stage': '$tempdir/spack-stage', } } diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index 07d2c286793..5f3cfef6f08 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -1769,9 +1769,7 @@ def check_for_unfinished_installation( else: partial = True - stage_is_managed_in_spack = self.stage.path.startswith( - spack.paths.stage_path) - if restage and stage_is_managed_in_spack: + if restage and self.stage.managed_by_spack: self.stage.destroy() self.stage.create() diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py index 09aa48d9dab..cb012af7ff4 100644 --- a/lib/spack/spack/patch.py +++ b/lib/spack/spack/patch.py @@ -192,9 +192,8 @@ def fetch(self, stage): fetch_digest = self.archive_sha256 fetcher = fs.URLFetchStrategy(self.url, fetch_digest) - mirror = os.path.join( - os.path.dirname(stage.mirror_path), - os.path.basename(self.url)) + mirror = os.path.join(os.path.dirname(stage.mirror_path), + os.path.basename(self.url)) self.stage = spack.stage.Stage(fetcher, mirror_path=mirror) self.stage.create() diff --git a/lib/spack/spack/paths.py b/lib/spack/spack/paths.py index 2f7ccf3b5da..d91b01d87c4 100644 --- a/lib/spack/spack/paths.py +++ b/lib/spack/spack/paths.py @@ -38,7 +38,6 @@ test_path = os.path.join(module_path, "test") hooks_path = os.path.join(module_path, "hooks") var_path = os.path.join(prefix, "var", "spack") -stage_path = os.path.join(var_path, "stage") repos_path = os.path.join(var_path, "repos") share_path = os.path.join(prefix, "share", "spack") diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 6c52afb5298..aa9920ca581 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -3,6 +3,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import grp import os import stat import sys @@ -16,7 +17,7 @@ import llnl.util.tty as tty from llnl.util.filesystem import mkdirp, can_access, install, install_tree -from llnl.util.filesystem import remove_if_dead_link, remove_linked_tree +from llnl.util.filesystem import remove_linked_tree import spack.paths import spack.caches @@ -28,45 +29,54 @@ import spack.util.path as sup from spack.util.crypto import prefix_bits, bit_length + +# The well-known stage source subdirectory name. _source_path_subdir = 'spack-src' + +# The temporary stage name prefix. _stage_prefix = 'spack-stage-' def _first_accessible_path(paths): - """Find a tmp dir that exists that we can access.""" + """Find the first path that is accessible, creating it if necessary.""" for path in paths: try: - # try to create the path if it doesn't exist. + # Ensure the user has access, creating the directory if necessary. path = sup.canonicalize_path(path) - mkdirp(path) + if os.path.exists(path): + if can_access(path): + return path + else: + # The path doesn't exist so create it and adjust permissions + # and group as needed (e.g., shared ``$tempdir``). + prefix = os.path.sep + parts = path.strip(os.path.sep).split(os.path.sep) + for part in parts: + prefix = os.path.join(prefix, part) + if not os.path.exists(prefix): + break + parent = os.path.dirname(prefix) + group = grp.getgrgid(os.stat(parent).st_gid)[0] + mkdirp(path, group=group, default_perms='parents') - # ensure accessible - if not can_access(path): - continue - - # return it if successful. - return path + if can_access(path): + return path except OSError as e: - tty.debug('OSError while checking temporary path %s: %s' % ( + tty.debug('OSError while checking stage path %s: %s' % ( path, str(e))) - continue return None -# cached temporary root -_tmp_root = None -_use_tmp_stage = True +# Cached stage path root +_stage_root = None -def get_tmp_root(): - global _tmp_root, _use_tmp_stage +def get_stage_root(): + global _stage_root - if not _use_tmp_stage: - return None - - if _tmp_root is None: + if _stage_root is None: candidates = spack.config.get('config:build_stage') if isinstance(candidates, string_types): candidates = [candidates] @@ -75,23 +85,16 @@ def get_tmp_root(): if not path: raise StageError("No accessible stage paths in:", candidates) - # Return None to indicate we're using a local staging area. - if path == sup.canonicalize_path(spack.paths.stage_path): - _use_tmp_stage = False - return None - # Ensure that any temp path is unique per user, so users don't # fight over shared temporary space. user = getpass.getuser() if user not in path: - path = os.path.join(path, user, 'spack-stage') - else: - path = os.path.join(path, 'spack-stage') + path = os.path.join(path, user) + mkdirp(path) - mkdirp(path) - _tmp_root = path + _stage_root = path - return _tmp_root + return _stage_root class Stage(object): @@ -139,6 +142,9 @@ class Stage(object): """Shared dict of all stage locks.""" stage_locks = {} + """Most staging is managed by Spack. DIYStage is one exception.""" + managed_by_spack = True + def __init__( self, url_or_fetch_strategy, name=None, mirror_path=None, keep=False, path=None, lock=True, @@ -165,6 +171,17 @@ def __init__( is deleted on exit when no exceptions are raised. Pass True to keep the stage intact even if no exceptions are raised. + + path + If provided, the stage path to use for associated builds. + + lock + True if the stage directory file lock is to be used, False + otherwise. + + search_fn + The search function that provides the fetch strategy + instance. """ # TODO: fetch/stage coupling needs to be reworked -- the logic # TODO: here is convoluted and not modular enough. @@ -184,20 +201,19 @@ def __init__( self.srcdir = None - # TODO : this uses a protected member of tempfile, but seemed the only - # TODO : way to get a temporary name besides, the temporary link name - # TODO : won't be the same as the temporary stage area in tmp_root + # TODO: This uses a protected member of tempfile, but seemed the only + # TODO: way to get a temporary name. It won't be the same as the + # TODO: temporary stage area in _stage_root. self.name = name if name is None: self.name = _stage_prefix + next(tempfile._get_candidate_names()) self.mirror_path = mirror_path - # Try to construct here a temporary name for the stage directory - # If this is a named stage, then construct a named path. + # Use the provided path or construct an optionally named stage path. if path is not None: self.path = path else: - self.path = os.path.join(spack.paths.stage_path, self.name) + self.path = os.path.join(get_stage_root(), self.name) # Flag to decide whether to delete the stage folder on exit or not self.keep = keep @@ -210,7 +226,7 @@ def __init__( if self.name not in Stage.stage_locks: sha1 = hashlib.sha1(self.name.encode('utf-8')).digest() lock_id = prefix_bits(sha1, bit_length(sys.maxsize)) - stage_lock_path = os.path.join(spack.paths.stage_path, '.lock') + stage_lock_path = os.path.join(get_stage_root(), '.lock') Stage.stage_locks[self.name] = spack.util.lock.Lock( stage_lock_path, lock_id, 1) @@ -254,44 +270,6 @@ def __exit__(self, exc_type, exc_val, exc_tb): if self._lock is not None: self._lock.release_write() - def _need_to_create_path(self): - """Makes sure nothing weird has happened since the last time we - looked at path. Returns True if path already exists and is ok. - Returns False if path needs to be created.""" - # Path doesn't exist yet. Will need to create it. - if not os.path.exists(self.path): - return True - - # Path exists but points at something else. Blow it away. - if not os.path.isdir(self.path): - os.unlink(self.path) - return True - - # Path looks ok, but need to check the target of the link. - if os.path.islink(self.path): - tmp_root = get_tmp_root() - if tmp_root is not None: - real_path = os.path.realpath(self.path) - real_tmp = os.path.realpath(tmp_root) - - # 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: - # otherwise, just unlink it and start over. - os.unlink(self.path) - return True - - else: - # If we're not tmp mode, then it's a link and we want a - # directory. - os.unlink(self.path) - return True - - return False - @property def expected_archive_files(self): """Possible archive file paths.""" @@ -455,30 +433,18 @@ def restage(self): self.fetcher.reset() def create(self): - """Creates the stage directory. - - If get_tmp_root() is None, the stage directory is created - directly under spack.paths.stage_path, otherwise this will attempt to - create a stage in a temporary directory and link it into - spack.paths.stage_path. - """ - # Create the top-level stage directory - mkdirp(spack.paths.stage_path) - remove_if_dead_link(self.path) + Ensures the top-level (config:build_stage) directory exists. + """ + # Emulate file permissions for tempfile.mkdtemp. + if not os.path.exists(self.path): + print("TLD: %s does not exist, creating it" % self.path) + mkdirp(self.path, mode=stat.S_IRWXU) + elif not os.path.isdir(self.path): + print("TLD: %s is not a directory, replacing it" % self.path) + os.remove(self.path) + mkdirp(self.path, mode=stat.S_IRWXU) - # If a tmp_root exists then create a directory there and then link it - # in the stage area, otherwise create the stage directory in self.path - if self._need_to_create_path(): - tmp_root = get_tmp_root() - if tmp_root is not None: - # tempfile.mkdtemp already sets mode 0700 - tmp_dir = tempfile.mkdtemp('', _stage_prefix, tmp_root) - tty.debug('link %s -> %s' % (self.path, tmp_dir)) - os.symlink(tmp_dir, self.path) - else: - # emulate file permissions for tempfile.mkdtemp - mkdirp(self.path, mode=stat.S_IRWXU) # Make sure we can actually do something with the stage we made. ensure_access(self.path) self.created = True @@ -562,7 +528,7 @@ def _add_to_root_stage(self): @pattern.composite(method_list=[ 'fetch', 'create', 'created', 'check', 'expand_archive', 'restage', - 'destroy', 'cache_local']) + 'destroy', 'cache_local', 'managed_by_spack']) class StageComposite: """Composite for Stage type objects. The first item in this composite is considered to be the root package, and operations that return a value are @@ -612,6 +578,9 @@ class DIYStage(object): directory naming convention. """ + """DIY staging is, by definition, not managed by Spack.""" + managed_by_spack = False + def __init__(self, path): if path is None: raise ValueError("Cannot construct DIYStage without a path.") @@ -659,7 +628,7 @@ def cache_local(self): tty.msg("Sources for DIY stages are not cached") -def ensure_access(file=spack.paths.stage_path): +def ensure_access(file): """Ensure we can access a directory and die with an error if we can't.""" if not can_access(file): tty.die("Insufficient permissions for %s" % file) @@ -667,9 +636,10 @@ def ensure_access(file=spack.paths.stage_path): def purge(): """Remove all build directories in the top-level stage path.""" - if os.path.isdir(spack.paths.stage_path): - for stage_dir in os.listdir(spack.paths.stage_path): - stage_path = os.path.join(spack.paths.stage_path, stage_dir) + root = get_stage_root() + if os.path.isdir(root): + for stage_dir in os.listdir(root): + stage_path = os.path.join(root, stage_dir) remove_linked_tree(stage_path) diff --git a/lib/spack/spack/test/cmd/location.py b/lib/spack/spack/test/cmd/location.py index 6dbc4d94d40..f48cdb75e76 100644 --- a/lib/spack/spack/test/cmd/location.py +++ b/lib/spack/spack/test/cmd/location.py @@ -11,6 +11,7 @@ import spack.environment as ev from spack.main import SpackCommand, SpackCommandError import spack.paths +import spack.stage # Everything here uses (or can use) the mock config and database. @@ -128,4 +129,4 @@ def test_location_stage_dir(mock_spec): @pytest.mark.db def test_location_stages(mock_spec): """Tests spack location --stages.""" - assert location('--stages').strip() == spack.paths.stage_path + assert location('--stages').strip() == spack.stage.get_stage_root() diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 53493eda910..f8c8e2a8bcc 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -121,14 +121,40 @@ def reset_compiler_cache(): spack.compilers._compiler_cache = {} -@pytest.fixture(scope='session', autouse=True) -def mock_stage(tmpdir_factory): - """Mocks up a fake stage directory for use by tests.""" - stage_path = spack.paths.stage_path - new_stage = str(tmpdir_factory.mktemp('mock_stage')) - spack.paths.stage_path = new_stage - yield new_stage - spack.paths.stage_path = stage_path +@pytest.fixture +def clear_stage_root(monkeypatch): + """Ensure spack.stage._stage_root is not set at test start.""" + monkeypatch.setattr(spack.stage, '_stage_root', None) + yield + + +@pytest.fixture(scope='function', autouse=True) +def mock_stage(clear_stage_root, tmpdir_factory, request): + """Establish the temporary build_stage for the mock archive.""" + # Workaround to skip mock_stage for 'nomockstage' test cases + if 'nomockstage' not in request.keywords: + new_stage = tmpdir_factory.mktemp('mock-stage') + new_stage_path = str(new_stage) + + # Set test_stage_path as the default directory to use for test stages. + current = spack.config.get('config:build_stage') + spack.config.set('config', + {'build_stage': new_stage_path}, scope='user') + + # Ensure the source directory exists + source_path = new_stage.join(spack.stage._source_path_subdir) + source_path.ensure(dir=True) + + yield new_stage_path + + spack.config.set('config', {'build_stage': current}, scope='user') + + # Clean up the test stage directory + if os.path.isdir(new_stage_path): + shutil.rmtree(new_stage_path) + else: + # Must yield a path to avoid a TypeError on test teardown + yield str(tmpdir_factory) @pytest.fixture(scope='session') @@ -138,7 +164,7 @@ def ignore_stage_files(): Used to track which leftover files in the stage have been seen. """ # to start with, ignore the .lock file at the stage root. - return set(['.lock']) + return set(['.lock', spack.stage._source_path_subdir]) def remove_whatever_it_is(path): @@ -173,17 +199,19 @@ def check_for_leftover_stage_files(request, mock_stage, ignore_stage_files): to tests that need it. """ + stage_path = mock_stage + yield files_in_stage = set() - if os.path.exists(spack.paths.stage_path): - files_in_stage = set( - os.listdir(spack.paths.stage_path)) - ignore_stage_files + if os.path.exists(stage_path): + stage_files = os.listdir(stage_path) + files_in_stage = set(stage_files) - ignore_stage_files if 'disable_clean_stage_check' in request.keywords: # clean up after tests that are expected to be dirty for f in files_in_stage: - path = os.path.join(spack.paths.stage_path, f) + path = os.path.join(stage_path, f) remove_whatever_it_is(path) else: ignore_stage_files |= files_in_stage diff --git a/lib/spack/spack/test/data/config.yaml b/lib/spack/spack/test/data/config.yaml index 2fcd10ad97c..501001ff008 100644 --- a/lib/spack/spack/test/data/config.yaml +++ b/lib/spack/spack/test/data/config.yaml @@ -5,7 +5,7 @@ config: - $spack/lib/spack/spack/test/data/templates - $spack/lib/spack/spack/test/data/templates_again build_stage: - - $tempdir + - $tempdir/spack-stage-test - /nfs/tmp2/$user - $spack/var/spack/stage source_cache: $spack/var/spack/cache diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py index 4fc360ec4ef..f70844978e3 100644 --- a/lib/spack/spack/test/llnl/util/filesystem.py +++ b/lib/spack/spack/test/llnl/util/filesystem.py @@ -107,6 +107,21 @@ def test_non_existing_dir(self, stage): assert os.path.exists('dest/sub/directory/a/b/2') + def test_parent_dir(self, stage): + """Test copying to from a parent directory.""" + + # Make sure we get the right error if we try to copy a parent into + # a descendent directory. + with pytest.raises(ValueError, matches="Cannot copy"): + with fs.working_dir(str(stage)): + fs.copy_tree('source', 'source/sub/directory') + + # Only point with this check is to make sure we don't try to perform + # the copy. + with pytest.raises(IOError, matches="No such file or directory"): + with fs.working_dir(str(stage)): + fs.copy_tree('foo/ba', 'foo/bar') + def test_symlinks_true(self, stage): """Test copying with symlink preservation.""" diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py index 5ea3ba65bc1..c3df33dc8b1 100644 --- a/lib/spack/spack/test/patch.py +++ b/lib/spack/spack/test/patch.py @@ -32,10 +32,10 @@ @pytest.fixture() -def mock_stage(tmpdir, monkeypatch): - # don't disrupt the spack install directory with tests. - mock_path = str(tmpdir) - monkeypatch.setattr(spack.paths, 'stage_path', mock_path) +def mock_patch_stage(tmpdir_factory, monkeypatch): + # Don't disrupt the spack install directory with tests. + mock_path = str(tmpdir_factory.mktemp('mock-patch-stage')) + monkeypatch.setattr(spack.stage, '_stage_root', mock_path) return mock_path @@ -52,7 +52,7 @@ def mock_stage(tmpdir, monkeypatch): '252c0af58be3d90e5dc5e0d16658434c9efa5d20a5df6c10bf72c2d77f780866', None) ]) -def test_url_patch(mock_stage, filename, sha256, archive_sha256): +def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256): # Make a patch object url = 'file://' + filename pkg = spack.repo.get('patch') @@ -61,13 +61,9 @@ def test_url_patch(mock_stage, filename, sha256, archive_sha256): # make a stage with Stage(url) as stage: # TODO: url isn't used; maybe refactor Stage - # TODO: there is probably a better way to mock this. - stage.mirror_path = mock_stage # don't disrupt the spack install - - # Fake a source path and ensure the directory exists - with working_dir(stage.path): - mkdirp(spack.stage._source_path_subdir) + stage.mirror_path = mock_patch_stage + mkdirp(stage.source_path) with working_dir(stage.source_path): # write a file to be patched with open('foo.txt', 'w') as f: diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py index 9f5e7363f53..b8b217032a1 100644 --- a/lib/spack/spack/test/stage.py +++ b/lib/spack/spack/test/stage.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) """Test that the Stage class works correctly.""" +import grp import os import collections import shutil @@ -12,7 +13,7 @@ import pytest -from llnl.util.filesystem import working_dir +from llnl.util.filesystem import mkdirp, working_dir import spack.paths import spack.stage @@ -20,6 +21,7 @@ from spack.resource import Resource from spack.stage import Stage, StageComposite, ResourceStage, DIYStage +from spack.util.path import canonicalize_path # The following values are used for common fetch and stage mocking fixtures: _archive_base = 'test-files' @@ -38,6 +40,10 @@ _include_hidden = 2 _include_extra = 3 +# Some standard unix directory that does NOT include the username +_non_user_root = os.path.join(os.path.sep, 'opt') + + # Mock fetch directories are expected to appear as follows: # # TMPDIR/ @@ -133,7 +139,7 @@ def check_destroy(stage, stage_name): assert not os.path.exists(stage_path) # tmp stage needs to remove tmp dir too. - if spack.stage._use_tmp_stage: + if not stage.managed_by_spack: target = os.path.realpath(stage_path) assert not os.path.exists(target) @@ -145,153 +151,102 @@ def check_setup(stage, stage_name, archive): # Ensure stage was created in the spack stage directory assert os.path.isdir(stage_path) - if spack.stage.get_tmp_root(): - # Check that the stage dir is really a symlink. - assert os.path.islink(stage_path) + # Make sure it points to a valid directory + target = os.path.realpath(stage_path) + assert os.path.isdir(target) + assert not os.path.islink(target) - # Make sure it points to a valid directory - target = os.path.realpath(stage_path) - assert os.path.isdir(target) - assert not os.path.islink(target) - - # Make sure the directory is in the place we asked it to - # be (see setUp, tearDown, and use_tmp) - assert target.startswith(str(archive.test_tmp_dir)) - - else: - # Make sure the stage path is NOT a link for a non-tmp stage - assert not os.path.islink(stage_path) + # Make sure the directory is in the place we asked it to + # be (see setUp, tearDown, and use_tmp) + assert target.startswith(str(archive.stage_path)) def get_stage_path(stage, stage_name): """Figure out where a stage should be living. This depends on whether it's named. """ + stage_path = spack.stage.get_stage_root() if stage_name is not None: # If it is a named stage, we know where the stage should be - return os.path.join(spack.paths.stage_path, stage_name) + return os.path.join(stage_path, stage_name) else: # If it's unnamed, ensure that we ran mkdtemp in the right spot. assert stage.path is not None - assert stage.path.startswith(spack.paths.stage_path) + assert stage.path.startswith(stage_path) return stage.path @pytest.fixture -def no_path_for_stage(monkeypatch): - """Ensure there is no accessible path for staging.""" - def _no_stage_path(paths): - return None - - monkeypatch.setattr(spack.stage, '_first_accessible_path', _no_stage_path) - yield - - -@pytest.fixture -def no_tmp_root_stage(monkeypatch): - """ - Disable use of a temporary root for staging. - - Note that it can be important for other tests that the previous settings be - restored when the test case is over. - """ - monkeypatch.setattr(spack.stage, '_tmp_root', None) - monkeypatch.setattr(spack.stage, '_use_tmp_stage', False) - yield - - -@pytest.fixture -def non_user_path_for_stage(config): - """ - Use a non-user path for staging. - - Note that it can be important for other tests that the previous settings be - restored when the test case is over. - """ +def bad_stage_path(): + """Temporarily ensure there is no accessible path for staging.""" current = spack.config.get('config:build_stage') - spack.config.set('config', {'build_stage': ['/var/spack/non-path']}, - scope='user') + spack.config.set('config', {'build_stage': '/no/such/path'}, scope='user') yield spack.config.set('config', {'build_stage': current}, scope='user') @pytest.fixture -def stage_path_for_stage(config): - """ - Use the basic stage_path for staging. +def non_user_path_for_stage(monkeypatch): + """Temporarily use a Linux-standard non-user path for staging. """ + def _can_access(path, perms): + return True - Note that it can be important for other tests that the previous settings be - restored when the test case is over. - """ current = spack.config.get('config:build_stage') - spack.config.set('config', - {'build_stage': spack.paths.stage_path}, scope='user') + spack.config.set('config', {'build_stage': [_non_user_root]}, scope='user') + monkeypatch.setattr(os, 'access', _can_access) yield spack.config.set('config', {'build_stage': current}, scope='user') @pytest.fixture -def tmp_path_for_stage(tmpdir, config): +def instance_path_for_stage(): """ - Use a built-in, temporary, test directory for staging. + Temporarily use the "traditional" spack instance stage path for staging. + + Note that it can be important for other tests that the previous settings be + restored when the test case is over. + """ + current = spack.config.get('config:build_stage') + base = canonicalize_path(os.path.join('$spack', 'test-stage')) + mkdirp(base) + path = tempfile.mkdtemp(dir=base) + spack.config.set('config', {'build_stage': path}, scope='user') + yield + spack.config.set('config', {'build_stage': current}, scope='user') + shutil.rmtree(base) + + +@pytest.fixture +def tmp_path_for_stage(tmpdir): + """ + Use a temporary test directory for staging. Note that it can be important for other tests that the previous settings be restored when the test case is over. """ current = spack.config.get('config:build_stage') spack.config.set('config', {'build_stage': [str(tmpdir)]}, scope='user') - yield + yield tmpdir spack.config.set('config', {'build_stage': current}, scope='user') @pytest.fixture -def tmp_root_stage(monkeypatch): - """ - Enable use of a temporary root for staging. - - Note that it can be important for other tests that the previous settings be - restored when the test case is over. - """ - monkeypatch.setattr(spack.stage, '_tmp_root', None) - monkeypatch.setattr(spack.stage, '_use_tmp_stage', True) - yield - - -@pytest.fixture -def tmpdir_for_stage(config, mock_stage_archive): - """ - Use the mock_stage_archive's temporary directory for staging. - - Note that it can be important for other tests that the previous settings be - restored when the test case is over. - """ - archive = mock_stage_archive() - current = spack.config.get('config:build_stage') - spack.config.set( - 'config', - {'build_stage': [str(archive.test_tmp_dir)]}, - scope='user') - yield - spack.config.set('config', {'build_stage': current}, scope='user') - - -@pytest.fixture -def tmp_build_stage_dir(tmpdir, config): +def tmp_build_stage_dir(tmpdir): """Establish the temporary build_stage for the mock archive.""" - test_tmp_path = tmpdir.join('tmp') + test_stage_path = tmpdir.join('stage') - # Set test_tmp_path as the default test directory to use for stages. + # Set test_stage_path as the default directory to use for test stages. current = spack.config.get('config:build_stage') spack.config.set('config', - {'build_stage': [str(test_tmp_path)]}, scope='user') + {'build_stage': [str(test_stage_path)]}, scope='user') - yield (tmpdir, test_tmp_path) + yield (tmpdir, test_stage_path) spack.config.set('config', {'build_stage': current}, scope='user') @pytest.fixture -def mock_stage_archive(tmp_build_stage_dir, tmp_root_stage, request): +def mock_stage_archive(clear_stage_root, tmp_build_stage_dir, request): """ Create the directories and files for the staged mock archive. @@ -301,7 +256,7 @@ def mock_stage_archive(tmp_build_stage_dir, tmp_root_stage, request): # Mock up a stage area that looks like this: # # tmpdir/ test_files_dir - # tmp/ test_tmp_path (where stage should be) + # stage/ test_stage_path (where stage should be) # <_archive_base>/ archive_dir_path # <_readme_fn> Optional test_readme (contains _readme_contents) # <_extra_fn> Optional extra file (contains _extra_contents) @@ -309,8 +264,8 @@ def mock_stage_archive(tmp_build_stage_dir, tmp_root_stage, request): # <_archive_fn> archive_url = file:///path/to/<_archive_fn> # def create_stage_archive(expected_file_list=[_include_readme]): - tmpdir, test_tmp_path = tmp_build_stage_dir - test_tmp_path.ensure(dir=True) + tmpdir, test_stage_path = tmp_build_stage_dir + test_stage_path.ensure(dir=True) # Create the archive directory and associated file archive_dir = tmpdir.join(_archive_base) @@ -349,10 +304,10 @@ def create_stage_archive(expected_file_list=[_include_readme]): tar(*tar_args) Archive = collections.namedtuple( - 'Archive', ['url', 'tmpdir', 'test_tmp_dir', 'archive_dir'] + 'Archive', ['url', 'tmpdir', 'stage_path', 'archive_dir'] ) return Archive(url=archive_url, tmpdir=tmpdir, - test_tmp_dir=test_tmp_path, archive_dir=archive_dir) + stage_path=test_stage_path, archive_dir=archive_dir) return create_stage_archive @@ -368,22 +323,33 @@ def mock_noexpand_resource(tmpdir): @pytest.fixture def mock_expand_resource(tmpdir): """Sets up an expandable resource in tmpdir prior to staging.""" - resource_dir = tmpdir.join('resource-expand') + # Mock up an expandable resource: + # + # tmpdir/ test_files_dir + # resource-expand/ resource source dir + # resource-file.txt resource contents (contains 'test content') + # resource.tar.gz archive of resource content + # + subdir = 'resource-expand' + resource_dir = tmpdir.join(subdir) + resource_dir.ensure(dir=True) + archive_name = 'resource.tar.gz' archive = tmpdir.join(archive_name) archive_url = 'file://' + str(archive) - test_file = resource_dir.join('resource-file.txt') - resource_dir.ensure(dir=True) + + filename = 'resource-file.txt' + test_file = resource_dir.join(filename) test_file.write('test content\n') with tmpdir.as_cwd(): tar = spack.util.executable.which('tar', required=True) - tar('czf', str(archive_name), 'resource-expand') + tar('czf', str(archive_name), subdir) MockResource = collections.namedtuple( 'MockResource', ['url', 'files']) - return MockResource(archive_url, ['resource-file.txt']) + return MockResource(archive_url, [filename]) @pytest.fixture @@ -404,7 +370,7 @@ def composite_stage_with_expanding_resource( resource_stage = ResourceStage( test_resource_fetcher, root_stage, test_resource) composite_stage.append(resource_stage) - return composite_stage, root_stage, resource_stage + return composite_stage, root_stage, resource_stage, mock_expand_resource @pytest.fixture @@ -445,7 +411,6 @@ class TestStage(object): stage_name = 'spack-test-stage' - @pytest.mark.usefixtures('tmpdir_for_stage') def test_setup_and_destroy_name_with_tmp(self, mock_stage_archive): archive = mock_stage_archive() with Stage(archive.url, name=self.stage_name) as stage: @@ -458,14 +423,12 @@ def test_setup_and_destroy_name_without_tmp(self, mock_stage_archive): check_setup(stage, self.stage_name, archive) check_destroy(stage, self.stage_name) - @pytest.mark.usefixtures('tmpdir_for_stage') def test_setup_and_destroy_no_name_with_tmp(self, mock_stage_archive): archive = mock_stage_archive() with Stage(archive.url) as stage: check_setup(stage, None, archive) check_destroy(stage, None) - @pytest.mark.usefixtures('tmpdir_for_stage') def test_noexpand_stage_file( self, mock_stage_archive, mock_noexpand_resource): """When creating a stage with a nonexpanding URL, the 'archive_file' @@ -479,7 +442,6 @@ def test_noexpand_stage_file( assert os.path.exists(stage.archive_file) @pytest.mark.disable_clean_stage_check - @pytest.mark.usefixtures('tmpdir_for_stage') def test_composite_stage_with_noexpand_resource( self, mock_stage_archive, mock_noexpand_resource): archive = mock_stage_archive() @@ -505,12 +467,10 @@ def test_composite_stage_with_noexpand_resource( os.path.join(composite_stage.source_path, resource_dst_name)) @pytest.mark.disable_clean_stage_check - @pytest.mark.usefixtures('tmpdir_for_stage') def test_composite_stage_with_expand_resource( - self, mock_stage_archive, mock_expand_resource, - composite_stage_with_expanding_resource): + self, composite_stage_with_expanding_resource): - composite_stage, root_stage, resource_stage = ( + composite_stage, root_stage, resource_stage, mock_resource = ( composite_stage_with_expanding_resource) composite_stage.create() @@ -519,23 +479,24 @@ def test_composite_stage_with_expand_resource( assert composite_stage.expanded # Archive is expanded - for fname in mock_expand_resource.files: + for fname in mock_resource.files: file_path = os.path.join( root_stage.source_path, 'resource-dir', fname) assert os.path.exists(file_path) + # Perform a little cleanup + shutil.rmtree(root_stage.path) + @pytest.mark.disable_clean_stage_check - @pytest.mark.usefixtures('tmpdir_for_stage') def test_composite_stage_with_expand_resource_default_placement( - self, mock_stage_archive, mock_expand_resource, - composite_stage_with_expanding_resource): + self, composite_stage_with_expanding_resource): """For a resource which refers to a compressed archive which expands to a directory, check that by default the resource is placed in the source_path of the root stage with the name of the decompressed directory. """ - composite_stage, root_stage, resource_stage = ( + composite_stage, root_stage, resource_stage, mock_resource = ( composite_stage_with_expanding_resource) resource_stage.resource.placement = None @@ -544,11 +505,14 @@ def test_composite_stage_with_expand_resource_default_placement( composite_stage.fetch() composite_stage.expand_archive() - for fname in mock_expand_resource.files: + for fname in mock_resource.files: file_path = os.path.join( root_stage.source_path, 'resource-expand', fname) assert os.path.exists(file_path) + # Perform a little cleanup + shutil.rmtree(root_stage.path) + def test_setup_and_destroy_no_name_without_tmp(self, mock_stage_archive): archive = mock_stage_archive() with Stage(archive.url) as stage: @@ -710,42 +674,108 @@ def test_source_path_available(self, mock_stage_archive): assert source_path.endswith(spack.stage._source_path_subdir) assert not os.path.exists(source_path) - def test_first_accessible_path_error(self): - """Test _first_accessible_path handling of an OSError.""" - with tempfile.NamedTemporaryFile() as _file: - assert spack.stage._first_accessible_path([_file.name]) is None + def test_first_accessible_path(self, tmpdir): + """Test _first_accessible_path names.""" + spack_dir = tmpdir.join('spack-test-fap') + name = str(spack_dir) + files = [os.path.join(os.path.sep, 'no', 'such', 'path'), name] - def test_get_tmp_root_no_use(self, no_tmp_root_stage): - """Ensure not using tmp root results in no path.""" - assert spack.stage.get_tmp_root() is None + # Ensure the tmpdir path is returned since the user should have access + path = spack.stage._first_accessible_path(files) + assert path == name + assert os.path.isdir(path) - def test_get_tmp_root_no_stage_path(self, tmp_root_stage, - no_path_for_stage): - """Ensure using tmp root with no stage path raises StageError.""" + # Ensure an existing path is returned + spack_subdir = spack_dir.join('existing').ensure(dir=True) + subdir = str(spack_subdir) + path = spack.stage._first_accessible_path([subdir]) + assert path == subdir + + # Cleanup + shutil.rmtree(str(name)) + + def test_first_accessible_perms(self, tmpdir): + """Test _first_accessible_path permissions.""" + name = str(tmpdir.join('first', 'path')) + path = spack.stage._first_accessible_path([name]) + + # Ensure the non-existent path was created + assert path == name + assert os.path.isdir(path) + + # Ensure the non-existent subdirectories have their parent's perms + prefix = str(tmpdir) + status = os.stat(prefix) + group = grp.getgrgid(status.st_gid)[0] + parts = path[len(prefix):].split(os.path.sep) + for part in parts: + prefix = os.path.join(prefix, part) + prefix_status = os.stat(prefix) + assert group == grp.getgrgid(os.stat(prefix).st_gid)[0] + assert status.st_mode == prefix_status.st_mode + + # Cleanup + shutil.rmtree(os.path.dirname(name)) + + def test_get_stage_root_bad_path(self, clear_stage_root, bad_stage_path): + """Ensure an invalid stage path root raises a StageError.""" with pytest.raises(spack.stage.StageError, match="No accessible stage paths in"): - spack.stage.get_tmp_root() + assert spack.stage.get_stage_root() is None - def test_get_tmp_root_non_user_path(self, tmp_root_stage, - non_user_path_for_stage): - """Ensure build_stage of tmp root with non-user path includes user.""" - path = spack.stage.get_tmp_root() - assert path.endswith(os.path.join(getpass.getuser(), 'spack-stage')) + # Make sure the cached stage path values are unchanged. + assert spack.stage._stage_root is None - def test_get_tmp_root_use(self, tmp_root_stage, tmp_path_for_stage): - """Ensure build_stage of tmp root provides has right ancestors.""" - path = spack.stage.get_tmp_root() - shutil.rmtree(path) - assert path.rfind('test_get_tmp_root_use') > 0 - assert path.endswith('spack-stage') + def test_get_stage_root_non_user_path(self, clear_stage_root, + non_user_path_for_stage): + """Ensure a non-user stage root includes the username.""" + # The challenge here is whether the user has access to the standard + # non-user path. If not, the path should still appear in the error. + try: + path = spack.stage.get_stage_root() + assert getpass.getuser() in path.split(os.path.sep) - def test_get_tmp_root_stage_path(self, tmp_root_stage, - stage_path_for_stage): - """ - Ensure build_stage of tmp root with stage_path means use local path. - """ - assert spack.stage.get_tmp_root() is None - assert not spack.stage._use_tmp_stage + # Make sure the cached stage path values are changed appropriately. + assert spack.stage._stage_root == path + except OSError as e: + expected = os.path.join(_non_user_root, getpass.getuser()) + assert expected in str(e) + + # Make sure the cached stage path values are unchanged. + assert spack.stage._stage_root is None + + def test_get_stage_root_tmp(self, clear_stage_root, tmp_path_for_stage): + """Ensure a temp path stage root is a suitable temp path.""" + assert spack.stage._stage_root is None + + tmpdir = tmp_path_for_stage + path = spack.stage.get_stage_root() + assert path == str(tmpdir) + assert 'test_get_stage_root_tmp' in path + + # Make sure the cached stage path values are changed appropriately. + assert spack.stage._stage_root == path + + # Add then purge a couple of directories + dir1 = tmpdir.join('dir1') + dir1.ensure(dir=True) + dir2 = tmpdir.join('dir2') + dir2.ensure(dir=True) + + spack.stage.purge() + assert not os.path.exists(str(dir1)) + assert not os.path.exists(str(dir2)) + + def test_get_stage_root_in_spack(self, clear_stage_root, + instance_path_for_stage): + """Ensure an instance path stage root is a suitable path.""" + assert spack.stage._stage_root is None + + path = spack.stage.get_stage_root() + assert 'spack' in path.split(os.path.sep) + + # Make sure the cached stage path values are changed appropriately. + assert spack.stage._stage_root == path def test_stage_constructor_no_fetcher(self): """Ensure Stage constructor with no URL or fetch strategy fails.""" @@ -814,3 +844,41 @@ def test_diystage_preserve_file(self, tmpdir): assert os.path.isfile(readmefn) with open(readmefn) as _file: _file.read() == _readme_contents + + +@pytest.fixture +def tmp_build_stage_nondir(tmpdir): + """Establish the temporary build_stage pointing to non-directory.""" + test_stage_path = tmpdir.join('stage', 'afile') + test_stage_path.ensure(dir=False) + + # Set test_stage_path as the default directory to use for test stages. + current = spack.config.get('config:build_stage') + stage_dir = os.path.dirname(str(test_stage_path)) + spack.config.set('config', {'build_stage': [stage_dir]}, scope='user') + + yield test_stage_path + + spack.config.set('config', {'build_stage': current}, scope='user') + + +def test_stage_create_replace_path(tmp_build_stage_dir): + """Ensure stage creation replaces a non-directory path.""" + _, test_stage_path = tmp_build_stage_dir + test_stage_path.ensure(dir=True) + + nondir = test_stage_path.join('afile') + nondir.ensure(dir=False) + path = str(nondir) + + stage = Stage(path, name='') + stage.create() # Should ensure the path is converted to a dir + + assert os.path.isdir(stage.path) + + +def test_cannot_access(): + """Ensure can_access dies with the expected error.""" + with pytest.raises(SystemExit, matches='Insufficient permissions'): + # It's far more portable to use a non-existent filename. + spack.stage.ensure_access('/no/such/file') diff --git a/lib/spack/spack/test/util/spack_lock_wrapper.py b/lib/spack/spack/test/util/spack_lock_wrapper.py index 1a0e41c7f6a..c64af82e379 100644 --- a/lib/spack/spack/test/util/spack_lock_wrapper.py +++ b/lib/spack/spack/test/util/spack_lock_wrapper.py @@ -38,6 +38,8 @@ def test_disable_locking(tmpdir): assert old_value == spack.config.get('config:locks') +# "Disable" mock_stage fixture to avoid subdir permissions issues on cleanup. +@pytest.mark.nomockstage def test_lock_checks_user(tmpdir): """Ensure lock checks work with a self-owned, self-group repo.""" uid = os.getuid() @@ -70,6 +72,8 @@ def test_lock_checks_user(tmpdir): lk.check_lock_safety(path) +# "Disable" mock_stage fixture to avoid subdir permissions issues on cleanup. +@pytest.mark.nomockstage def test_lock_checks_group(tmpdir): """Ensure lock checks work with a self-owned, non-self-group repo.""" uid = os.getuid()