Deprecate `spack:concretization` over `concretizer:unify` (#30038)

* Introduce concretizer:unify option to replace spack:concretization

* Deprecate concretization

* Make spack:concretization overrule concretize:unify for now

* Add environment update logic to move from spack:concretization to spack:concretizer:reuse

* Migrate spack:concretization to spack:concretize:unify in all locations

* For new environments make concretizer:unify explicit, so that defaults can be changed in 0.19

11 files changed, 138 insertions, 41 deletions

diff --git a/lib/spack/docs/containers.rst b/lib/spack/docs/containers.rst
index 72d7a82d9d..8ef54e8693 100644
--- a/lib/spack/docs/containers.rst
+++ b/lib/spack/docs/containers.rst
@@ -59,7 +59,8 @@ other techniques to minimize the size of the final image:
    &&   echo "  specs:" \
    &&   echo "  - gromacs+mpi" \
    &&   echo "  - mpich" \
-   &&   echo "  concretization: together" \
+   &&   echo "  concretizer: together" \
+   &&   echo "    unify: true" \
    &&   echo "  config:" \
    &&   echo "    install_tree: /opt/software" \
    &&   echo "  view: /opt/view") > /opt/spack-environment/spack.yaml
@@ -245,7 +246,8 @@ software is respectively built and installed:
    &&   echo "  specs:" \
    &&   echo "  - gromacs+mpi" \
    &&   echo "  - mpich" \
-   &&   echo "  concretization: together" \
+   &&   echo "  concretizer:" \
+   &&   echo "    unify: true" \
    &&   echo "  config:" \
    &&   echo "    install_tree: /opt/software" \
    &&   echo "  view: /opt/view") > /opt/spack-environment/spack.yaml
@@ -366,7 +368,8 @@ produces, for instance, the following ``Dockerfile``:
    &&   echo "      externals:" \
    &&   echo "      - spec: cuda%gcc" \
    &&   echo "        prefix: /usr/local/cuda" \
-   &&   echo "  concretization: together" \
+   &&   echo "  concretizer:" \
+   &&   echo "    unify: true" \
    &&   echo "  config:" \
    &&   echo "    install_tree: /opt/software" \
    &&   echo "  view: /opt/view") > /opt/spack-environment/spack.yaml
diff --git a/lib/spack/docs/environments.rst b/lib/spack/docs/environments.rst
index 7744dc87e6..fca70d8af9 100644
--- a/lib/spack/docs/environments.rst
+++ b/lib/spack/docs/environments.rst
@@ -281,8 +281,8 @@ need to be installed alongside each other. Central installations done
 at HPC centers by system administrators or user support groups
 are a common case that fits in this behavior.
 Environments *can also be configured to concretize all
-the root specs in a self-consistent way* to ensure that
-each package in the environment comes with a single configuration. This
+the root specs in a unified way* to ensure that
+each package in the environment corresponds to a single concrete spec. This
 mode of operation is usually what is required by software developers that
 want to deploy their development environment.
 
@@ -499,7 +499,7 @@ Spec concretization
 
 Specs can be concretized separately or together, as already
 explained in :ref:`environments_concretization`. The behavior active
-under any environment is determined by the ``concretization`` property:
+under any environment is determined by the ``concretizer:unify`` property:
 
 .. code-block:: yaml
 
@@ -509,10 +509,15 @@ under any environment is determined by the ``concretization`` property:
          - netcdf
          - nco
          - py-sphinx
-       concretization: together
+       concretizer:
+         unify: true
 
-which can currently take either one of the two allowed values ``together`` or ``separately``
-(the default).
+.. note::
+
+   The ``concretizer:unify`` config option was introduced in Spack 0.18 to
+   replace the ``concretization`` property. For reference,
+   ``concretization: separately`` is replaced by ``concretizer:unify:true``,
+   and ``concretization: together`` is replaced by ``concretizer:unify:false``.
 
 .. admonition:: Re-concretization of user specs
 
diff --git a/lib/spack/docs/pipelines.rst b/lib/spack/docs/pipelines.rst
index e2891deed0..bd695c29ba 100644
--- a/lib/spack/docs/pipelines.rst
+++ b/lib/spack/docs/pipelines.rst
@@ -115,7 +115,8 @@ And here's the spack environment built by the pipeline represented as a
 
    spack:
      view: false
