Deprecate top-level module config (#28659)

* Ignore top-level module config; add auto-update

In Spack 0.17 we got module sets (modules:[name]:[prop]), and for
backwards compat modules:[prop] was short for modules:default:[prop].

But this makes it awkward to define default config for the "default"
module set.

Since 0.17 is branched off, we can now deprecate top-level module config
(that is, just ignore it with a warning).

This PR does that, and it implements `spack config update modules` to
make upgrading easy (we should have added that to 0.17 already...)

It also removes references to  `dotkit` stuff which was already
deprecated in 0.13 and could have been removed in 0.14.

Prefix inspections are the only exception, since the top-level prefix inspections
used for `spack load` and `spack env activate`.

11 files changed, 103 insertions, 191 deletions

diff --git a/etc/spack/defaults/modules.yaml b/etc/spack/defaults/modules.yaml
index 35be259d4d..8914a6a733 100644
--- a/etc/spack/defaults/modules.yaml
+++ b/etc/spack/defaults/modules.yaml
@@ -35,13 +35,10 @@ modules:
 
   # These are configurations for the module set named "default"
   default:
-    # These values are defaulted in the code. They are not defaulted here so
-    # that we can enable backwards compatibility with the old syntax more
-    # easily (old value is in the config yaml, config:module_roots)
     # Where to install modules
-    # roots:
-    #  tcl:    $spack/share/spack/modules
-    #  lmod:   $spack/share/spack/lmod
+    roots:
+     tcl:    $spack/share/spack/modules
+     lmod:   $spack/share/spack/lmod
     # What type of modules to use
     enable:
       - tcl
diff --git a/lib/spack/docs/config_yaml.rst b/lib/spack/docs/config_yaml.rst
index 3d21642749..6fb673e461 100644
--- a/lib/spack/docs/config_yaml.rst
+++ b/lib/spack/docs/config_yaml.rst
@@ -73,21 +73,6 @@ used to configure module names.
    able to find the old installation directories.
 
 --------------------
-``module_roots``
---------------------
-
-Controls where Spack installs generated module files.  You can customize
-the location for each type of module.  e.g.:
-
-.. code-block:: yaml
-
-   module_roots:
-     tcl:    $spack/share/spack/modules
-     lmod:   $spack/share/spack/lmod
-
-See :ref:`modules` for details.
-
---------------------
 ``build_stage``
 --------------------
 
diff --git a/lib/spack/docs/configuration.rst b/lib/spack/docs/configuration.rst
index a7b0a6c74b..21c8f4afaf 100644
--- a/lib/spack/docs/configuration.rst
+++ b/lib/spack/docs/configuration.rst
@@ -37,8 +37,6 @@ Here is an example ``config.yaml`` file:
 
    config:
      install_tree: $spack/opt/spack
-     module_roots:
-       lmod: $spack/share/spack/lmod
      build_stage:
        - $tempdir/$user/spack-stage
        - ~/.spack/stage
@@ -253,8 +251,6 @@ your configurations look like this:
 
    config:
      install_tree: $spack/opt/spack
-     module_roots:
-       lmod: $spack/share/spack/lmod
      build_stage:
        - $tempdir/$user/spack-stage
        - ~/.spack/stage
@@ -278,8 +274,6 @@ command:
    $ spack config get config
    config:
      install_tree: /some/other/directory
-     module_roots:
-       lmod: $spack/share/spack/lmod
      build_stage:
        - $tempdir/$user/spack-stage
        - ~/.spack/stage
@@ -345,13 +339,11 @@ higher-precedence scope is *prepended* to the defaults. ``spack config
 get config`` shows the result:
 
 .. code-block:: console
-   :emphasize-lines: 7-10
+   :emphasize-lines: 5-8
 
    $ spack config get config
    config:
      install_tree: /some/other/directory
-     module_roots:
-       lmod: $spack/share/spack/lmod
      build_stage:
        - /lustre-scratch/$user/spack
        - ~/mystage
@@ -375,13 +367,11 @@ user config looked like this:
 The merged configuration would look like this:
 
 .. code-block:: console
-   :emphasize-lines: 7-8
+   :emphasize-lines: 5-6
 
    $ spack config get config
    config:
      install_tree: /some/other/directory
-     module_roots:
-       lmod: $spack/share/spack/lmod
      build_stage:
        - /lustre-scratch/$user/spack
        - ~/mystage
@@ -502,9 +492,6 @@ account all scopes. For example, to see the fully merged
      template_dirs:
      - $spack/templates
      directory_layout: {architecture}/{compiler.name}-{compiler.version}/{name}-{version}-{hash}
-     module_roots:
-       tcl: $spack/share/spack/modules
-       lmod: $spack/share/spack/lmod
      build_stage:
      - $tempdir/$user/spack-stage
      - ~/.spack/stage
@@ -552,9 +539,6 @@ down the problem:
    /home/myuser/spack/etc/spack/defaults/config.yaml:23    template_dirs:
    /home/myuser/spack/etc/spack/defaults/config.yaml:24    - $spack/templates
    /home/myuser/spack/etc/spack/defaults/config.yaml:28    directory_layout: {architecture}/{compiler.name}-{compiler.version}/{name}-{version}-{hash}
-   /home/myuser/spack/etc/spack/defaults/config.yaml:32    module_roots:
-   /home/myuser/spack/etc/spack/defaults/config.yaml:33      tcl: $spack/share/spack/modules
-   /home/myuser/spack/etc/spack/defaults/config.yaml:34      lmod: $spack/share/spack/lmod
    /home/myuser/spack/etc/spack/defaults/config.yaml:49    build_stage:
    /home/myuser/spack/etc/spack/defaults/config.yaml:50    - $tempdir/$user/spack-stage
    /home/myuser/spack/etc/spack/defaults/config.yaml:51    - ~/.spack/stage
diff --git a/lib/spack/docs/module_file_support.rst b/lib/spack/docs/module_file_support.rst
index 5ce69dfc91..5aa57f95bf 100644
--- a/lib/spack/docs/module_file_support.rst
+++ b/lib/spack/docs/module_file_support.rst
@@ -181,10 +181,7 @@ to the environment variables listed below the folder name.
 Spack modules can be configured for multiple module sets. The default
 module set is named ``default``. All Spack commands which operate on
 modules default to apply the ``default`` module set, but can be
-applied to any module set in the configuration. Settings applied at
-the root of the configuration (e.g. ``modules:enable`` rather than
-``modules:default:enable``) are applied to the default module set for
-backwards compatibility.
+applied to any module set in the configuration.
 
 """""""""""""""""""""""""
 Changing the modules root
diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py
index 884522b7c2..4b81b1a4b3 100644
--- a/lib/spack/spack/modules/common.py
+++ b/lib/spack/spack/modules/common.py
@@ -57,11 +57,7 @@ import spack.util.spack_yaml as syaml
 #: config section for this file
 def configuration(module_set_name):
     config_path = 'modules:%s' % module_set_name
-    config = spack.config.get(config_path, {})
-    if not config and module_set_name == 'default':
-        # return old format for backward compatibility
-        return spack.config.get('modules', {})
-    return config
+    return spack.config.get(config_path, {})
 
 
 #: Valid tokens for naming scheme and env variable names
@@ -233,11 +229,6 @@ def root_path(name, module_set_name):
     # Root folders where the various module files should be written
     roots = spack.config.get('modules:%s:roots' % module_set_name, {})
 
-    # For backwards compatibility, read the old module roots for default set
-    if module_set_name == 'default':
-        roots = spack.config.merge_yaml(
-            spack.config.get('config:module_roots', {}), roots)
-
     # Merge config values into the defaults so we prefer configured values
     roots = spack.config.merge_yaml(defaults, roots)
 
@@ -698,12 +689,11 @@ class BaseContext(tengine.Context):
     def environment_modifications(self):
         """List of environment modifications to be processed."""
         # Modifications guessed by inspecting the spec prefix
-        std_prefix_inspections = spack.config.get(
-            'modules:prefix_inspections', {})
-        set_prefix_inspections = spack.config.get(
-            'modules:%s:prefix_inspections' % self.conf.name, {})
-        prefix_inspections = spack.config.merge_yaml(
-            std_prefix_inspections, set_prefix_inspections)
+        prefix_inspections = syaml.syaml_dict()
+        spack.config.merge_yaml(prefix_inspections, spack.config.get(
+            'modules:prefix_inspections', {}))
+        spack.config.merge_yaml(prefix_inspections, spack.config.get(
+            'modules:%s:prefix_inspections' % self.conf.name, {}))
 
         use_view = spack.config.get(
             'modules:%s:use_view' % self.conf.name, False)
@@ -939,7 +929,9 @@ def disable_modules():
     """Disable the generation of modulefiles within the context manager."""
     data = {
         'modules:': {
-            'enable': []
+            'default': {
+                'enable': []
+            }
         }
     }
     disable_scope = spack.config.InternalConfigScope('disable_modules', data=data)
diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py
index 06dadc802c..1000fcb5a8 100644
--- a/lib/spack/spack/schema/config.py
+++ b/lib/spack/spack/schema/config.py
@@ -56,22 +56,6 @@ properties = {
                 'type': 'array',
                 'items': {'type': 'string'}
             },
-            'module_roots': {
-                'type': 'object',
-                'additionalProperties': False,
-                'properties': {
-                    'tcl': {'type': 'string'},
-                    'lmod': {'type': 'string'},
-                    'dotkit': {'type': 'string'},
-                },
-                'deprecatedProperties': {
-                    'properties': ['dotkit'],
-                    'message': 'specifying a "dotkit" module root has no '
-                               'effect [support for "dotkit" has been '
-                               'dropped in v0.13.0]',
-                    'error': False
-                },
-            },
             'source_cache': {'type': 'string'},
             'misc_cache': {'type': 'string'},
             'connect_timeout': {'type': 'integer', 'minimum': 0},
@@ -108,6 +92,12 @@ properties = {
                 'items': {'type': 'string'}
             }
         },
