From d600aef4f474b443dd136908f0998ac5a337669c Mon Sep 17 00:00:00 2001 From: Chris Green Date: Fri, 5 May 2023 07:40:49 -0500 Subject: Relax environment manifest filename requirements and lockfile identification criteria (#37413) * Relax filename requirements and lockfile identification criteria * Tests * Update function docs and help text * Update function documentation * Update Sphinx documentation * Adjustments per https://github.com/spack/spack/pull/37413#pullrequestreview-1413540132 * Further tweaks per https://github.com/spack/spack/pull/37413#pullrequestreview-1413971254 * Doc fixes per https://github.com/spack/spack/pull/37413#issuecomment-1535976068 --- lib/spack/docs/configuration.rst | 5 ++- lib/spack/docs/environments.rst | 8 ++-- lib/spack/docs/mirrors.rst | 2 +- lib/spack/spack/cmd/env.py | 11 +++-- lib/spack/spack/environment/environment.py | 28 +++++++----- lib/spack/spack/test/env.py | 71 ++++++++++++++++++++++++++---- 6 files changed, 92 insertions(+), 33 deletions(-) diff --git a/lib/spack/docs/configuration.rst b/lib/spack/docs/configuration.rst index 021ae84136..7026825fa8 100644 --- a/lib/spack/docs/configuration.rst +++ b/lib/spack/docs/configuration.rst @@ -20,8 +20,9 @@ case you want to skip directly to specific docs: * :ref:`packages.yaml ` * :ref:`repos.yaml ` -You can also add any of these as inline configuration in ``spack.yaml`` -in an :ref:`environment `. +You can also add any of these as inline configuration in the YAML +manifest file (``spack.yaml``) describing an :ref:`environment +`. ----------- YAML Format diff --git a/lib/spack/docs/environments.rst b/lib/spack/docs/environments.rst index e2805e4f01..415d590882 100644 --- a/lib/spack/docs/environments.rst +++ b/lib/spack/docs/environments.rst @@ -94,9 +94,9 @@ an Environment, the ``.spack-env`` directory also contains: * ``logs/``: A directory containing the build logs for the packages in this Environment. -Spack Environments can also be created from either a ``spack.yaml`` -manifest or a ``spack.lock`` lockfile. To create an Environment from a -``spack.yaml`` manifest: +Spack Environments can also be created from either a manifest file +(usually but not necessarily named, ``spack.yaml``) or a lockfile. +To create an Environment from a manifest: .. code-block:: console @@ -174,7 +174,7 @@ Anonymous specs can be created in place using the command: $ spack env create -d . -In this case Spack simply creates a spack.yaml file in the requested +In this case Spack simply creates a ``spack.yaml`` file in the requested directory. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/lib/spack/docs/mirrors.rst b/lib/spack/docs/mirrors.rst index ed15cb7d8b..2f376c095e 100644 --- a/lib/spack/docs/mirrors.rst +++ b/lib/spack/docs/mirrors.rst @@ -163,7 +163,7 @@ your site. Mirror environment ^^^^^^^^^^^^^^^^^^ -To create a mirror of all packages required by a concerte environment, activate the environment and call ``spack mirror create -a``. +To create a mirror of all packages required by a concrete environment, activate the environment and call ``spack mirror create -a``. This is especially useful to create a mirror of an environment concretized on another machine. .. code-block:: console diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py index 29f6db412a..3e122087f6 100644 --- a/lib/spack/spack/cmd/env.py +++ b/lib/spack/spack/cmd/env.py @@ -283,7 +283,7 @@ def env_create_setup_parser(subparser): "envfile", nargs="?", default=None, - help="optional init file; can be spack.yaml or spack.lock", + help="either a lockfile (must end with '.json' or '.lock') or a manifest file.", ) @@ -292,7 +292,7 @@ def env_create(args): # Expand relative paths provided on the command line to the current working directory # This way we interpret `spack env create --with-view ./view --dir ./env` as # a view in $PWD/view, not $PWD/env/view. This is different from specifying a relative - # path in spack.yaml, which is resolved relative to the environment file. + # path in the manifest, which is resolved relative to the manifest file's location. with_view = os.path.abspath(args.with_view) elif args.without_view: with_view = False @@ -317,7 +317,7 @@ def _env_create(name_or_path, *, init_file=None, dir=False, with_view=None, keep Arguments: name_or_path (str): name of the environment to create, or path to it init_file (str or file): optional initialization file -- can be - spack.yaml or spack.lock + a JSON lockfile (*.lock, *.json) or YAML manifest file dir (bool): if True, create an environment in a directory instead of a named environment keep_relative (bool): if True, develop paths are copied verbatim into @@ -355,8 +355,7 @@ def env_remove(args): """Remove a *named* environment. This removes an environment managed by Spack. Directory environments - and `spack.yaml` files embedded in repositories should be removed - manually. + and manifests embedded in repositories should be removed manually. """ read_envs = [] for env_name in args.rm_env: @@ -568,7 +567,7 @@ def env_revert(args): # Check that both the spack.yaml and the backup exist, the inform user # on what is going to happen and ask for confirmation if not os.path.exists(manifest_file): - msg = "cannot fine the manifest file of the environment [file={0}]" + msg = "cannot find the manifest file of the environment [file={0}]" tty.die(msg.format(manifest_file)) if not os.path.exists(backup_file): msg = "cannot find the old manifest file to be restored [file={0}]" diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 07ed5a868d..b3d4176c04 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -281,9 +281,12 @@ def create( A managed environment is created in a root directory managed by this Spack instance, so that Spack can keep track of them. + Files with suffix ``.json`` or ``.lock`` are considered lockfiles. Files with any other name + are considered manifest files. + Args: name: name of the managed environment - init_file: either a "spack.yaml" or a "spack.lock" file or None + 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, @@ -303,9 +306,12 @@ def create_in_dir( ) -> "Environment": """Create an environment in the directory passed as input and returns it. + Files with suffix ``.json`` or ``.lock`` are considered lockfiles. Files with any other name + are considered manifest files. + Args: manifest_dir: directory where to create the environment. - init_file: either a "spack.yaml" or a "spack.lock" file or None + 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, @@ -2496,7 +2502,8 @@ def initialize_environment_dir( ) -> None: """Initialize an environment directory starting from an envfile. - The envfile can be either a "spack.yaml" manifest file, or a "spack.lock" file. + Files with suffix .json or .lock are considered lockfiles. Files with any other name + are considered manifest files. Args: environment_dir: directory where the environment should be placed @@ -2533,21 +2540,18 @@ def initialize_environment_dir( msg = f"cannot initialize environment, {envfile} is not a valid file" raise SpackEnvironmentError(msg) - if not str(envfile).endswith(manifest_name) and not str(envfile).endswith(lockfile_name): - msg = ( - f"cannot initialize environment from '{envfile}', either a '{manifest_name}'" - f" or a '{lockfile_name}' file is needed" - ) - raise SpackEnvironmentError(msg) - _ensure_env_dir() # When we have a lockfile we should copy that and produce a consistent default manifest - if str(envfile).endswith(lockfile_name): + if str(envfile).endswith(".lock") or str(envfile).endswith(".json"): shutil.copy(envfile, target_lockfile) # This constructor writes a spack.yaml which is consistent with the root # specs in the spack.lock - EnvironmentManifestFile.from_lockfile(environment_dir) + try: + EnvironmentManifestFile.from_lockfile(environment_dir) + except Exception as e: + msg = f"cannot initialize environment, '{environment_dir}' from lockfile" + raise SpackEnvironmentError(msg) from e return shutil.copy(envfile, target_manifest) diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py index fc0ef4cba8..e40f42fa04 100644 --- a/lib/spack/spack/test/env.py +++ b/lib/spack/spack/test/env.py @@ -3,6 +3,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) """Test environment internals without CLI""" +import filecmp import os import pickle import sys @@ -316,20 +317,13 @@ def test_cannot_initiliaze_if_dirname_exists_as_a_file(tmp_path): ev.create_in_dir(dir_name) -def test_cannot_initiliaze_if_init_file_does_not_exist(tmp_path): +def test_cannot_initialize_if_init_file_does_not_exist(tmp_path): """Tests that initializing an environment passing a non-existing init file raises an error.""" init_file = tmp_path / ev.manifest_name with pytest.raises(ev.SpackEnvironmentError, match="cannot initialize"): ev.create_in_dir(tmp_path, init_file=init_file) -def test_cannot_initialize_from_random_file(tmp_path): - init_file = tmp_path / "foo.txt" - init_file.touch() - with pytest.raises(ev.SpackEnvironmentError, match="cannot initialize"): - ev.create_in_dir(tmp_path, init_file=init_file) - - def test_environment_pickle(tmp_path): env1 = ev.create_in_dir(tmp_path) obj = pickle.dumps(env1) @@ -419,3 +413,64 @@ def test_preserving_comments_when_adding_specs( content = spack_yaml.read_text() assert content == expected_yaml + + +@pytest.mark.parametrize("filename", [ev.lockfile_name, "as9582g54.lock", "m3ia54s.json"]) +@pytest.mark.regression("37410") +def test_initialize_from_lockfile(tmp_path, filename): + """Some users have workflows where they store multiple lockfiles in the + same directory, and pick one of them to create an environment depending + on external parameters e.g. while running CI jobs. This test ensures that + Spack can create environments from lockfiles that are not necessarily named + 'spack.lock' and can thus coexist in the same directory. + """ + + init_file = tmp_path / filename + env_dir = tmp_path / "env_dir" + init_file.write_text('{ "roots": [] }\n') + ev.initialize_environment_dir(env_dir, init_file) + + assert os.path.exists(env_dir / ev.lockfile_name) + assert filecmp.cmp(env_dir / ev.lockfile_name, init_file, shallow=False) + + +def test_cannot_initialize_from_bad_lockfile(tmp_path): + """Test that we fail on an incorrectly constructed lockfile""" + + init_file = tmp_path / ev.lockfile_name + env_dir = tmp_path / "env_dir" + + init_file.write_text("Not a legal JSON file\n") + + with pytest.raises(ev.SpackEnvironmentError, match="from lockfile"): + ev.initialize_environment_dir(env_dir, init_file) + + +@pytest.mark.parametrize("filename", ["random.txt", "random.yaml", ev.manifest_name]) +@pytest.mark.regression("37410") +def test_initialize_from_random_file_as_manifest(tmp_path, filename): + """Some users have workflows where they store multiple lockfiles in the + same directory, and pick one of them to create an environment depending + on external parameters e.g. while running CI jobs. This test ensures that + Spack can create environments from manifest that are not necessarily named + 'spack.yaml' and can thus coexist in the same directory. + """ + + init_file = tmp_path / filename + env_dir = tmp_path / "env_dir" + + init_file.write_text( + """\ +spack: + view: true + concretizer: + unify: true + specs: [] +""" + ) + + ev.create_in_dir(env_dir, init_file) + + assert not os.path.exists(env_dir / ev.lockfile_name) + assert os.path.exists(env_dir / ev.manifest_name) + assert filecmp.cmp(env_dir / ev.manifest_name, init_file, shallow=False) -- cgit v1.2.3-60-g2f50