environment views: better, earlier error on clash (#35541)

Spack generally ignores file-file projection clashes in environment
views, but would eventually error when linking the `.spack` directory
for two specs of the same package.

This leads to obscure errors where users have no clue what the issue is
and how to fix it. On top of that, the error comes very late, since it
happens when the .spack dir contents are linked (which happens after
everything else)

This PR improves that by doing a quick check ahead of time if clashes
are going to be anticipated (by simply checking for clashes in the
projection of each spec's .spack metadir). If there are clashes, a
human-readable error is thrown which shows two of the conflicting specs,
and tells users to user unify:true, view:false, or set up custom
projections.

4 files changed, 53 insertions, 9 deletions

diff --git a/lib/spack/llnl/util/link_tree.py b/lib/spack/llnl/util/link_tree.py
index 52268bc3f8..dece1c5edf 100644
--- a/lib/spack/llnl/util/link_tree.py
+++ b/lib/spack/llnl/util/link_tree.py
@@ -430,6 +430,11 @@ class MergeConflictError(Exception):
     pass
 
 
+class ConflictingSpecsError(MergeConflictError):
+    def __init__(self, spec_1, spec_2):
+        super(MergeConflictError, self).__init__(spec_1, spec_2)
+
+
 class SingleMergeConflictError(MergeConflictError):
     def __init__(self, path):
         super(MergeConflictError, self).__init__("Package merge blocked by file: %s" % path)
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index a46b8f9835..813ffca0b6 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -18,7 +18,9 @@ import ruamel.yaml as yaml
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
+import llnl.util.tty.color as clr
 from llnl.util.lang import dedupe
+from llnl.util.link_tree import ConflictingSpecsError
 from llnl.util.symlink import symlink
 
 import spack.compilers
@@ -603,7 +605,24 @@ class ViewDescriptor(object):
                 os.unlink(tmp_symlink_name)
             except (IOError, OSError):
                 pass
-            raise e
+
+            # Give an informative error message for the typical error case: two specs, same package
+            # project to same prefix.
+            if isinstance(e, ConflictingSpecsError):
+                spec_a = e.args[0].format(color=clr.get_color_when())
+                spec_b = e.args[1].format(color=clr.get_color_when())
+                raise SpackEnvironmentViewError(
+                    f"The environment view in {self.root} could not be created, "
+                    "because the following two specs project to the same prefix:\n"
+                    f"    {spec_a}, and\n"
+                    f"    {spec_b}.\n"
+                    "    To resolve this issue:\n"
+                    "        a. use `concretization:unify:true` to ensure there is only one "
+                    "package per spec in the environment, or\n"
+                    "        b. disable views with `view:false`, or\n"
+                    "        c. create custom view projections."
+                ) from e
+            raise
 
         # Remove the old root when it's in the same folder as the new root. This guards
         # against removal of an arbitrary path when the original symlink in self.root
diff --git a/lib/spack/spack/filesystem_view.py b/lib/spack/spack/filesystem_view.py
index 058a952216..500adb4e43 100644
--- a/lib/spack/spack/filesystem_view.py
+++ b/lib/spack/spack/filesystem_view.py
@@ -20,6 +20,7 @@ from llnl.util.filesystem import (
 )
 from llnl.util.lang import index_by, match_predicate
 from llnl.util.link_tree import (
+    ConflictingSpecsError,
     DestinationMergeVisitor,
     LinkTree,
     MergeConflictSummary,
@@ -638,6 +639,22 @@ class SimpleFilesystemView(FilesystemView):
     def __init__(self, root, layout, **kwargs):
         super(SimpleFilesystemView, self).__init__(root, layout, **kwargs)
 
+    def _sanity_check_view_projection(self, specs):
+        """A very common issue is that we end up with two specs of the same
+        package, that project to the same prefix. We want to catch that as
+        early as possible and give a sensible error to the user. Here we use
+        the metadata dir (.spack) projection as a quick test to see whether
+        two specs in the view are going to clash. The metadata dir is used
+        because it's always added by Spack with identical files, so a
+        guaranteed clash that's easily verified."""
+        seen = dict()
+        for current_spec in specs:
+            metadata_dir = self.relative_metadata_dir_for_spec(current_spec)
+            conflicting_spec = seen.get(metadata_dir)
+            if conflicting_spec:
+                raise ConflictingSpecsError(current_spec, conflicting_spec)
+            seen[metadata_dir] = current_spec
+
     def add_specs(self, *specs, **kwargs):
         assert all((s.concrete for s in specs))
         if len(specs) == 0:
@@ -652,6 +669,8 @@ class SimpleFilesystemView(FilesystemView):
         if kwargs.get("exclude", None):
             specs = set(filter_exclude(specs, kwargs["exclude"]))
 
+        self._sanity_check_view_projection(specs)
+
         # Ignore spack meta data folder.
         def skip_list(file):
             return os.path.basename(file) == spack.store.layout.metadata_dir
@@ -714,16 +733,17 @@ class SimpleFilesystemView(FilesystemView):
             for src_root, group in per_source
         }
 
+    def relative_metadata_dir_for_spec(self, spec):
+        return os.path.join(
+            self.get_relative_projection_for_spec(spec), spack.store.layout.metadata_dir, spec.name
+        )
+
     def link_metadata(self, specs):
         metadata_visitor = SourceMergeVisitor()
 
         for spec in specs:
             src_prefix = os.path.join(spec.package.view_source(), spack.store.layout.metadata_dir)
-            proj = os.path.join(
-                self.get_relative_projection_for_spec(spec),
-                spack.store.layout.metadata_dir,
-                spec.name,
-            )
+            proj = self.relative_metadata_dir_for_spec(spec)
             metadata_visitor.set_projection(proj)
             visit_directory_tree(src_prefix, metadata_visitor)
 
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index dc183a84f4..e757384b8f 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -1200,7 +1200,7 @@ def test_env_view_fails(tmpdir, mock_packages, mock_stage, mock_fetch, install_m
         add("libelf")
         add("libelf cflags=-g")
         with pytest.raises(
-            llnl.util.link_tree.MergeConflictSummary, match=spack.store.layout.metadata_dir
+            ev.SpackEnvironmentViewError, match="two specs project to the same prefix"
         ):
             install("--fake")
 
@@ -2830,10 +2830,10 @@ def test_failed_view_cleanup(tmpdir, mock_stage, mock_fetch, install_mockery):
     all_views = os.path.dirname(resolved_view)
     views_before = os.listdir(all_views)
 
-    # Add a spec that results in MergeConflictError's when creating a view
+    # Add a spec that results in view clash when creating a view
     with ev.read("env"):
         add("libelf cflags=-O3")
-        with pytest.raises(llnl.util.link_tree.MergeConflictError):
+        with pytest.raises(ev.SpackEnvironmentViewError):
             install("--fake")
 
     # Make sure there is no broken view in the views directory, and the current