tests: cleanup config:build_stage handling (fixes #12651, #12798)

lib/spack/spack/config.py

@@ -110,6 +110,9 @@
 #: this is shorter and more readable than listing all choices
 scopes_metavar = '{defaults,system,site,user}[/PLATFORM]'
 
+#: Base name for the (internal) overrides scope.
+overrides_base_name = 'overrides-'
+
 
 def first_existing(dictionary, keys):
     """Get the value of the first key in keys that is in the dictionary."""
@@ -549,11 +552,11 @@ def override(path_or_scope, value=None):
     an internal config scope for it and push/pop that scope.
 
     """
-    base_name = 'overrides-'
     if isinstance(path_or_scope, ConfigScope):
         overrides = path_or_scope
         config.push_scope(path_or_scope)
     else:
+        base_name = overrides_base_name
         # Ensure the new override gets a unique scope name
         current_overrides = [s.name for s in
                              config.matching_scopes(r'^{0}'.format(base_name))]

lib/spack/spack/stage.py

@@ -91,18 +91,29 @@ def _first_accessible_path(paths):
 
 
 def _resolve_paths(candidates):
-    """Resolve paths, removing extra $user from $tempdir if needed."""
+    """
+    Resolve candidate paths and make user-related adjustments.
+
+    Adjustments involve removing extra $user from $tempdir if $tempdir includes
+    $user and appending $user if it is not present in the path.
+    """
     temp_path = sup.canonicalize_path('$tempdir')
-    tmp_has_usr = getpass.getuser() in temp_path.split(os.path.sep)
+    user = getpass.getuser()
+    tmp_has_usr = user in temp_path.split(os.path.sep)
 
     paths = []
     for path in candidates:
-        # First remove the extra `$user` node from a `$tempdir/$user` entry
+        # Remove the extra `$user` node from a `$tempdir/$user` entry for
-        # for hosts that automatically append `$user` to `$tempdir`.
+        # hosts that automatically append `$user` to `$tempdir`.
         if path.startswith(os.path.join('$tempdir', '$user')) and tmp_has_usr:
             path = os.path.join('$tempdir', path[15:])
 
-        paths.append(sup.canonicalize_path(path))
+        # Ensure the path is unique per user.
+        can_path = sup.canonicalize_path(path)
+        if user not in can_path:
+            can_path = os.path.join(can_path, user)
+
+        paths.append(can_path)
 
     return paths
 
@@ -125,14 +136,6 @@ def get_stage_root():
             raise StageError("No accessible stage paths in:",
                              ' '.join(resolved_candidates))
 
-        # Ensure that path is unique per user in an attempt to avoid
-        # conflicts in shared temporary spaces.  Emulate permissions from
-        # `tempfile.mkdtemp`.
-        user = getpass.getuser()
-        if user not in path:
-            path = os.path.join(path, user)
-            mkdirp(path, mode=stat.S_IRWXU)
-
         _stage_root = path
 
     return _stage_root

lib/spack/spack/test/config.py

@@ -627,14 +627,13 @@ def test_add_command_line_scopes(tmpdir, mutable_config):
     spack.config._add_command_line_scopes(mutable_config, [str(tmpdir)])
 
 
-@pytest.mark.nomockstage
 def test_nested_override():
     """Ensure proper scope naming of nested overrides."""
-    # WARNING: Base name must match that used in `config.py`s `override()`.
+    base_name = spack.config.overrides_base_name
-    base_name = 'overrides-'
 
     def _check_scopes(num_expected, debug_values):
-        scope_names = [s.name for s in spack.config.config.scopes.values()]
+        scope_names = [s.name for s in spack.config.config.scopes.values() if
+                       s.name.startswith(base_name)]
 
         for i in range(num_expected):
             name = '{0}{1}'.format(base_name, i)
@@ -651,11 +650,9 @@ def _check_scopes(num_expected, debug_values):
         _check_scopes(1, [True])
 
 
-@pytest.mark.nomockstage
 def test_alternate_override(monkeypatch):
     """Ensure proper scope naming of override when conflict present."""
-    # WARNING: Base name must match that used in `config.py`s `override()`.
+    base_name = spack.config.overrides_base_name
-    base_name = 'overrides-'
 
     def _matching_scopes(regexpr):
         return [spack.config.InternalConfigScope('{0}1'.format(base_name))]

lib/spack/spack/test/conftest.py

@@ -17,7 +17,7 @@
 import pytest
 import ruamel.yaml as yaml
 
-from llnl.util.filesystem import remove_linked_tree
+from llnl.util.filesystem import mkdirp, remove_linked_tree
 
 import spack.architecture
 import spack.compilers
@@ -122,34 +122,27 @@ def reset_compiler_cache():
     spack.compilers._compiler_cache = {}
 
 
-@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):
+def mock_stage(tmpdir_factory, monkeypatch, request):
     """Establish the temporary build_stage for the mock archive."""
-    # Workaround to skip mock_stage for 'nomockstage' test cases
+    # The approach with this autouse fixture is to set the stage root
+    # instead of using spack.config.override() to avoid configuration
+    # conflicts with dozens of tests that rely on other configuration
+    # fixtures, such as config.
     if 'nomockstage' not in request.keywords:
+        # Set the build stage to the requested path
         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.
+        # Ensure the source directory exists within the new stage path
-        current = spack.config.get('config:build_stage')
+        source_path = os.path.join(new_stage_path,
-        spack.config.set('config',
+                                   spack.stage._source_path_subdir)
-                         {'build_stage': new_stage_path}, scope='user')
+        mkdirp(source_path)
 
-        # Ensure the source directory exists
+        monkeypatch.setattr(spack.stage, '_stage_root', new_stage_path)
-        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)

lib/spack/spack/test/stage.py

@@ -13,7 +13,7 @@
 
 import pytest
 
-from llnl.util.filesystem import mkdirp, working_dir
+from llnl.util.filesystem import mkdirp, touch, working_dir
 
 import spack.paths
 import spack.stage
@@ -40,9 +40,6 @@
 _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:
 #
@@ -70,6 +67,13 @@
 #
 
 
+@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
+
+
 def check_expand_archive(stage, stage_name, expected_file_list):
     """
     Ensure the expanded archive directory contains the expected structure and
@@ -176,83 +180,22 @@ def get_stage_path(stage, stage_name):
         return stage.path
 
 
+# TODO: Revisit use of the following fixture (and potentially leveraging
+#       the `mock_stage` path in `mock_stage_archive`) per discussions in
+#       #12857.  See also #13065.
 @pytest.fixture
-def bad_stage_path():
+def tmp_build_stage_dir(tmpdir, clear_stage_root):
-    """Temporarily ensure there is no accessible path for staging."""
+    """Use a temporary test directory for the stage root."""
-    current = spack.config.get('config:build_stage')
+    test_path = str(tmpdir.join('stage'))
-    spack.config.set('config', {'build_stage': '/no/such/path'}, scope='user')
+    with spack.config.override('config:build_stage', test_path):
-    yield
+        yield tmpdir, spack.stage.get_stage_root()
-    spack.config.set('config', {'build_stage': current}, scope='user')
+
+    shutil.rmtree(test_path)
 
 
 @pytest.fixture
-def non_user_path_for_stage(monkeypatch):
+def mock_stage_archive(tmp_build_stage_dir):
-    """Temporarily use a Linux-standard non-user path for staging. """
+    """Create the directories and files for the staged mock archive."""
-    def _can_access(path, perms):
-        return True
-
-    current = spack.config.get('config:build_stage')
-    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 instance_path_for_stage():
-    """
-    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 tmpdir
-    spack.config.set('config', {'build_stage': current}, scope='user')
-
-
-@pytest.fixture
-def tmp_build_stage_dir(tmpdir):
-    """Establish the temporary build_stage for the mock archive."""
-    test_stage_path = tmpdir.join('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': [str(test_stage_path)]}, scope='user')
-
-    yield (tmpdir, test_stage_path)
-
-    spack.config.set('config', {'build_stage': current}, scope='user')
-
-
-@pytest.fixture
-def mock_stage_archive(clear_stage_root, tmp_build_stage_dir, request):
-    """
-    Create the directories and files for the staged mock archive.
-
-    Note that it can be important for other tests that the previous settings be
-    restored when the test case is over.
-    """
     # Mock up a stage area that looks like this:
     #
     # tmpdir/                test_files_dir
@@ -265,7 +208,7 @@ def mock_stage_archive(clear_stage_root, tmp_build_stage_dir, request):
     #
     def create_stage_archive(expected_file_list=[_include_readme]):
         tmpdir, test_stage_path = tmp_build_stage_dir
-        test_stage_path.ensure(dir=True)
+        mkdirp(test_stage_path)
 
         # Create the archive directory and associated file
         archive_dir = tmpdir.join(_archive_base)
@@ -733,82 +676,87 @@ def test_resolve_paths(self):
 
         assert spack.stage._resolve_paths([]) == []
 
+        # resolved path without user appends user
         paths = [os.path.join(os.path.sep, 'a', 'b', 'c')]
-        assert spack.stage._resolve_paths(paths) == paths
-
-        tmp = '$tempdir'
-        paths = [os.path.join(tmp, 'stage'), os.path.join(tmp, '$user')]
-        can_paths = [canonicalize_path(paths[0]), canonicalize_path(tmp)]
         user = getpass.getuser()
-        if user not in can_paths[1].split(os.path.sep):
+        can_paths = [os.path.join(paths[0], user)]
-            can_paths[1] = os.path.join(can_paths[1], user)
         assert spack.stage._resolve_paths(paths) == can_paths
 
-    def test_get_stage_root_bad_path(self, clear_stage_root, bad_stage_path):
+        # resolved path with node including user does not append user
+        paths = [os.path.join(os.path.sep, 'spack-{0}'.format(user), 'stage')]
+        assert spack.stage._resolve_paths(paths) == paths
+
+        # resolve paths where user
+        tmp = '$tempdir'
+        can_tmpdir = canonicalize_path(tmp)
+        temp_has_user = user in can_tmpdir.split(os.sep)
+        paths = [os.path.join(tmp, 'stage'), os.path.join(tmp, '$user')]
+        can_paths = [canonicalize_path(p) for p in paths]
+
+        if temp_has_user:
+            can_paths[1] = can_tmpdir
+        else:
+            can_paths[0] = os.path.join(can_paths[0], user)
+
+        assert spack.stage._resolve_paths(paths) == can_paths
+
+    def test_get_stage_root_bad_path(self, clear_stage_root):
         """Ensure an invalid stage path root raises a StageError."""
-        with pytest.raises(spack.stage.StageError,
+        with spack.config.override('config:build_stage', '/no/such/path'):
-                           match="No accessible stage paths in"):
+            with pytest.raises(spack.stage.StageError,
-            assert spack.stage.get_stage_root() is None
+                               match="No accessible stage paths in"):
+                spack.stage.get_stage_root()
 
         # 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,
+    @pytest.mark.parametrize(
-                                          non_user_path_for_stage):
+        'path,purged', [('stage-1234567890abcdef1234567890abcdef', True),
-        """Ensure a non-user stage root includes the username."""
+                        ('stage-abcdef12345678900987654321fedcba', True),
-        # The challenge here is whether the user has access to the standard
+                        ('stage-a1b2c3', False)])
-        # non-user path.  If not, the path should still appear in the error.
+    def test_stage_purge(self, tmpdir, clear_stage_root, path, purged):
+        """Test purging of stage directories."""
+        stage_dir = tmpdir.join('stage')
+        stage_path = str(stage_dir)
+
+        test_dir = stage_dir.join(path)
+        test_dir.ensure(dir=True)
+        test_path = str(test_dir)
+
+        with spack.config.override('config:build_stage', stage_path):
+            stage_root = spack.stage.get_stage_root()
+            assert stage_path == stage_root
+
+            spack.stage.purge()
+
+            if purged:
+                assert not os.path.exists(test_path)
+            else:
+                assert os.path.exists(test_path)
+                shutil.rmtree(test_path)
+
+    def test_get_stage_root_in_spack(self, clear_stage_root):
+        """Ensure an instance path is an accessible build stage path."""
+
+        base = canonicalize_path(os.path.join('$spack', '.spack-test-stage'))
+        mkdirp(base)
+        test_path = tempfile.mkdtemp(dir=base)
+
         try:
-            path = spack.stage.get_stage_root()
+            with spack.config.override('config:build_stage',  test_path):
-            assert getpass.getuser() in path.split(os.path.sep)
+                path = spack.stage.get_stage_root()
 
-            # Make sure the cached stage path values are changed appropriately.
+                assert 'spack' in path.split(os.path.sep)
-            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.
+                # Make sure cached stage path value was changed appropriately
-            assert spack.stage._stage_root is None
+                assert spack.stage._stage_root == test_path
 
-    def test_get_stage_root_tmp(self, clear_stage_root, tmp_path_for_stage):
+                # Make sure the directory exists
-        """Ensure a temp path stage root is a suitable temp path."""
+                assert os.path.isdir(spack.stage._stage_root)
-        assert spack.stage._stage_root is None
 
-        tmpdir = tmp_path_for_stage
+        finally:
-        path = spack.stage.get_stage_root()
+            # Clean up regardless of outcome
-        assert path == str(tmpdir)
+            shutil.rmtree(base)
-        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 few directories
-        dir1 = tmpdir.join('stage-1234567890abcdef1234567890abcdef')
-        dir1.ensure(dir=True)
-        dir2 = tmpdir.join('stage-abcdef12345678900987654321fedcba')
-        dir2.ensure(dir=True)
-        dir3 = tmpdir.join('stage-a1b2c3')
-        dir3.ensure(dir=True)
-
-        spack.stage.purge()
-        assert not os.path.exists(str(dir1))
-        assert not os.path.exists(str(dir2))
-        assert os.path.exists(str(dir3))
-
-        # Cleanup
-        shutil.rmtree(str(dir3))
-
-    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."""
@@ -879,34 +827,19 @@ def test_diystage_preserve_file(self, tmpdir):
             _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)
+    mkdirp(test_stage_path)
 
-    nondir = test_stage_path.join('afile')
+    nondir = os.path.join(test_stage_path, 'afile')
-    nondir.ensure(dir=False)
+    touch(nondir)
     path = str(nondir)
 
     stage = Stage(path, name='')
-    stage.create()  # Should ensure the path is converted to a dir
+    stage.create()
 
+    # Ensure the stage path is "converted" to a directory
     assert os.path.isdir(stage.path)