From d94e8ab36fb2bb7c2c7ee04bc76838bad24e5a20 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Tue, 4 Jun 2024 17:52:21 +0200 Subject: [PATCH] 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 --- lib/spack/spack/cmd/view.py | 12 +- lib/spack/spack/environment/environment.py | 14 +-- lib/spack/spack/filesystem_view.py | 112 ++++++++++++------ .../repos/builtin/packages/python/package.py | 46 +++++++ 4 files changed, 131 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 @@ 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 bc31f820b0..294bbd1c91 100644 --- a/lib/spack/spack/environment/environment.py +++ b/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): 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 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): - # 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 @@ 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 = 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 diff --git a/var/spack/repos/builtin/packages/python/package.py b/var/spack/repos/builtin/packages/python/package.py index b87ee81305..2883433a92 100644 --- a/var/spack/repos/builtin/packages/python/package.py +++ b/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