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>

lib/spack/spack/cmd/view.py

@@ -38,10 +38,10 @@
 
 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_type = args.action if args.action in actions_link else "symlink"
-        link_fn = view_func_parser(args.action)
+    view = fsv.YamlFilesystemView(
-    else:
-        link_fn = view_func_parser("symlink")
-
-    view = 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,
     )
 

lib/spack/spack/environment/environment.py

@@ -30,6 +30,7 @@
 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
@@ -52,7 +53,6 @@
 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
@@ -606,7 +606,7 @@ def __init__(
         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):
@@ -640,7 +640,7 @@ def to_dict(self):
         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
@@ -690,7 +690,7 @@ def get_projection_for_spec(self, spec):
         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
@@ -710,14 +710,14 @@ def view(self, new: Optional[str] = None) -> SimpleFilesystemView:
             )
         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):

lib/spack/spack/filesystem_view.py

@@ -10,8 +10,9 @@
 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 @@
 _projections_path = ".spack/projections.yaml"
 
 
-def view_symlink(src, dst, **kwargs):
+LinkCallbackType = Callable[[str, str, "FilesystemView", Optional["spack.spec.Spec"]], None]
-    # keyword arguments are irrelevant
+
-    # here to fit required call signature
+
+def view_symlink(src: str, dst: str, *args, **kwargs) -> None:
     symlink(src, dst)
 
 
-def view_hardlink(src, dst, **kwargs):
+def view_hardlink(src: str, dst: str, *args, **kwargs) -> None:
-    # keyword arguments are irrelevant
-    # here to fit required call signature
     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):
+#: supported string values for `link_type` in an env, mapped to canonical values
-    # What method are we using for this view
+_LINK_TYPES = {
-    if parsed_name in ("hardlink", "hard"):
+    "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"):
+    elif link_type == "symlink":
-        return view_copy
-    elif parsed_name in ("add", "symlink", "soft"):
         return view_symlink
-    else:
+    elif link_type == "copy":
-        raise ValueError(f"invalid link type for view: '{parsed_name}'")
+        return view_copy
 
-
+    assert False, "invalid link type"  # need mypy Literal values
-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
 
 
 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 @@ def __init__(self, root, layout, **kwargs):
         """
         self._root = root
         self.layout = layout
+        self.projections = {} if projections is None else projections
 
-        self.projections = kwargs.get("projections", {})
+        self.ignore_conflicts = ignore_conflicts
-
+        self.verbose = verbose
-        self.ignore_conflicts = kwargs.get("ignore_conflicts", False)
-        self.verbose = kwargs.get("verbose", False)
 
         # Setup link function to include view
-        link_func = kwargs.get("link", view_symlink)
+        self.link_type = link_type
-        self.link = ft.partial(link_func, view=self)
+        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):
+    def __init__(
-        super().__init__(root, layout, **kwargs)
+        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

var/spack/repos/builtin/packages/python/package.py

@@ -21,6 +21,25 @@
 from spack.util.prefix import Prefix
 
 
+def make_pyvenv_cfg(python_spec: "spack.spec.Spec", venv_prefix: str) -> str:
+    """Make a pyvenv_cfg file for a given (real) python command and venv prefix."""
+    python_cmd = python_spec.command.path
+    lines = [
+        # directory containing python command
+        f"home = {os.path.dirname(python_cmd)}",
+        # venv should not allow site packages from the real python to be loaded
+        "include-system-site-packages = false",
+        # version of the python command
+        f"version = {python_spec.version}",
+        # the path to the python command
+        f"executable = {python_cmd}",
+        # command "used" to create the pyvenv.cfg
+        f"command = {python_cmd} -m venv --without-pip {venv_prefix}",
+    ]
+
+    return "\n".join(lines) + "\n"
+
+
 class Python(Package):
     """The Python programming language."""
 
@@ -1235,6 +1254,33 @@ def setup_dependent_package(self, module, dependent_spec):
         module.python_platlib = join_path(dependent_spec.prefix, self.platlib)
         module.python_purelib = join_path(dependent_spec.prefix, self.purelib)
 
+    def add_files_to_view(self, view, merge_map, skip_if_exists=True):
+        """Make the view a virtual environment if it isn't one already.
+
+        If `python-venv` is linked into the view, it will already be a virtual
+        environment. If not, then this is an older python that doesn't use the
+        python-venv support, or we may be using python packages that
+        use ``depends_on("python")`` but not ``extends("python")``.
+
+        We used to copy the python interpreter in, but we can get the same effect in a
+        simpler way by adding a ``pyvenv.cfg`` to the environment.
+
+        """
+        super().add_files_to_view(view, merge_map, skip_if_exists=skip_if_exists)
+
+        # location of python inside the view, where we will put the venv config
+        projection = view.get_projection_for_spec(self.spec)
+        pyvenv_cfg = os.path.join(projection, "pyvenv.cfg")
+        if os.path.lexists(pyvenv_cfg):
+            return
+
+        # don't put a pyvenv.cfg in a copy view
+        if view.link_type == "copy":
+            return
+
+        with open(pyvenv_cfg, "w") as cfg_file:
+            cfg_file.write(make_pyvenv_cfg(self.spec["python"], projection))
+
     def test_hello_world(self):
         """run simple hello world program"""
         # do not use self.command because we are also testing the run env