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

@@ -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.
+  # Recommended options are given below.
-  # You can use $tempdir to refer to the system default temp directory
-  # (as returned by tempfile.gettempdir()).
   #
-  # A value of $spack/var/spack/stage indicates that Spack should run
+  # Builds can be faster in temporary directories on some (e.g., HPC) systems.
-  # builds directly inside its install directory without staging them in
+  # 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
+    - $tempdir/spack-stage
-    - $spack/var/spack/stage
 
 
   # Cache directory for already downloaded source tarballs and archived

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
+building in a temporary file system is faster.  Usually, there is also more
-in the home directory.  Usually, there is also more space available in
+space available in the temporary location than in the home directory.  If the
-the temporary location than in the home directory. So, Spack tries to
+username is not already in the path, Spack will also append the value of
-create build stages in temporary space.
+`$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
+    - $tempdir/spack-stage
-    - $spack/var/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
+``build_stage``.  After the package is successfully installed, Spack deletes
-``$spack/var/spack/stage``. This is used to track the temporary
-directory.  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
 <cmd-spack-clean>`.
 
 .. note::
 
-   The last item in the list is ``$spack/var/spack/stage``.  If this is the
+   The build will fail if there is no writable directory in the ``build_stage``
-   only writable directory in the ``build_stage`` list, Spack will build
+   list.
-   *directly* in ``$spack/var/spack/stage`` and will not link to temporary
-   space.
 
 --------------------
 ``source_cache``

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)
+    for s, d in traverse_tree(abs_src, abs_dest, order='pre',
-    dest = os.path.abspath(dest)
-
-    for s, d in traverse_tree(src, 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))

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)

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))

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',
     }
 }
 

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(
+        if restage and self.stage.managed_by_spack:
-            spack.paths.stage_path)
-        if restage and stage_is_managed_in_spack:
             self.stage.destroy()
             self.stage.create()
 

lib/spack/spack/patch.py

@@ -192,8 +192,7 @@ def fetch(self, stage):
             fetch_digest = self.archive_sha256
 
         fetcher = fs.URLFetchStrategy(self.url, fetch_digest)
-        mirror = os.path.join(
+        mirror = os.path.join(os.path.dirname(stage.mirror_path),
-            os.path.dirname(stage.mirror_path),
                               os.path.basename(self.url))
 
         self.stage = spack.stage.Stage(fetcher, mirror_path=mirror)

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")
 

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 can_access(path):
-            if not can_access(path):
-                continue
-
-            # return it if successful.
                     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
+# Cached stage path root
-_tmp_root = None
+_stage_root = None
-_use_tmp_stage = True
 
 
-def get_tmp_root():
+def get_stage_root():
-    global _tmp_root, _use_tmp_stage
+    global _stage_root
 
-    if not _use_tmp_stage:
+    if _stage_root is None:
-        return None
-
-    if _tmp_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')
+            path = os.path.join(path, user)
-        else:
-            path = os.path.join(path, 'spack-stage')
-
             mkdirp(path)
-        _tmp_root = path
 
-    return _tmp_root
+        _stage_root = path
+
+    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: This uses a protected member of tempfile, but seemed the only
-        # TODO : way to get a temporary name besides, the temporary link name
+        # TODO: way to get a temporary name.  It won't be the same as the
-        # TODO : won't be the same as the temporary stage area in tmp_root
+        # 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
+        # Use the provided path or construct an optionally named stage path.
-        # If this is a named stage, then construct a named 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
+        Ensures the top-level (config:build_stage) directory exists.
-        mkdirp(spack.paths.stage_path)
+        """
-        remove_if_dead_link(self.path)
+        # Emulate file permissions for tempfile.mkdtemp.
-
+        if not os.path.exists(self.path):
-        # If a tmp_root exists then create a directory there and then link it
+            print("TLD: %s does not exist, creating it" % self.path)
-        # 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)
+        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)
+
         # 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):
+    root = get_stage_root()
-        for stage_dir in os.listdir(spack.paths.stage_path):
+    if os.path.isdir(root):
-            stage_path = os.path.join(spack.paths.stage_path, stage_dir)
+        for stage_dir in os.listdir(root):
+            stage_path = os.path.join(root, stage_dir)
             remove_linked_tree(stage_path)
 
 

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()

lib/spack/spack/test/conftest.py

@@ -121,14 +121,40 @@ def reset_compiler_cache():
     spack.compilers._compiler_cache = {}
 
 
-@pytest.fixture(scope='session', autouse=True)
+@pytest.fixture
-def mock_stage(tmpdir_factory):
+def clear_stage_root(monkeypatch):
-    """Mocks up a fake stage directory for use by tests."""
+    """Ensure spack.stage._stage_root is not set at test start."""
-    stage_path = spack.paths.stage_path
+    monkeypatch.setattr(spack.stage, '_stage_root', None)
-    new_stage = str(tmpdir_factory.mktemp('mock_stage'))
+    yield
-    spack.paths.stage_path = new_stage
+
-    yield new_stage
+
-    spack.paths.stage_path = stage_path
+@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):
+    if os.path.exists(stage_path):
-        files_in_stage = set(
+        stage_files = os.listdir(stage_path)
-            os.listdir(spack.paths.stage_path)) - ignore_stage_files
+        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

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

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."""
 

lib/spack/spack/test/patch.py

@@ -32,10 +32,10 @@
 
 
 @pytest.fixture()
-def mock_stage(tmpdir, monkeypatch):
+def mock_patch_stage(tmpdir_factory, monkeypatch):
-    # don't disrupt the spack install directory with tests.
+    # Don't disrupt the spack install directory with tests.
-    mock_path = str(tmpdir)
+    mock_path = str(tmpdir_factory.mktemp('mock-patch-stage'))
-    monkeypatch.setattr(spack.paths, 'stage_path', mock_path)
+    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_patch_stage
-        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)
 