-     concretization: separately
+     concretizer:
+       unify: false
 
      definitions:
      - pkgs:
diff --git a/lib/spack/docs/replace_conda_homebrew.rst b/lib/spack/docs/replace_conda_homebrew.rst
index f2f81a4799..3f640e35cf 100644
--- a/lib/spack/docs/replace_conda_homebrew.rst
+++ b/lib/spack/docs/replace_conda_homebrew.rst
@@ -61,7 +61,7 @@ You can see the packages we added earlier in the ``specs:`` section. If you
 ever want to add more packages, you can either use ``spack add`` or manually
 edit this file.
 
-We also need to change the ``concretization:`` option. By default, Spack
+We also need to change the ``concretizer:unify`` option. By default, Spack
 concretizes each spec *separately*, allowing multiple versions of the same
 package to coexist. Since we want a single consistent environment, we want to
 concretize all of the specs *together*.
@@ -78,7 +78,8 @@ Here is what your ``spack.yaml`` looks like with this new setting:
      # add package specs to the `specs` list
      specs: [bash@5, python, py-numpy, py-scipy, py-matplotlib]
      view: true
-     concretization: together
+     concretizer:
+       unify: true
 
 ^^^^^^^^^^^^^^^^
 Symlink location
diff --git a/lib/spack/docs/spack.yaml b/lib/spack/docs/spack.yaml
index 75e3069175..04754d13eb 100644
--- a/lib/spack/docs/spack.yaml
+++ b/lib/spack/docs/spack.yaml
@@ -25,4 +25,5 @@ spack:
   - subversion
   # Plotting
   - graphviz
-  concretization: together
+  concretizer:
+    unify: true
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 8c71deb561..2536186b63 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -79,8 +79,9 @@ lockfile_name = 'spack.lock'
 env_subdir_name = '.spack-env'
 
 
-#: default spack.yaml file to put in new environments
-default_manifest_yaml = """\
+def default_manifest_yaml():
+    """default spack.yaml file to put in new environments"""
+    return """\
 # This is a Spack Environment file.
 #
 # It describes a set of packages to be installed, along with
@@ -89,7 +90,11 @@ spack:
   # add package specs to the `specs` list
   specs: []
   view: true
-"""
+  concretizer:
+    unify: {}
+""".format('true' if spack.config.get('concretizer:unify') else 'false')
+
+
 #: regex for validating enviroment names
 valid_environment_name_re = r'^\w[\w-]*$'
 
@@ -632,11 +637,11 @@ class Environment(object):
             # the init file.
             with fs.open_if_filename(init_file) as f:
                 if hasattr(f, 'name') and f.name.endswith('.lock'):
-                    self._read_manifest(default_manifest_yaml)
+                    self._read_manifest(default_manifest_yaml())
                     self._read_lockfile(f)
                     self._set_user_specs_from_lockfile()
                 else:
-                    self._read_manifest(f, raw_yaml=default_manifest_yaml)
+                    self._read_manifest(f, raw_yaml=default_manifest_yaml())
 
                 # Rewrite relative develop paths when initializing a new
                 # environment in a different location from the spack.yaml file.
@@ -700,7 +705,7 @@ class Environment(object):
         default_manifest = not os.path.exists(self.manifest_path)
         if default_manifest:
             # No manifest, use default yaml
-            self._read_manifest(default_manifest_yaml)
+            self._read_manifest(default_manifest_yaml())
         else:
             with open(self.manifest_path) as f:
                 self._read_manifest(f)
@@ -766,8 +771,11 @@ class Environment(object):
             self.views = {}
         # Retrieve the current concretization strategy
         configuration = config_dict(self.yaml)
-        # default concretization to separately
-        self.concretization = configuration.get('concretization', 'separately')
+
+        # Let `concretization` overrule `concretize:unify` config for now.
+        unify = spack.config.get('concretizer:unify')
+        self.concretization = configuration.get(
+            'concretization', 'together' if unify else 'separately')
 
         # Retrieve dev-build packages:
         self.dev_specs = configuration.get('develop', {})