+        'deprecatedProperties': {
+            'properties': ['module_roots'],
+            'message': 'config:module_roots has been replaced by '
+                       'modules:[module set]:roots and is ignored',
+            'error': False
+        }
     },
 }
 
diff --git a/lib/spack/spack/schema/modules.py b/lib/spack/spack/schema/modules.py
index 981ccdbda6..a06a459253 100644
--- a/lib/spack/spack/schema/modules.py
+++ b/lib/spack/spack/schema/modules.py
@@ -8,6 +8,8 @@
 .. literalinclude:: _spack_root/lib/spack/spack/schema/modules.py
    :lines: 13-
 """
+import warnings
+
 import spack.schema.environment
 import spack.schema.projections
 
@@ -21,8 +23,8 @@ spec_regex = r'(?!hierarchy|core_specs|verbose|hash_length|whitelist|' \
              r'defaults)(^\w[\w-]*)'
 
 #: Matches a valid name for a module set
-# Banned names are valid entries at that level in the previous schema
-set_regex = r'(?!enable|lmod|tcl|dotkit|prefix_inspections)^\w[\w-]*'
+valid_module_set_name = r'^(?!arch_folder$|lmod$|roots$|enable$|prefix_inspections$|'\
+                        r'tcl$|use_view$)\w[\w-]*$'
 
 #: Matches an anonymous spec, i.e. a spec without a root name
 anonymous_spec_regex = r'^[\^@%+~]'
@@ -117,24 +119,12 @@ module_type_configuration = {
 }
 
 
-#: The "real" module properties -- the actual configuration parameters.
-#: They are separate from ``properties`` because they can appear both
-#: at the top level of a Spack ``modules:`` config (old, deprecated format),
-#: and within a named module set (new format with multiple module sets).
 module_config_properties = {
     'use_view': {'anyOf': [
         {'type': 'string'},
         {'type': 'boolean'}
     ]},
     'arch_folder': {'type': 'boolean'},
-    'prefix_inspections': {
-        'type': 'object',
-        'additionalProperties': False,
-        'patternProperties': {
-            # prefix-relative path to be inspected for existence
-            r'^[\w-]*': array_of_strings
-        }
-    },
     'roots': {
         'type': 'object',
         'properties': {
@@ -147,15 +137,8 @@ module_config_properties = {
         'default': [],
         'items': {
             'type': 'string',
-            'enum': ['tcl', 'dotkit', 'lmod']
-        },
-        'deprecatedProperties': {
-            'properties': ['dotkit'],
-            'message': 'cannot enable "dotkit" in modules.yaml '
-            '[support for "dotkit" has been dropped '
-            'in v0.13.0]',
-            'error': False
-        },
+            'enum': ['tcl', 'lmod']
+        }
     },
     'lmod': {
         'allOf': [
@@ -178,40 +161,54 @@ module_config_properties = {
             {}  # Specific tcl extensions
         ]
     },
-    'dotkit': {
-        'allOf': [
-            # Base configuration
-            module_type_configuration,
-            {}  # Specific dotkit extensions
-        ]
+    'prefix_inspections': {
+        'type': 'object',
+        'additionalProperties': False,
+        'patternProperties': {
+            # prefix-relative path to be inspected for existence
+            r'^[\w-]*': array_of_strings
+        }
     },
 }
 
 
+def deprecation_msg_default_module_set(instance, props):
+    return (
+        'Top-level properties "{0}" in module config are ignored as of Spack v0.18. '
+        'They should be set on the "default" module set. Run\n\n'
+        '\t$ spack config update modules\n\n'
+        'to update the file to the new format'.format('", "'.join(instance))
+    )
+
+
 # Properties for inclusion into other schemas (requires definitions)
 properties = {
     'modules': {
         'type': 'object',
+        'additionalProperties': False,
+        'properties': {
+            'prefix_inspections': {
+                'type': 'object',
+                'additionalProperties': False,
+                'patternProperties': {
+                    # prefix-relative path to be inspected for existence
+                    r'^[\w-]*': array_of_strings
+                }
+            },
+        },
         'patternProperties': {
-            set_regex: {
+            valid_module_set_name: {
                 'type': 'object',
                 'default': {},
                 'additionalProperties': False,
-                'properties': module_config_properties,
-                'deprecatedProperties': {
-                    'properties': ['dotkit'],
-                    'message': 'the "dotkit" section in modules.yaml has no effect'
-                    ' [support for "dotkit" has been dropped in v0.13.0]',
-                    'error': False
-                }
+                'properties': module_config_properties
             },
+            # Deprecated top-level keys (ignored in 0.18 with a warning)
+            '^(arch_folder|lmod|roots|enable|tcl|use_view)$': {}
         },
-        # Available here for backwards compatibility
-        'properties': module_config_properties,
         'deprecatedProperties': {
-            'properties': ['dotkit'],
-            'message': 'the "dotkit" section in modules.yaml has no effect'
-            ' [support for "dotkit" has been dropped in v0.13.0]',
+            'properties': ['arch_folder', 'lmod', 'roots', 'enable', 'tcl', 'use_view'],
+            'message': deprecation_msg_default_module_set,
             'error': False
         }
     }
@@ -225,3 +222,39 @@ schema = {
     'additionalProperties': False,
     'properties': properties,
 }
+
+
+def update(data):
+    """Update the data in place to remove deprecated properties.
+
+    Args:
+        data (dict): dictionary to be updated
+
+    Returns:
+        True if data was changed, False otherwise
+    """
+    changed = False
+
+    deprecated_top_level_keys = ('arch_folder', 'lmod', 'roots', 'enable',
+                                 'tcl', 'use_view')
+
+    # Don't update when we already have a default module set
+    if 'default' in data:
+        if any(key in data for key in deprecated_top_level_keys):
+            warnings.warn('Did not move top-level module properties into "default" '
+                          'module set, because the "default" module set is already '
+                          'defined')
+        return changed
+
+    default = {}
+
+    # Move deprecated top-level keys under "default" module set.
+    for key in deprecated_top_level_keys:
+        if key in data:
+            default[key] = data.pop(key)
+
+    if default:
+        changed = True
+        data['default'] = default
+
+    return changed
diff --git a/lib/spack/spack/test/bootstrap.py b/lib/spack/spack/test/bootstrap.py
index 8c56cd1cf5..f8eea0de52 100644
--- a/lib/spack/spack/test/bootstrap.py
+++ b/lib/spack/spack/test/bootstrap.py
@@ -73,15 +73,15 @@ def test_bootstrap_deactivates_environments(active_mock_environment):
 @pytest.mark.regression('25805')
 def test_bootstrap_disables_modulefile_generation(mutable_config):
     # Be sure to enable both lmod and tcl in modules.yaml
-    spack.config.set('modules:enable', ['tcl', 'lmod'])
+    spack.config.set('modules:default:enable', ['tcl', 'lmod'])
 
-    assert 'tcl' in spack.config.get('modules:enable')
-    assert 'lmod' in spack.config.get('modules:enable')
+    assert 'tcl' in spack.config.get('modules:default:enable')
+    assert 'lmod' in spack.config.get('modules:default:enable')
     with spack.bootstrap.ensure_bootstrap_configuration():
-        assert 'tcl' not in spack.config.get('modules:enable')
-        assert 'lmod' not in spack.config.get('modules:enable')
-    assert 'tcl' in spack.config.get('modules:enable')
-    assert 'lmod' in spack.config.get('modules:enable')
+        assert 'tcl' not in spack.config.get('modules:default:enable')
+        assert 'lmod' not in spack.config.get('modules:default:enable')
+    assert 'tcl' in spack.config.get('modules:default:enable')
+    assert 'lmod' in spack.config.get('modules:default:enable')
 
 
 @pytest.mark.regression('25992')
diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
index a765582a2b..4a27acf9ca 100644
--- a/lib/spack/spack/test/config.py
+++ b/lib/spack/spack/test/config.py
@@ -561,20 +561,6 @@ def test_read_config_override_all(mock_low_high_config, write_config_file):
     }
 
 
-@pytest.mark.regression('23663')
-def test_read_with_default(mock_low_high_config):
-    # this very synthetic example ensures that config.get(path, default)
-    # returns default if any element of path doesn't exist, regardless
-    # of the type of default.
-    spack.config.set('modules', {'enable': []})
-
-    default_conf = spack.config.get('modules:default', 'default')
-    assert default_conf == 'default'
-
-    default_enable = spack.config.get('modules:default:enable', [])
-    assert default_enable == []
-
-
 def test_read_config_override_key(mock_low_high_config, write_config_file):
     write_config_file('config', config_low, 'low')
     write_config_file('config', config_override_key, 'high')
@@ -1100,22 +1086,6 @@ compilers:
 """)
 
 
