python: make every view a `venv` (#44382)

#40773 introduced python-venv, which improved build isolation and avoids issues with,
e.g., `ubuntu`'s system python modifying `sysconfig` to include a (very unwanted)
`local` directory within the default install layout.

This addresses a few cases where #40773 removed functionality, without harming the
default cases where we use `python-venv`.

Traditionally, *every* view with `python` in it was essentially a virtual environment,
because we would copy the `python` interpreter and `os.py` into every view when linking.
We now rely on `python-venv` to do that, but only when it's used (i.e. new builds) and
only for packages that have an `extends("python")` directive.

This again makes every view with `python` in it a virtual environment, but only
if we're not already using a package like `python-venv`. This uses a different
mechanism from before -- instead of using the `virtualenv` trick of copying `python`
into the prefix, we instead create a `pyvenv.cfg` like `venv` (the more modern way
to do it).

This fixes two things:
1. If you already had an environment before Spack `v0.22` that worked, it would
   stop working without a reconcretize and rebuild in `v0.22`, because we no longer
   copy the python interpreter on link. Adding `pyvenv.cfg` fixes this in a more
   modern way, so old views will keep working.

2. If you have an env that only includes python packages that use `depends_on("python")`
   instead of `extends("python")`, those packages will now be importable as before,
   though they won't have the same level of build isolation you'd get with `extends`
   and `python-venv`.

* views: avoid making client code deal with link functions

Users of views and ViewDescriptors shouldn't have to deal with link functions -- they
should just say what type of linking they want.

- [x] views take a link_type, not a link function
- [x] views work out the link function from the link type
- [x] view descriptors and commands now just tell the view what they want.

* python: simplify logic for avoiding pyvenv.cfg in copy views

Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>

3 files changed, 85 insertions, 53 deletions

diff --git a/lib/spack/spack/cmd/view.py b/lib/spack/spack/cmd/view.py
index 586a9c6eb4..103a6ffb0e 100644
--- a/lib/spack/spack/cmd/view.py
+++ b/lib/spack/spack/cmd/view.py
@@ -38,10 +38,10 @@ from llnl.util.link_tree import MergeConflictError
 
 import spack.cmd
 import spack.environment as ev
+import spack.filesystem_view as fsv
 import spack.schema.projections
 import spack.store
 from spack.config import validate
-from spack.filesystem_view import YamlFilesystemView, view_func_parser
 from spack.util import spack_yaml as s_yaml
 
 description = "project packages to a compact naming scheme on the filesystem"
@@ -193,17 +193,13 @@ def view(parser, args):
         ordered_projections = {}
 
     # What method are we using for this view
-    if args.action in actions_link:
-        link_fn = view_func_parser(args.action)
-    else:
-        link_fn = view_func_parser("symlink")
-
-    view = YamlFilesystemView(
+    link_type = args.action if args.action in actions_link else "symlink"
+    view = fsv.YamlFilesystemView(
         path,
         spack.store.STORE.layout,
         projections=ordered_projections,
         ignore_conflicts=getattr(args, "ignore_conflicts", False),
-        link=link_fn,
+        link_type=link_type,
         verbose=args.verbose,
     )
 
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 54a9ab1795..32aff3e238 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -31,6 +31,7 @@ import spack.config
 import spack.deptypes as dt
 import spack.error
 import spack.fetch_strategy
+import spack.filesystem_view as fsv
 import spack.hash_types as ht
 import spack.hooks
 import spack.main
@@ -53,7 +54,6 @@ import spack.util.spack_yaml as syaml
 import spack.util.url
 import spack.version
 from spack import traverse
-from spack.filesystem_view import SimpleFilesystemView, inverse_view_func_parser, view_func_parser
 from spack.installer import PackageInstaller
 from spack.schema.env import TOP_LEVEL_KEY
 from spack.spec import Spec
@@ -607,7 +607,7 @@ class ViewDescriptor:
         self.projections = projections
         self.select = select
         self.exclude = exclude
-        self.link_type = view_func_parser(link_type)
+        self.link_type = fsv.canonicalize_link_type(link_type)
         self.link = link
 
     def select_fn(self, spec):
@@ -641,7 +641,7 @@ class ViewDescriptor:
         if self.exclude:
             ret["exclude"] = self.exclude
         if self.link_type:
-            ret["link_type"] = inverse_view_func_parser(self.link_type)
+            ret["link_type"] = self.link_type
         if self.link != default_view_link:
             ret["link"] = self.link
         return ret
@@ -691,7 +691,7 @@ class ViewDescriptor:
         to exist on the filesystem."""
         return self._view(self.root).get_projection_for_spec(spec)
 
-    def view(self, new: Optional[str] = None) -> SimpleFilesystemView:
+    def view(self, new: Optional[str] = None) -> fsv.SimpleFilesystemView:
         """
         Returns a view object for the *underlying* view directory. This means that the
         self.root symlink is followed, and that the view has to exist on the filesystem
@@ -711,14 +711,14 @@ class ViewDescriptor:
             )
         return self._view(path)
 
-    def _view(self, root: str) -> SimpleFilesystemView:
+    def _view(self, root: str) -> fsv.SimpleFilesystemView:
         """Returns a view object for a given root dir."""
-        return SimpleFilesystemView(
+        return fsv.SimpleFilesystemView(
             root,
             spack.store.STORE.layout,
             ignore_conflicts=True,
             projections=self.projections,
-            link=self.link_type,
+            link_type=self.link_type,
         )
 
     def __contains__(self, spec):
diff --git a/lib/spack/spack/filesystem_view.py b/lib/spack/spack/filesystem_view.py
index 81a330b4a9..0e508a9bd8 100644
--- a/lib/spack/spack/filesystem_view.py
+++ b/lib/spack/spack/filesystem_view.py
@@ -10,8 +10,9 @@ import re
 import shutil
 import stat
 import sys
-from typing import Optional
+from typing import Callable, Dict, Optional
 
+from llnl.string import comma_or
 from llnl.util import tty
 from llnl.util.filesystem import (
     mkdirp,
@@ -49,19 +50,20 @@ __all__ = ["FilesystemView", "YamlFilesystemView"]
 _projections_path = ".spack/projections.yaml"
 
 
-def view_symlink(src, dst, **kwargs):
-    # keyword arguments are irrelevant
-    # here to fit required call signature
+LinkCallbackType = Callable[[str, str, "FilesystemView", Optional["spack.spec.Spec"]], None]
+
+
+def view_symlink(src: str, dst: str, *args, **kwargs) -> None:
     symlink(src, dst)
 
 
-def view_hardlink(src, dst, **kwargs):
-    # keyword arguments are irrelevant
-    # here to fit required call signature
+def view_hardlink(src: str, dst: str, *args, **kwargs) -> None:
     os.link(src, dst)
 
 
-def view_copy(src: str, dst: str, view, spec: Optional[spack.spec.Spec] = None):
+def view_copy(
+    src: str, dst: str, view: "FilesystemView", spec: Optional["spack.spec.Spec"] = None
+) -> None:
     """
     Copy a file from src to dst.
 
@@ -104,27 +106,40 @@ def view_copy(src: str, dst: str, view, spec: Optional[spack.spec.Spec] = None):
         tty.debug(f"Can't change the permissions for {dst}")
 
 
-def view_func_parser(parsed_name):
-    # What method are we using for this view
-    if parsed_name in ("hardlink", "hard"):
+#: supported string values for `link_type` in an env, mapped to canonical values
+_LINK_TYPES = {
+    "hardlink": "hardlink",
+    "hard": "hardlink",
+    "copy": "copy",
+    "relocate": "copy",
+    "add": "symlink",
+    "symlink": "symlink",
+    "soft": "symlink",
+}
+
+_VALID_LINK_TYPES = sorted(set(_LINK_TYPES.values()))
+
+
+def canonicalize_link_type(link_type: str) -> str:
+    """Return canonical"""
+    canonical = _LINK_TYPES.get(link_type)
+    if not canonical:
+        raise ValueError(
+            f"Invalid link type: '{link_type}. Must be one of {comma_or(_VALID_LINK_TYPES)}'"
+        )
+    return canonical
+
+
+def function_for_link_type(link_type: str) -> LinkCallbackType:
+    link_type = canonicalize_link_type(link_type)
+    if link_type == "hardlink":
         return view_hardlink
-    elif parsed_name in ("copy", "relocate"):
-        return view_copy
-    elif parsed_name in ("add", "symlink", "soft"):
+    elif link_type == "symlink":
         return view_symlink
-    else:
-        raise ValueError(f"invalid link type for view: '{parsed_name}'")
-
+    elif link_type == "copy":
+        return view_copy
 
-def inverse_view_func_parser(view_type):
-    # get string based on view type
-    if view_type is view_hardlink:
-        link_name = "hardlink"
-    elif view_type is view_copy:
-        link_name = "copy"
-    else:
-        link_name = "symlink"
-    return link_name
+    assert False, "invalid link type"  # need mypy Literal values
 
 
 class FilesystemView:
@@ -140,7 +155,16 @@ class FilesystemView:
     directory structure.
     """
 
-    def __init__(self, root, layout, **kwargs):
+    def __init__(
+        self,
+        root: str,
+        layout: "spack.directory_layout.DirectoryLayout",
+        *,
+        projections: Optional[Dict] = None,
+        ignore_conflicts: bool = False,
+        verbose: bool = False,
+        link_type: str = "symlink",
+    ):
         """
         Initialize a filesystem view under the given `root` directory with
         corresponding directory `layout`.
@@ -149,15 +173,14 @@ class FilesystemView:
         """
         self._root = root
         self.layout = layout
+        self.projections = {} if projections is None else projections
 
-        self.projections = kwargs.get("projections", {})
-
-        self.ignore_conflicts = kwargs.get("ignore_conflicts", False)
-        self.verbose = kwargs.get("verbose", False)
+        self.ignore_conflicts = ignore_conflicts
+        self.verbose = verbose
 
         # Setup link function to include view
-        link_func = kwargs.get("link", view_symlink)
-        self.link = ft.partial(link_func, view=self)
+        self.link_type = link_type
+        self.link = ft.partial(function_for_link_type(link_type), view=self)
 
     def add_specs(self, *specs, **kwargs):
         """
@@ -255,8 +278,24 @@ class YamlFilesystemView(FilesystemView):
     Filesystem view to work with a yaml based directory layout.
     """
 
-    def __init__(self, root, layout, **kwargs):
-        super().__init__(root, layout, **kwargs)
+    def __init__(
+        self,
+        root: str,
+        layout: "spack.directory_layout.DirectoryLayout",
+        *,
+        projections: Optional[Dict] = None,
+        ignore_conflicts: bool = False,
+        verbose: bool = False,
+        link_type: str = "symlink",
+    ):
+        super().__init__(
+            root,
+            layout,
+            projections=projections,
+            ignore_conflicts=ignore_conflicts,
+            verbose=verbose,
+            link_type=link_type,
+        )
 
         # Super class gets projections from the kwargs
         # YAML specific to get projections from YAML file
@@ -638,9 +677,6 @@ class SimpleFilesystemView(FilesystemView):
     """A simple and partial implementation of FilesystemView focused on performance and immutable
     views, where specs cannot be removed after they were added."""
 
-    def __init__(self, root, layout, **kwargs):
-        super().__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