@@ -1869,17 +1877,15 @@ class Environment(object):
             regenerate (bool): regenerate views and run post-write hooks as
                 well as writing if True.
         """
-        # Intercept environment not using the latest schema format and prevent
-        # them from being modified
-        manifest_exists = os.path.exists(self.manifest_path)
-        if manifest_exists and not is_latest_format(self.manifest_path):
-            msg = ('The environment "{0}" needs to be written to disk, but '
-                   'is currently using a deprecated format. Please update it '
-                   'using:\n\n'
-                   '\tspack env update {0}\n\n'
-                   'Note that previous versions of Spack will not be able to '
+        # Warn that environments are not in the latest format.
+        if not is_latest_format(self.manifest_path):
+            ver = '.'.join(str(s) for s in spack.spack_version_info[:2])
+            msg = ('The environment "{}" is written to disk in a deprecated format. '
+                   'Please update it using:\n\n'
+                   '\tspack env update {}\n\n'
+                   'Note that versions of Spack older than {} may not be able to '
                    'use the updated configuration.')
-            raise RuntimeError(msg.format(self.name))
+            tty.warn(msg.format(self.name, self.name, ver))
 
         # ensure path in var/spack/environments
         fs.mkdirp(self.path)
@@ -2231,14 +2237,16 @@ def _top_level_key(data):
 
 
 def is_latest_format(manifest):
-    """Return True if the manifest file is at the latest schema format,
-    False otherwise.
+    """Return False if the manifest file exists and is not in the latest schema format.
 
     Args:
         manifest (str): manifest file to be analyzed
     """
-    with open(manifest) as f:
-        data = syaml.load(f)
+    try:
+        with open(manifest) as f:
+            data = syaml.load(f)
+    except (OSError, IOError):
+        return True
     top_level_key = _top_level_key(data)
     changed = spack.schema.env.update(data[top_level_key])
     return not changed
diff --git a/lib/spack/spack/schema/__init__.py b/lib/spack/spack/schema/__init__.py
index 60a28bfb4a..45514d4a7c 100644
--- a/lib/spack/spack/schema/__init__.py
+++ b/lib/spack/spack/schema/__init__.py
@@ -4,6 +4,8 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 """This module contains jsonschema files for all of Spack's YAML formats."""
 
+import warnings
+
 import six
 
 import llnl.util.lang
@@ -49,10 +51,12 @@ def _make_validator():
             msg = msg_str_or_func.format(properties=deprecated_properties)
         else:
             msg = msg_str_or_func(instance, deprecated_properties)
+            if msg is None:
+                return
 
         is_error = deprecated['error']
         if not is_error:
-            llnl.util.tty.warn(msg)
+            warnings.warn(msg)
         else:
             import jsonschema
             yield jsonschema.ValidationError(msg)
diff --git a/lib/spack/spack/schema/concretizer.py b/lib/spack/spack/schema/concretizer.py
index 9138ab7685..46d0e9126b 100644
--- a/lib/spack/spack/schema/concretizer.py
+++ b/lib/spack/spack/schema/concretizer.py
@@ -25,6 +25,14 @@ properties = {
                     }
                 }
             },
+            'unify': {
+                'type': 'boolean'
+                # Todo: add when_possible.
+                # 'oneOf': [
+                #     {'type': 'boolean'},
+                #     {'type': 'string', 'enum': ['when_possible']}
+                # ]
+            }
         }
     }
 }
diff --git a/lib/spack/spack/schema/env.py b/lib/spack/spack/schema/env.py
index 65ef1a76a8..85a33cb188 100644
--- a/lib/spack/spack/schema/env.py
+++ b/lib/spack/spack/schema/env.py
@@ -16,6 +16,24 @@ import spack.schema.merged
 import spack.schema.packages
 import spack.schema.projections
 
+warned_about_concretization = False
+
+
+def deprecate_concretization(instance, props):
+    global warned_about_concretization
+    if warned_about_concretization:
+        return None
+    # Deprecate `spack:concretization` in favor of `spack:concretizer:unify`.
+    concretization_to_unify = {'together': 'true', 'separately': 'false'}
+    concretization = instance['concretization']
+    unify = concretization_to_unify[concretization]
+
+    return (
+        'concretization:{} is deprecated and will be removed in Spack 0.19 in favor of '
+        'the new concretizer:unify:{} config option.'.format(concretization, unify)
+    )
+
+
 #: legal first keys in the schema
 keys = ('spack', 'env')
 