-@pytest.mark.regression('13045')
-def test_dotkit_in_config_does_not_raise(
-        mock_low_high_config, write_config_file, capsys
-):
-    write_config_file('config',
-                      {'config': {'module_roots': {'dotkit': '/some/path'}}},
-                      'high')
-    spack.main.print_setup_info('sh')
-    captured = capsys.readouterr()
-
-    # Check that we set the variables we expect and that
-    # we throw a a deprecation warning without raising
-    assert '_sp_sys_type' in captured[0]  # stdout
-    assert 'Warning' in captured[1]  # stderr
-
-
 def test_internal_config_section_override(mock_low_high_config,
                                           write_config_file):
     write_config_file('config', config_merge_list, 'low')
diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py
index f2aa048705..feaa9e7905 100644
--- a/lib/spack/spack/test/modules/lmod.py
+++ b/lib/spack/spack/test/modules/lmod.py
@@ -313,23 +313,6 @@ class TestLmod(object):
         projection = writer.spec.format(writer.conf.projections['all'])
         assert projection in writer.layout.use_name
 
-    def test_config_backwards_compat(self, mutable_config):
-        settings = {
-            'enable': ['lmod'],
-            'lmod': {
-                'core_compilers': ['%gcc@0.0.0']
-            }
-        }
-
-        spack.config.set('modules:default', settings)
-        new_format = spack.modules.lmod.configuration('default')
-
-        spack.config.set('modules', settings)
-        old_format = spack.modules.lmod.configuration('default')
-
-        assert old_format == new_format
-        assert old_format == settings['lmod']
-
     def test_modules_relative_to_view(
         self, tmpdir, modulefile_content, module_configuration, install_mockery,
         mock_fetch
diff --git a/lib/spack/spack/test/modules/tcl.py b/lib/spack/spack/test/modules/tcl.py
index 121e47422b..d9e4ad17bd 100644
--- a/lib/spack/spack/test/modules/tcl.py
+++ b/lib/spack/spack/test/modules/tcl.py
@@ -391,25 +391,6 @@ class TestTcl(object):
         content = modulefile_content('mpileaks ^mpich')
         assert len([x for x in content if 'is-loaded' in x]) == 0
 
-    def test_config_backwards_compat(self, mutable_config):
-        settings = {
-            'enable': ['tcl'],
-            'tcl': {
-                'all': {
-                    'conflict': ['{name}']
-                }
-            }
-        }
-
-        spack.config.set('modules:default', settings)
-        new_format = spack.modules.tcl.configuration('default')
-
-        spack.config.set('modules', settings)
-        old_format = spack.modules.tcl.configuration('default')
-
-        assert old_format == new_format
-        assert old_format == settings['tcl']
-
     def test_modules_no_arch(self, factory, module_configuration):
         module_configuration('no_arch')
         module, spec = factory(mpileaks_spec_string)