+        mkdirp(stage.source_path)
         with working_dir(stage.source_path):
             # write a file to be patched
             with open('foo.txt', 'w') as f:

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,10 +151,6 @@ 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)
@@ -156,142 +158,95 @@ def check_setup(stage, stage_name, archive):
 
     # 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))
+    assert target.startswith(str(archive.stage_path))
-
-    else:
-        # Make sure the stage path is NOT a link for a non-tmp stage
-        assert not os.path.islink(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):
+def bad_stage_path():
-    """Ensure there is no accessible path for staging."""
+    """Temporarily 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.
-    """
     current = spack.config.get('config:build_stage')
-    spack.config.set('config', {'build_stage': ['/var/spack/non-path']},
+    spack.config.set('config', {'build_stage': '/no/such/path'}, scope='user')
-                     scope='user')
     yield
     spack.config.set('config', {'build_stage': current}, scope='user')
 
 
 @pytest.fixture
-def stage_path_for_stage(config):
+def non_user_path_for_stage(monkeypatch):
-    """
+    """Temporarily use a Linux-standard non-user path for staging. """
-    Use the basic stage_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',
+    spack.config.set('config', {'build_stage': [_non_user_root]}, scope='user')
-                     {'build_stage': spack.paths.stage_path}, 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):
+def tmp_build_stage_dir(tmpdir):
-    """
-    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):
     """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
+        tmpdir, test_stage_path = tmp_build_stage_dir
-        test_tmp_path.ensure(dir=True)
+        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,
+            self, composite_stage_with_expanding_resource):
-            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,
+            self, composite_stage_with_expanding_resource):
-            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):
+    def test_first_accessible_path(self, tmpdir):
-        """Test _first_accessible_path handling of an OSError."""
+        """Test _first_accessible_path names."""
-        with tempfile.NamedTemporaryFile() as _file:
+        spack_dir = tmpdir.join('spack-test-fap')
-            assert spack.stage._first_accessible_path([_file.name]) is None
+        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 the tmpdir path is returned since the user should have access
-        """Ensure not using tmp root results in no path."""
+        path = spack.stage._first_accessible_path(files)
-        assert spack.stage.get_tmp_root() is None
+        assert path == name
+        assert os.path.isdir(path)
 
-    def test_get_tmp_root_no_stage_path(self, tmp_root_stage,
+        # Ensure an existing path is returned
-                                        no_path_for_stage):
+        spack_subdir = spack_dir.join('existing').ensure(dir=True)
-        """Ensure using tmp root with no stage path raises StageError."""
+        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,
+        # Make sure the cached stage path values are unchanged.
+        assert spack.stage._stage_root is None
+
+    def test_get_stage_root_non_user_path(self, clear_stage_root,
                                           non_user_path_for_stage):
-        """Ensure build_stage of tmp root with non-user path includes user."""
+        """Ensure a non-user stage root includes the username."""
-        path = spack.stage.get_tmp_root()
+        # The challenge here is whether the user has access to the standard
-        assert path.endswith(os.path.join(getpass.getuser(), 'spack-stage'))
+        # 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_use(self, tmp_root_stage, tmp_path_for_stage):
+            # Make sure the cached stage path values are changed appropriately.
-        """Ensure build_stage of tmp root provides has right ancestors."""
+            assert spack.stage._stage_root == path
-        path = spack.stage.get_tmp_root()
+        except OSError as e:
-        shutil.rmtree(path)
+            expected = os.path.join(_non_user_root, getpass.getuser())
-        assert path.rfind('test_get_tmp_root_use') > 0
+            assert expected in str(e)
-        assert path.endswith('spack-stage')
 
-    def test_get_tmp_root_stage_path(self, tmp_root_stage,
+            # Make sure the cached stage path values are unchanged.
-                                     stage_path_for_stage):
+            assert spack.stage._stage_root is None
-        """
+
-        Ensure build_stage of tmp root with stage_path means use local path.
+    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.get_tmp_root() is None
+        assert spack.stage._stage_root is None
-        assert not spack.stage._use_tmp_stage
+
+        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')

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()