@@ -61,6 +79,11 @@ schema = {
             'type': 'object',
             'default': {},
             'additionalProperties': False,
+            'deprecatedProperties': {
+                'properties': ['concretization'],
+                'message': deprecate_concretization,
+                'error': False
+            },
             'properties': union_dicts(
                 # merged configuration scope schemas
                 spack.schema.merged.properties,
@@ -169,11 +192,33 @@ def update(data):
     Returns:
         True if data was changed, False otherwise
     """
+    updated = False
     if 'include' in data:
         msg = ("included configuration files should be updated manually"
                " [files={0}]")
         warnings.warn(msg.format(', '.join(data['include'])))
 
     if 'packages' in data:
-        return spack.schema.packages.update(data['packages'])
-    return False
+        updated |= spack.schema.packages.update(data['packages'])
+
+    # Spack 0.19 drops support for `spack:concretization` in favor of
+    # `spack:concretizer:unify`. Here we provide an upgrade path that changes the former
+    # into the latter, or warns when there's an ambiguity. Note that Spack 0.17 is not
+    # forward compatible with `spack:concretizer:unify`.
+    if 'concretization' in data:
+        has_unify = 'unify' in data.get('concretizer', {})
+        to_unify = {'together': True, 'separately': False}
+        unify = to_unify[data['concretization']]
+
+        if has_unify and data['concretizer']['unify'] != unify:
+            warnings.warn(
+                'The following configuration conflicts: '
+                '`spack:concretization:{}` and `spack:concretizer:unify:{}`'
+                '. Please update manually.'.format(
+                    data['concretization'], data['concretizer']['unify']))
+        else:
+            data.update({'concretizer': {'unify': unify}})
+            data.pop('concretization')
+            updated = True
+
+    return updated
diff --git a/lib/spack/spack/test/cmd/config.py b/lib/spack/spack/test/cmd/config.py
index 4b63994618..99aa005f6e 100644
--- a/lib/spack/spack/test/cmd/config.py
+++ b/lib/spack/spack/test/cmd/config.py
@@ -486,7 +486,7 @@ def test_config_remove_from_env(mutable_empty_config, mutable_mock_env_path):
         config('rm', 'config:dirty')
         output = config('get')
 
-    expected = ev.default_manifest_yaml
+    expected = ev.default_manifest_yaml()
     expected += """  config: {}
 
 """
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 48f0092c09..350dbe7ec1 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -2251,7 +2251,7 @@ def test_env_write_only_non_default():
     with open(e.manifest_path, 'r') as f:
         yaml = f.read()
 
-    assert yaml == ev.default_manifest_yaml
+    assert yaml == ev.default_manifest_yaml()
 
 
 @pytest.mark.regression('20526')
@@ -2358,6 +2358,26 @@ def test_old_format_cant_be_updated_implicitly(packages_yaml_v015):
         add('hdf5')
 
 
+@pytest.mark.parametrize('concretization,unify', [
+    ('together', 'true'),
+    ('separately', 'false')
+])
+def test_update_concretization_to_concretizer_unify(concretization, unify, tmpdir):
+    spack_yaml = """\
+spack:
+  concretization: {}
+""".format(concretization)
+    tmpdir.join('spack.yaml').write(spack_yaml)
+    # Update the environment
+    env('update', '-y', str(tmpdir))
+    with open(str(tmpdir.join('spack.yaml'))) as f:
+        assert f.read() == """\
+spack:
+  concretizer:
+    unify: {}
+""".format(unify)
+
+
 @pytest.mark.regression('18147')
 def test_can_update_attributes_with_override(tmpdir):
     spack_yaml = """
@@ -2391,7 +2411,8 @@ spack:
         modules:
         - libelf/3.18.1
 
-  concretization: together
+  concretizer:
+    unify: false
 """
     abspath = tmpdir.join('spack.yaml')
     abspath.write(spack_yaml)