From 14c7bfe9cedaa779717852aef6520761ffe95cd3 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 18 Dec 2023 00:47:53 -0800 Subject: `spack develop`: convert to config (#35273) Convert the 'develop' section of an environment to a dedicated configuration section. This means for example that instead of having to define `develop` specs in the `spack.yaml`, the environment can `include:` another `develop.yaml` configuration which specifies which specs should be developed in the environment. This change is not expected to be disruptive given that existing environment `spack.yaml` files will conform to the new schema. (Update 11/28/2023) I have implemented the `develop`/`undevelop` commands in terms of more-generic modification functions added to the `config` module: `change_or_add` and `update_all`. It is assumed that the semantics added here (described in 11/18 update) would be desirable to extend to other config update actions (e.g. adding compilers, changing package requirements, adding mirrors). (Update 11/18/2023) I have updated this such that `spack develop`, and `spack undevelop` to potentially modify all writable scopes, like https://github.com/spack/spack/pull/41147. https://github.com/spack/spack/pull/35307 will be useful for modifying included scopes, but generally speaking specifying a `--scope` will not be required for `spack develop`: `spack develop` will add new develop specs to whatever scope already has develop specs defined, or to the highest-priority writable scope (which should be the env scope). TODOs: - [x] If you `spack undevelop` a package which is mentioned at multiple layers of configuration, then currently this would only modify one of them. That's not technically a new issue (has always existed for configuration modification), but may be confusing to users when presented via an interface other than `spack config set` - [x] Need to add (or confirm) the ability to modify individual config files by providing a path (rather than using a scope identifier as a key to retrieve associated config). - [x] `spack develop` adds new develop specs to the scope that defines them (potentially skipping higher priority scopes to e.g. augment included scope files) --------- Co-authored-by: scheibelp Co-authored-by: Todd Gamblin --- lib/spack/spack/cmd/develop.py | 78 +++++++--- lib/spack/spack/cmd/undevelop.py | 50 +++++-- lib/spack/spack/config.py | 79 +++++++++++ lib/spack/spack/environment/environment.py | 221 +++++++++-------------------- lib/spack/spack/schema/develop.py | 34 +++++ lib/spack/spack/schema/env.py | 15 -- lib/spack/spack/schema/merged.py | 2 + lib/spack/spack/test/cmd/develop.py | 12 +- lib/spack/spack/test/cmd/env.py | 50 ++++++- lib/spack/spack/test/cmd/undevelop.py | 4 +- lib/spack/spack/test/config.py | 28 ++++ 11 files changed, 365 insertions(+), 208 deletions(-) create mode 100644 lib/spack/spack/schema/develop.py (limited to 'lib') diff --git a/lib/spack/spack/cmd/develop.py b/lib/spack/spack/cmd/develop.py index f515352175..ca3ab36e4d 100644 --- a/lib/spack/spack/cmd/develop.py +++ b/lib/spack/spack/cmd/develop.py @@ -45,10 +45,41 @@ def setup_parser(subparser): arguments.add_common_arguments(subparser, ["spec"]) -def develop(parser, args): - env = spack.cmd.require_active_env(cmd_name="develop") +def _update_config(spec, path): + find_fn = lambda section: spec.name in section + + entry = {"spec": str(spec)} + if path != spec.name: + entry["path"] = path + + def change_fn(section): + section[spec.name] = entry + + spack.config.change_or_add("develop", find_fn, change_fn) + + +def _retrieve_develop_source(spec, abspath): + # "steal" the source code via staging API. We ask for a stage + # to be created, then copy it afterwards somewhere else. It would be + # better if we can create the `source_path` directly into its final + # destination. + pkg_cls = spack.repo.PATH.get_pkg_class(spec.name) + # We construct a package class ourselves, rather than asking for + # Spec.package, since Spec only allows this when it is concrete + package = pkg_cls(spec) + if isinstance(package.stage[0].fetcher, spack.fetch_strategy.GitFetchStrategy): + package.stage[0].fetcher.get_full_repo = True + # If we retrieved this version before and cached it, we may have + # done so without cloning the full git repo; likewise, any + # mirror might store an instance with truncated history. + package.stage[0].disable_mirrors() + package.stage.steal_source(abspath) + + +def develop(parser, args): if not args.spec: + env = spack.cmd.require_active_env(cmd_name="develop") if args.clone is False: raise SpackError("No spec provided to spack develop command") @@ -66,7 +97,7 @@ def develop(parser, args): # Both old syntax `spack develop pkg@x` and new syntax `spack develop pkg@=x` # are currently supported. spec = spack.spec.parse_with_version_concrete(entry["spec"]) - env.develop(spec=spec, path=path, clone=True) + _retrieve_develop_source(spec, abspath) if not env.dev_specs: tty.warn("No develop specs to download") @@ -81,12 +112,16 @@ def develop(parser, args): version = spec.versions.concrete_range_as_version if not version: raise SpackError("Packages to develop must have a concrete version") - spec.versions = spack.version.VersionList([version]) - # default path is relative path to spec.name + # If user does not specify --path, we choose to create a directory in the + # active environment's directory, named after the spec path = args.path or spec.name - abspath = spack.util.path.canonicalize_path(path, default_wd=env.path) + if not os.path.isabs(path): + env = spack.cmd.require_active_env(cmd_name="develop") + abspath = spack.util.path.canonicalize_path(path, default_wd=env.path) + else: + abspath = path # clone default: only if the path doesn't exist clone = args.clone @@ -96,15 +131,24 @@ def develop(parser, args): if not clone and not os.path.exists(abspath): raise SpackError("Provided path %s does not exist" % abspath) - if clone and os.path.exists(abspath): - if args.force: - shutil.rmtree(abspath) - else: - msg = "Path %s already exists and cannot be cloned to." % abspath - msg += " Use `spack develop -f` to overwrite." - raise SpackError(msg) - + if clone: + if os.path.exists(abspath): + if args.force: + shutil.rmtree(abspath) + else: + msg = "Path %s already exists and cannot be cloned to." % abspath + msg += " Use `spack develop -f` to overwrite." + raise SpackError(msg) + + _retrieve_develop_source(spec, abspath) + + # Note: we could put develop specs in any scope, but I assume + # users would only ever want to do this for either (a) an active + # env or (b) a specified config file (e.g. that is included by + # an environment) + # TODO: when https://github.com/spack/spack/pull/35307 is merged, + # an active env is not required if a scope is specified + env = spack.cmd.require_active_env(cmd_name="develop") + tty.debug("Updating develop config for {0} transactionally".format(env.name)) with env.write_transaction(): - changed = env.develop(spec, path, clone) - if changed: - env.write() + _update_config(spec, path) diff --git a/lib/spack/spack/cmd/undevelop.py b/lib/spack/spack/cmd/undevelop.py index 8f3c19cd1a..7159708796 100644 --- a/lib/spack/spack/cmd/undevelop.py +++ b/lib/spack/spack/cmd/undevelop.py @@ -17,21 +17,51 @@ def setup_parser(subparser): subparser.add_argument( "-a", "--all", action="store_true", help="remove all specs from (clear) the environment" ) + arguments.add_common_arguments(subparser, ["specs"]) -def undevelop(parser, args): - env = spack.cmd.require_active_env(cmd_name="undevelop") +def _update_config(specs_to_remove, remove_all=False): + def change_fn(dev_config): + modified = False + for spec in specs_to_remove: + if spec.name in dev_config: + tty.msg("Undevelop: removing {0}".format(spec.name)) + del dev_config[spec.name] + modified = True + if remove_all and dev_config: + dev_config.clear() + modified = True + return modified + spack.config.update_all("develop", change_fn) + + +def undevelop(parser, args): + remove_specs = None + remove_all = False if args.all: - specs = env.dev_specs.keys() + remove_all = True else: - specs = spack.cmd.parse_specs(args.specs) + remove_specs = spack.cmd.parse_specs(args.specs) + # TODO: when https://github.com/spack/spack/pull/35307 is merged, + # an active env is not required if a scope is specified + env = spack.cmd.require_active_env(cmd_name="undevelop") with env.write_transaction(): - changed = False - for spec in specs: - tty.msg("Removing %s from environment %s development specs" % (spec, env.name)) - changed |= env.undevelop(spec) - if changed: - env.write() + _update_config(remove_specs, remove_all) + + updated_all_dev_specs = set(spack.config.get("develop")) + remove_spec_names = set(x.name for x in remove_specs) + + if remove_all: + not_fully_removed = updated_all_dev_specs + else: + not_fully_removed = updated_all_dev_specs & remove_spec_names + + if not_fully_removed: + tty.msg( + "The following specs could not be removed as develop specs" + " - see `spack config blame develop` to locate files requiring" + f" manual edits: {', '.join(not_fully_removed)}" + ) diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py index cd1be71c9d..58fcbfb6c8 100644 --- a/lib/spack/spack/config.py +++ b/lib/spack/spack/config.py @@ -70,6 +70,7 @@ SECTION_SCHEMAS = { "compilers": spack.schema.compilers.schema, "concretizer": spack.schema.concretizer.schema, "definitions": spack.schema.definitions.schema, + "develop": spack.schema.develop.schema, "mirrors": spack.schema.mirrors.schema, "repos": spack.schema.repos.schema, "packages": spack.schema.packages.schema, @@ -927,6 +928,84 @@ def scopes(): return CONFIG.scopes +def writable_scopes() -> List[ConfigScope]: + """ + Return list of writable scopes + """ + return list( + reversed( + list( + x + for x in CONFIG.scopes.values() + if not isinstance(x, (InternalConfigScope, ImmutableConfigScope)) + ) + ) + ) + + +def writable_scope_names() -> List[str]: + return list(x.name for x in writable_scopes()) + + +def matched_config(cfg_path): + return [(scope, get(cfg_path, scope=scope)) for scope in writable_scope_names()] + + +def change_or_add(section_name, find_fn, update_fn): + """Change or add a subsection of config, with additional logic to + select a reasonable scope where the change is applied. + + Search through config scopes starting with the highest priority: + the first matching a criteria (determined by ``find_fn``) is updated; + if no such config exists, find the first config scope that defines + any config for the named section; if no scopes define any related + config, then update the highest-priority config scope. + """ + configs_by_section = matched_config(section_name) + + found = False + for scope, section in configs_by_section: + found = find_fn(section) + if found: + break + + if found: + update_fn(section) + spack.config.set(section_name, section, scope=scope) + return + + # If no scope meets the criteria specified by ``find_fn``, + # then look for a scope that has any content (for the specified + # section name) + for scope, section in configs_by_section: + if section: + update_fn(section) + found = True + break + + if found: + spack.config.set(section_name, section, scope=scope) + return + + # If no scopes define any config for the named section, then + # modify the highest-priority scope. + scope, section = configs_by_section[0] + update_fn(section) + spack.config.set(section_name, section, scope=scope) + + +def update_all(section_name, change_fn): + """Change a config section, which may have details duplicated + across multiple scopes. + """ + configs_by_section = matched_config("develop") + + for scope, section in configs_by_section: + modified = change_fn(section) + if modified: + spack.config.set(section_name, section, scope=scope) + + def _validate_section_name(section): """Exit if the section is not a valid section.""" if section not in SECTION_SCHEMAS: diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 5d6273506e..2721ddb828 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -16,7 +16,7 @@ import time import urllib.parse import urllib.request import warnings -from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Union +from typing import Dict, Iterable, List, Optional, Set, Tuple, Union import llnl.util.filesystem as fs import llnl.util.tty as tty @@ -307,7 +307,7 @@ def create( def create_in_dir( - manifest_dir: Union[str, pathlib.Path], + root: Union[str, pathlib.Path], init_file: Optional[Union[str, pathlib.Path]] = None, with_view: Optional[Union[str, pathlib.Path, bool]] = None, keep_relative: bool = False, @@ -318,35 +318,72 @@ def create_in_dir( are considered manifest files. Args: - manifest_dir: directory where to create the environment. + root: directory where to create the environment. init_file: either a lockfile, a manifest file, or None with_view: whether a view should be maintained for the environment. If the value is a string, it specifies the path to the view keep_relative: if True, develop paths are copied verbatim into the new environment file, otherwise they are made absolute """ - initialize_environment_dir(manifest_dir, envfile=init_file) + initialize_environment_dir(root, envfile=init_file) if with_view is None and keep_relative: - return Environment(manifest_dir) + return Environment(root) try: - manifest = EnvironmentManifestFile(manifest_dir) + manifest = EnvironmentManifestFile(root) if with_view is not None: manifest.set_default_view(with_view) - if not keep_relative and init_file is not None and str(init_file).endswith(manifest_name): - init_file = pathlib.Path(init_file) - manifest.absolutify_dev_paths(init_file.parent) - manifest.flush() except (spack.config.ConfigFormatError, SpackEnvironmentConfigError) as e: - shutil.rmtree(manifest_dir) + shutil.rmtree(root) raise e - return Environment(manifest_dir) + env = Environment(root) + + if init_file: + init_file_dir = os.path.abspath(os.path.dirname(init_file)) + + if not keep_relative: + if env.path != init_file_dir: + # If we are here, we are creating an environment based on an + # spack.yaml file in another directory, and moreover we want + # dev paths in this environment to refer to their original + # locations. + _rewrite_relative_dev_paths_on_relocation(env, init_file_dir) + + return env + + +def _rewrite_relative_dev_paths_on_relocation(env, init_file_dir): + """When initializing the environment from a manifest file and we plan + to store the environment in a different directory, we have to rewrite + relative paths to absolute ones.""" + with env: + dev_specs = spack.config.get("develop", default={}, scope=env.env_file_config_scope_name()) + if not dev_specs: + return + for name, entry in dev_specs.items(): + dev_path = entry["path"] + expanded_path = os.path.normpath(os.path.join(init_file_dir, entry["path"])) + + # Skip if the expanded path is the same (e.g. when absolute) + if dev_path == expanded_path: + continue + + tty.debug("Expanding develop path for {0} to {1}".format(name, expanded_path)) + + dev_specs[name]["path"] = expanded_path + + spack.config.set("develop", dev_specs, scope=env.env_file_config_scope_name()) + + env._dev_specs = None + # If we changed the environment's spack.yaml scope, that will not be reflected + # in the manifest that we read + env._re_read() def environment_dir_from_name(name: str, exists_ok: bool = True) -> str: @@ -753,8 +790,6 @@ class Environment: #: Specs from "spack.yaml" self.spec_lists: Dict[str, SpecList] = {user_speclist_name: SpecList()} - #: Dev-build specs from "spack.yaml" - self.dev_specs: Dict[str, Any] = {} #: User specs from the last concretization self.concretized_user_specs: List[Spec] = [] #: Roots associated with the last concretization, in order @@ -765,6 +800,7 @@ class Environment: self._repo = None #: Previously active environment self._previous_active = None + self._dev_specs = None with lk.ReadTransaction(self.txlock): self.manifest = EnvironmentManifestFile(manifest_dir) @@ -858,19 +894,29 @@ class Environment: else: self.views = {} - # Retrieve dev-build packages: - self.dev_specs = copy.deepcopy(env_configuration.get("develop", {})) - for name, entry in self.dev_specs.items(): - # spec must include a concrete version - assert Spec(entry["spec"]).versions.concrete_range_as_version - # default path is the spec name - if "path" not in entry: - self.dev_specs[name]["path"] = name - @property def user_specs(self): return self.spec_lists[user_speclist_name] + @property + def dev_specs(self): + if not self._dev_specs: + self._dev_specs = self._read_dev_specs() + return self._dev_specs + + def _read_dev_specs(self): + dev_specs = {} + dev_config = spack.config.get("develop", {}) + for name, entry in dev_config.items(): + local_entry = {"spec": str(entry["spec"])} + # default path is the spec name + if "path" not in entry: + local_entry["path"] = name + else: + local_entry["path"] = entry["path"] + dev_specs[name] = local_entry + return dev_specs + def clear(self, re_read=False): """Clear the contents of the environment @@ -883,7 +929,7 @@ class Environment: self.spec_lists = collections.OrderedDict() self.spec_lists[user_speclist_name] = SpecList() - self.dev_specs = {} # dev-build specs from yaml + self._dev_specs = {} self.concretized_user_specs = [] # user specs from last concretize self.concretized_order = [] # roots of last concretize, in order self.specs_by_hash = {} # concretized specs by hash @@ -1251,82 +1297,6 @@ class Environment: del self.concretized_order[i] del self.specs_by_hash[dag_hash] - def develop(self, spec: Spec, path: str, clone: bool = False) -> bool: - """Add dev-build info for package - - Args: - spec: Set constraints on development specs. Must include a - concrete version. - path: Path to find code for developer builds. Relative - paths will be resolved relative to the environment. - clone: Clone the package code to the path. - If clone is False Spack will assume the code is already present - at ``path``. - - Return: - (bool): True iff the environment was changed. - """ - spec = spec.copy() # defensive copy since we access cached attributes - - if not spec.versions.concrete: - raise SpackEnvironmentError("Cannot develop spec %s without a concrete version" % spec) - - for name, entry in self.dev_specs.items(): - if name == spec.name: - e_spec = Spec(entry["spec"]) - e_path = entry["path"] - - if e_spec == spec: - if path == e_path: - tty.msg("Spec %s already configured for development" % spec) - return False - else: - tty.msg("Updating development path for spec %s" % spec) - break - else: - msg = "Updating development spec for package " - msg += "%s with path %s" % (spec.name, path) - tty.msg(msg) - break - else: - tty.msg("Configuring spec %s for development at path %s" % (spec, path)) - - if clone: - # "steal" the source code via staging API. We ask for a stage - # to be created, then copy it afterwards somewhere else. It would be - # better if we can create the `source_path` directly into its final - # destination. - abspath = spack.util.path.canonicalize_path(path, default_wd=self.path) - pkg_cls = spack.repo.PATH.get_pkg_class(spec.name) - # We construct a package class ourselves, rather than asking for - # Spec.package, since Spec only allows this when it is concrete - package = pkg_cls(spec) - if isinstance(package.fetcher, spack.fetch_strategy.GitFetchStrategy): - package.fetcher.get_full_repo = True - # If we retrieved this version before and cached it, we may have - # done so without cloning the full git repo; likewise, any - # mirror might store an instance with truncated history. - package.stage.disable_mirrors() - - package.stage.steal_source(abspath) - - # If it wasn't already in the list, append it - entry = {"path": path, "spec": str(spec)} - self.dev_specs[spec.name] = entry - self.manifest.add_develop_spec(spec.name, entry=entry.copy()) - return True - - def undevelop(self, spec): - """Remove develop info for abstract spec ``spec``. - - returns True on success, False if no entry existed.""" - spec = Spec(spec) # In case it's a spec object - if spec.name in self.dev_specs: - del self.dev_specs[spec.name] - self.manifest.remove_develop_spec(spec.name) - return True - return False - def is_develop(self, spec): """Returns true when the spec is built from local sources""" return spec.name in self.dev_specs @@ -2901,57 +2871,6 @@ class EnvironmentManifestFile(collections.abc.Mapping): self.set_default_view(view=False) - def add_develop_spec(self, pkg_name: str, entry: Dict[str, str]) -> None: - """Adds a develop spec to the manifest file - - Args: - pkg_name: name of the package to be developed - entry: spec and path of the developed package - """ - # The environment sets the path to pkg_name is that is implicit - if entry["path"] == pkg_name: - entry.pop("path") - - self.pristine_configuration.setdefault("develop", {}).setdefault(pkg_name, {}).update( - entry - ) - self.configuration.setdefault("develop", {}).setdefault(pkg_name, {}).update(entry) - self.changed = True - - def remove_develop_spec(self, pkg_name: str) -> None: - """Removes a develop spec from the manifest file - - Args: - pkg_name: package to be removed from development - - Raises: - SpackEnvironmentError: if there is nothing to remove - """ - try: - del self.pristine_configuration["develop"][pkg_name] - except KeyError as e: - msg = f"cannot remove '{pkg_name}' from develop specs in {self}, entry does not exist" - raise SpackEnvironmentError(msg) from e - del self.configuration["develop"][pkg_name] - self.changed = True - - def absolutify_dev_paths(self, init_file_dir: Union[str, pathlib.Path]) -> None: - """Normalizes the dev paths in the environment with respect to the directory where the - initialization file resides. - - Args: - init_file_dir: directory with the "spack.yaml" used to initialize the environment. - """ - init_file_dir = pathlib.Path(init_file_dir).absolute() - for _, entry in self.pristine_configuration.get("develop", {}).items(): - expanded_path = os.path.normpath(str(init_file_dir / entry["path"])) - entry["path"] = str(expanded_path) - - for _, entry in self.configuration.get("develop", {}).items(): - expanded_path = os.path.normpath(str(init_file_dir / entry["path"])) - entry["path"] = str(expanded_path) - self.changed = True - def flush(self) -> None: """Synchronizes the object with the manifest file on disk.""" if not self.changed: diff --git a/lib/spack/spack/schema/develop.py b/lib/spack/spack/schema/develop.py new file mode 100644 index 0000000000..e8c658de50 --- /dev/null +++ b/lib/spack/spack/schema/develop.py @@ -0,0 +1,34 @@ +# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + + +properties = { + "develop": { + "type": "object", + "default": {}, + "additionalProperties": False, + "patternProperties": { + r"\w[\w-]*": { + "type": "object", + "additionalProperties": False, + "properties": {"spec": {"type": "string"}, "path": {"type": "string"}}, + } + }, + } +} + + +def update(data): + return False + + +#: Full schema with metadata +schema = { + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "Spack repository configuration file schema", + "type": "object", + "additionalProperties": False, + "properties": properties, +} diff --git a/lib/spack/spack/schema/env.py b/lib/spack/spack/schema/env.py index 463c6680f0..fcb3a546bd 100644 --- a/lib/spack/spack/schema/env.py +++ b/lib/spack/spack/schema/env.py @@ -37,21 +37,6 @@ schema = { # extra environment schema properties { "include": {"type": "array", "default": [], "items": {"type": "string"}}, - "develop": { - "type": "object", - "default": {}, - "additionalProperties": False, - "patternProperties": { - r"\w[\w-]*": { - "type": "object", - "additionalProperties": False, - "properties": { - "spec": {"type": "string"}, - "path": {"type": "string"}, - }, - } - }, - }, "specs": spack.schema.spec_list_schema, "view": { "anyOf": [ diff --git a/lib/spack/spack/schema/merged.py b/lib/spack/spack/schema/merged.py index 7ceb649410..fbd8474070 100644 --- a/lib/spack/spack/schema/merged.py +++ b/lib/spack/spack/schema/merged.py @@ -18,6 +18,7 @@ import spack.schema.concretizer import spack.schema.config import spack.schema.container import spack.schema.definitions +import spack.schema.develop import spack.schema.mirrors import spack.schema.modules import spack.schema.packages @@ -34,6 +35,7 @@ properties = union_dicts( spack.schema.container.properties, spack.schema.ci.properties, spack.schema.definitions.properties, + spack.schema.develop.properties, spack.schema.mirrors.properties, spack.schema.modules.properties, spack.schema.packages.properties, diff --git a/lib/spack/spack/test/cmd/develop.py b/lib/spack/spack/test/cmd/develop.py index f80038c0ed..2b1320c690 100644 --- a/lib/spack/spack/test/cmd/develop.py +++ b/lib/spack/spack/test/cmd/develop.py @@ -19,7 +19,7 @@ env = SpackCommand("env") pytestmark = pytest.mark.not_on_windows("does not run on windows") -@pytest.mark.usefixtures("mutable_mock_env_path", "mock_packages", "mock_fetch", "config") +@pytest.mark.usefixtures("mutable_mock_env_path", "mock_packages", "mock_fetch", "mutable_config") class TestDevelop: def check_develop(self, env, spec, path=None): path = path or spec.name @@ -31,9 +31,9 @@ class TestDevelop: assert dev_specs_entry["spec"] == str(spec) # check yaml representation - yaml = env.manifest[ev.TOP_LEVEL_KEY] - assert spec.name in yaml["develop"] - yaml_entry = yaml["develop"][spec.name] + dev_config = spack.config.get("develop", {}) + assert spec.name in dev_config + yaml_entry = dev_config[spec.name] assert yaml_entry["spec"] == str(spec) if path == spec.name: # default paths aren't written out @@ -102,7 +102,7 @@ class TestDevelop: self.check_develop(e, spack.spec.Spec("mpich@=2.0")) assert len(e.dev_specs) == 1 - def test_develop_canonicalize_path(self, monkeypatch, config): + def test_develop_canonicalize_path(self, monkeypatch): env("create", "test") with ev.read("test") as e: path = "../$user" @@ -119,7 +119,7 @@ class TestDevelop: # Check modifications actually worked assert spack.spec.Spec("mpich@1.0").concretized().satisfies("dev_path=%s" % abspath) - def test_develop_canonicalize_path_no_args(self, monkeypatch, config): + def test_develop_canonicalize_path_no_args(self, monkeypatch): env("create", "test") with ev.read("test") as e: path = "$user" diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 4402dd0ba1..74752a737d 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -53,6 +53,7 @@ concretize = SpackCommand("concretize") stage = SpackCommand("stage") uninstall = SpackCommand("uninstall") find = SpackCommand("find") +develop = SpackCommand("develop") module = SpackCommand("module") sep = os.sep @@ -719,10 +720,10 @@ spack: assert any(x.intersects("mpileaks@2.2") for x in e._get_environment_specs()) -def test_with_config_bad_include(environment_from_manifest): +def test_with_config_bad_include_create(environment_from_manifest): """Confirm missing include paths raise expected exception and error.""" with pytest.raises(spack.config.ConfigFileError, match="2 missing include path"): - e = environment_from_manifest( + environment_from_manifest( """ spack: include: @@ -730,9 +731,42 @@ spack: - no/such/file.yaml """ ) - with e: - e.concretize() + +def test_with_config_bad_include_activate(environment_from_manifest, tmpdir): + env_root = pathlib.Path(tmpdir.ensure("env-root", dir=True)) + include1 = env_root / "include1.yaml" + include1.touch() + + abs_include_path = os.path.abspath(tmpdir.join("subdir").ensure("include2.yaml")) + + spack_yaml = env_root / ev.manifest_name + spack_yaml.write_text( + f""" +spack: + include: + - ./include1.yaml + - {abs_include_path} +""" + ) + + e = ev.Environment(env_root) + with e: + e.concretize() + + # we've created an environment with some included config files (which do + # in fact exist): now we remove them and check that we get a sensible + # error message + + os.remove(abs_include_path) + os.remove(include1) + with pytest.raises(spack.config.ConfigFileError) as exc: + ev.activate(e) + + err = exc.value.message + assert "missing include" in err + assert abs_include_path in err + assert "include1.yaml" in err assert ev.active_environment() is None @@ -1173,7 +1207,7 @@ def test_env_blocks_uninstall(mock_stage, mock_fetch, install_mockery): add("mpileaks") install("--fake") - out = uninstall("mpileaks", fail_on_error=False) + out = uninstall("-y", "mpileaks", fail_on_error=False) assert uninstall.returncode == 1 assert "The following environments still reference these specs" in out @@ -2902,13 +2936,15 @@ def test_virtual_spec_concretize_together(tmpdir): assert any(s.package.provides("mpi") for _, s in e.concretized_specs()) -def test_query_develop_specs(): +def test_query_develop_specs(tmpdir): """Test whether a spec is develop'ed or not""" + srcdir = tmpdir.ensure("here") + env("create", "test") with ev.read("test") as e: e.add("mpich") e.add("mpileaks") - e.develop(Spec("mpich@=1"), "here", clone=False) + develop("--no-clone", "-p", str(srcdir), "mpich@=1") assert e.is_develop(Spec("mpich")) assert not e.is_develop(Spec("mpileaks")) diff --git a/lib/spack/spack/test/cmd/undevelop.py b/lib/spack/spack/test/cmd/undevelop.py index 93b7c7ef90..7ba761aade 100644 --- a/lib/spack/spack/test/cmd/undevelop.py +++ b/lib/spack/spack/test/cmd/undevelop.py @@ -17,7 +17,7 @@ concretize = SpackCommand("concretize") pytestmark = pytest.mark.not_on_windows("does not run on windows") -def test_undevelop(tmpdir, config, mock_packages, mutable_mock_env_path): +def test_undevelop(tmpdir, mutable_config, mock_packages, mutable_mock_env_path): # setup environment envdir = tmpdir.mkdir("env") with envdir.as_cwd(): @@ -46,7 +46,7 @@ spack: assert not after.satisfies("dev_path=*") -def test_undevelop_nonexistent(tmpdir, config, mock_packages, mutable_mock_env_path): +def test_undevelop_nonexistent(tmpdir, mutable_config, mock_packages, mutable_mock_env_path): # setup environment envdir = tmpdir.mkdir("env") with envdir.as_cwd(): diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py index 2453172bec..e3513df42b 100644 --- a/lib/spack/spack/test/config.py +++ b/lib/spack/spack/test/config.py @@ -502,6 +502,34 @@ def test_parse_install_tree(config_settings, expected, mutable_config): assert projections == expected_proj +def test_change_or_add(mutable_config, mock_packages): + spack.config.add("packages:a:version:['1.0']", scope="user") + + spack.config.add("packages:b:version:['1.1']", scope="system") + + class ChangeTest: + def __init__(self, pkg_name, new_version): + self.pkg_name = pkg_name + self.new_version = new_version + + def find_fn(self, section): + return self.pkg_name in section + + def change_fn(self, section): + pkg_section = section.get(self.pkg_name, {}) + pkg_section["version"] = self.new_version + section[self.pkg_name] = pkg_section + + change1 = ChangeTest("b", ["1.2"]) + spack.config.change_or_add("packages", change1.find_fn, change1.change_fn) + assert "b" not in mutable_config.get("packages", scope="user") + assert mutable_config.get("packages")["b"]["version"] == ["1.2"] + + change2 = ChangeTest("c", ["1.0"]) + spack.config.change_or_add("packages", change2.find_fn, change2.change_fn) + assert "c" in mutable_config.get("packages", scope="user") + + @pytest.mark.not_on_windows("Padding unsupported on Windows") @pytest.mark.parametrize( "config_settings,expected", -- cgit v1.2.3-70-g09d2