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`.
This commit is contained in:
Harmen Stoppels 2022-04-08 21:00:35 +02:00 committed by GitHub
parent 13f3bd533d
commit 99083f1706
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 104 additions and 192 deletions

View File

@ -35,13 +35,10 @@ modules:
# These are configurations for the module set named "default" # These are configurations for the module set named "default"
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 # Where to install modules
# roots: roots:
# tcl: $spack/share/spack/modules tcl: $spack/share/spack/modules
# lmod: $spack/share/spack/lmod lmod: $spack/share/spack/lmod
# What type of modules to use # What type of modules to use
enable: enable:
- tcl - tcl

View File

@ -72,21 +72,6 @@ used to configure module names.
packages have been installed will prevent Spack from being packages have been installed will prevent Spack from being
able to find the old installation directories. 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`` ``build_stage``
-------------------- --------------------

View File

@ -37,8 +37,6 @@ Here is an example ``config.yaml`` file:
config: config:
install_tree: $spack/opt/spack install_tree: $spack/opt/spack
module_roots:
lmod: $spack/share/spack/lmod
build_stage: build_stage:
- $tempdir/$user/spack-stage - $tempdir/$user/spack-stage
- ~/.spack/stage - ~/.spack/stage
@ -253,8 +251,6 @@ your configurations look like this:
config: config:
install_tree: $spack/opt/spack install_tree: $spack/opt/spack
module_roots:
lmod: $spack/share/spack/lmod
build_stage: build_stage:
- $tempdir/$user/spack-stage - $tempdir/$user/spack-stage
- ~/.spack/stage - ~/.spack/stage
@ -278,8 +274,6 @@ command:
$ spack config get config $ spack config get config
config: config:
install_tree: /some/other/directory install_tree: /some/other/directory
module_roots:
lmod: $spack/share/spack/lmod
build_stage: build_stage:
- $tempdir/$user/spack-stage - $tempdir/$user/spack-stage
- ~/.spack/stage - ~/.spack/stage
@ -345,13 +339,11 @@ higher-precedence scope is *prepended* to the defaults. ``spack config
get config`` shows the result: get config`` shows the result:
.. code-block:: console .. code-block:: console
:emphasize-lines: 7-10 :emphasize-lines: 5-8
$ spack config get config $ spack config get config
config: config:
install_tree: /some/other/directory install_tree: /some/other/directory
module_roots:
lmod: $spack/share/spack/lmod
build_stage: build_stage:
- /lustre-scratch/$user/spack - /lustre-scratch/$user/spack
- ~/mystage - ~/mystage
@ -375,13 +367,11 @@ user config looked like this:
The merged configuration would look like this: The merged configuration would look like this:
.. code-block:: console .. code-block:: console
:emphasize-lines: 7-8 :emphasize-lines: 5-6
$ spack config get config $ spack config get config
config: config:
install_tree: /some/other/directory install_tree: /some/other/directory
module_roots:
lmod: $spack/share/spack/lmod
build_stage: build_stage:
- /lustre-scratch/$user/spack - /lustre-scratch/$user/spack
- ~/mystage - ~/mystage
@ -502,9 +492,6 @@ account all scopes. For example, to see the fully merged
template_dirs: template_dirs:
- $spack/templates - $spack/templates
directory_layout: {architecture}/{compiler.name}-{compiler.version}/{name}-{version}-{hash} directory_layout: {architecture}/{compiler.name}-{compiler.version}/{name}-{version}-{hash}
module_roots:
tcl: $spack/share/spack/modules
lmod: $spack/share/spack/lmod
build_stage: build_stage:
- $tempdir/$user/spack-stage - $tempdir/$user/spack-stage
- ~/.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:23 template_dirs:
/home/myuser/spack/etc/spack/defaults/config.yaml:24 - $spack/templates /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: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:49 build_stage:
/home/myuser/spack/etc/spack/defaults/config.yaml:50 - $tempdir/$user/spack-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 /home/myuser/spack/etc/spack/defaults/config.yaml:51 - ~/.spack/stage

View File

@ -181,10 +181,7 @@ to the environment variables listed below the folder name.
Spack modules can be configured for multiple module sets. The default Spack modules can be configured for multiple module sets. The default
module set is named ``default``. All Spack commands which operate on module set is named ``default``. All Spack commands which operate on
modules default to apply the ``default`` module set, but can be modules default to apply the ``default`` module set, but can be
applied to any module set in the configuration. Settings applied at applied to any module set in the configuration.
the root of the configuration (e.g. ``modules:enable`` rather than
``modules:default:enable``) are applied to the default module set for
backwards compatibility.
""""""""""""""""""""""""" """""""""""""""""""""""""
Changing the modules root Changing the modules root

View File

@ -57,11 +57,7 @@
#: config section for this file #: config section for this file
def configuration(module_set_name): def configuration(module_set_name):
config_path = 'modules:%s' % module_set_name config_path = 'modules:%s' % module_set_name
config = spack.config.get(config_path, {}) return 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
#: Valid tokens for naming scheme and env variable names #: 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 # Root folders where the various module files should be written
roots = spack.config.get('modules:%s:roots' % module_set_name, {}) 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 # Merge config values into the defaults so we prefer configured values
roots = spack.config.merge_yaml(defaults, roots) roots = spack.config.merge_yaml(defaults, roots)
@ -698,12 +689,11 @@ def configure_options(self):
def environment_modifications(self): def environment_modifications(self):
"""List of environment modifications to be processed.""" """List of environment modifications to be processed."""
# Modifications guessed by inspecting the spec prefix # Modifications guessed by inspecting the spec prefix
std_prefix_inspections = spack.config.get( prefix_inspections = syaml.syaml_dict()
'modules:prefix_inspections', {}) spack.config.merge_yaml(prefix_inspections, spack.config.get(
set_prefix_inspections = spack.config.get( 'modules:prefix_inspections', {}))
'modules:%s:prefix_inspections' % self.conf.name, {}) spack.config.merge_yaml(prefix_inspections, spack.config.get(
prefix_inspections = spack.config.merge_yaml( 'modules:%s:prefix_inspections' % self.conf.name, {}))
std_prefix_inspections, set_prefix_inspections)
use_view = spack.config.get( use_view = spack.config.get(
'modules:%s:use_view' % self.conf.name, False) 'modules:%s:use_view' % self.conf.name, False)
@ -939,7 +929,9 @@ def disable_modules():
"""Disable the generation of modulefiles within the context manager.""" """Disable the generation of modulefiles within the context manager."""
data = { data = {
'modules:': { 'modules:': {
'enable': [] 'default': {
'enable': []
}
} }
} }
disable_scope = spack.config.InternalConfigScope('disable_modules', data=data) disable_scope = spack.config.InternalConfigScope('disable_modules', data=data)

View File

@ -56,22 +56,6 @@
'type': 'array', 'type': 'array',
'items': {'type': 'string'} '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'}, 'source_cache': {'type': 'string'},
'misc_cache': {'type': 'string'}, 'misc_cache': {'type': 'string'},
'connect_timeout': {'type': 'integer', 'minimum': 0}, 'connect_timeout': {'type': 'integer', 'minimum': 0},
@ -108,6 +92,12 @@
'items': {'type': 'string'} 'items': {'type': 'string'}
} }
}, },
'deprecatedProperties': {
'properties': ['module_roots'],
'message': 'config:module_roots has been replaced by '
'modules:[module set]:roots and is ignored',
'error': False
}
}, },
} }

View File

@ -8,6 +8,8 @@
.. literalinclude:: _spack_root/lib/spack/spack/schema/modules.py .. literalinclude:: _spack_root/lib/spack/spack/schema/modules.py
:lines: 13- :lines: 13-
""" """
import warnings
import spack.schema.environment import spack.schema.environment
import spack.schema.projections import spack.schema.projections
@ -21,8 +23,8 @@
r'defaults)(^\w[\w-]*)' r'defaults)(^\w[\w-]*)'
#: Matches a valid name for a module set #: Matches a valid name for a module set
# Banned names are valid entries at that level in the previous schema valid_module_set_name = r'^(?!arch_folder$|lmod$|roots$|enable$|prefix_inspections$|'\
set_regex = r'(?!enable|lmod|tcl|dotkit|prefix_inspections)^\w[\w-]*' r'tcl$|use_view$)\w[\w-]*$'
#: Matches an anonymous spec, i.e. a spec without a root name #: Matches an anonymous spec, i.e. a spec without a root name
anonymous_spec_regex = r'^[\^@%+~]' anonymous_spec_regex = r'^[\^@%+~]'
@ -117,24 +119,12 @@
} }
#: 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 = { module_config_properties = {
'use_view': {'anyOf': [ 'use_view': {'anyOf': [
{'type': 'string'}, {'type': 'string'},
{'type': 'boolean'} {'type': 'boolean'}
]}, ]},
'arch_folder': {'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': { 'roots': {
'type': 'object', 'type': 'object',
'properties': { 'properties': {
@ -147,15 +137,8 @@
'default': [], 'default': [],
'items': { 'items': {
'type': 'string', 'type': 'string',
'enum': ['tcl', 'dotkit', 'lmod'] 'enum': ['tcl', 'lmod']
}, }
'deprecatedProperties': {
'properties': ['dotkit'],
'message': 'cannot enable "dotkit" in modules.yaml '
'[support for "dotkit" has been dropped '
'in v0.13.0]',
'error': False
},
}, },
'lmod': { 'lmod': {
'allOf': [ 'allOf': [
@ -178,40 +161,54 @@
{} # Specific tcl extensions {} # Specific tcl extensions
] ]
}, },
'dotkit': { 'prefix_inspections': {
'allOf': [ 'type': 'object',
# Base configuration 'additionalProperties': False,
module_type_configuration, 'patternProperties': {
{} # Specific dotkit extensions # 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 for inclusion into other schemas (requires definitions)
properties = { properties = {
'modules': { 'modules': {
'type': 'object', 'type': 'object',
'patternProperties': { 'additionalProperties': False,
set_regex: { 'properties': {
'prefix_inspections': {
'type': 'object', 'type': 'object',
'default': {},
'additionalProperties': False, 'additionalProperties': False,
'properties': module_config_properties, 'patternProperties': {
'deprecatedProperties': { # prefix-relative path to be inspected for existence
'properties': ['dotkit'], r'^[\w-]*': array_of_strings
'message': 'the "dotkit" section in modules.yaml has no effect'
' [support for "dotkit" has been dropped in v0.13.0]',
'error': False
} }
}, },
}, },
# Available here for backwards compatibility 'patternProperties': {
'properties': module_config_properties, valid_module_set_name: {
'type': 'object',
'default': {},
'additionalProperties': False,
'properties': module_config_properties
},
# Deprecated top-level keys (ignored in 0.18 with a warning)
'^(arch_folder|lmod|roots|enable|tcl|use_view)$': {}
},
'deprecatedProperties': { 'deprecatedProperties': {
'properties': ['dotkit'], 'properties': ['arch_folder', 'lmod', 'roots', 'enable', 'tcl', 'use_view'],
'message': 'the "dotkit" section in modules.yaml has no effect' 'message': deprecation_msg_default_module_set,
' [support for "dotkit" has been dropped in v0.13.0]',
'error': False 'error': False
} }
} }
@ -225,3 +222,39 @@
'additionalProperties': False, 'additionalProperties': False,
'properties': properties, '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

View File

@ -73,15 +73,15 @@ def test_bootstrap_deactivates_environments(active_mock_environment):
@pytest.mark.regression('25805') @pytest.mark.regression('25805')
def test_bootstrap_disables_modulefile_generation(mutable_config): def test_bootstrap_disables_modulefile_generation(mutable_config):
# Be sure to enable both lmod and tcl in modules.yaml # 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 'tcl' in spack.config.get('modules:default:enable')
assert 'lmod' in spack.config.get('modules:enable') assert 'lmod' in spack.config.get('modules:default:enable')
with spack.bootstrap.ensure_bootstrap_configuration(): with spack.bootstrap.ensure_bootstrap_configuration():
assert 'tcl' not in spack.config.get('modules:enable') assert 'tcl' not in spack.config.get('modules:default:enable')
assert 'lmod' not in spack.config.get('modules:enable') assert 'lmod' not in spack.config.get('modules:default:enable')
assert 'tcl' in spack.config.get('modules:enable') assert 'tcl' in spack.config.get('modules:default:enable')
assert 'lmod' in spack.config.get('modules:enable') assert 'lmod' in spack.config.get('modules:default:enable')
@pytest.mark.regression('25992') @pytest.mark.regression('25992')

View File

@ -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): 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_low, 'low')
write_config_file('config', config_override_key, 'high') write_config_file('config', config_override_key, 'high')
@ -1100,22 +1086,6 @@ def test_bad_compilers_yaml(tmpdir):
""") """)
@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, def test_internal_config_section_override(mock_low_high_config,
write_config_file): write_config_file):
write_config_file('config', config_merge_list, 'low') write_config_file('config', config_merge_list, 'low')

View File

@ -313,23 +313,6 @@ def test_projections_all(self, factory, module_configuration):
projection = writer.spec.format(writer.conf.projections['all']) projection = writer.spec.format(writer.conf.projections['all'])
assert projection in writer.layout.use_name 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( def test_modules_relative_to_view(
self, tmpdir, modulefile_content, module_configuration, install_mockery, self, tmpdir, modulefile_content, module_configuration, install_mockery,
mock_fetch mock_fetch

View File

@ -391,25 +391,6 @@ def test_autoload_with_constraints(
content = modulefile_content('mpileaks ^mpich') content = modulefile_content('mpileaks ^mpich')
assert len([x for x in content if 'is-loaded' in x]) == 0 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): def test_modules_no_arch(self, factory, module_configuration):
module_configuration('no_arch') module_configuration('no_arch')
module, spec = factory(mpileaks_spec_string) module, spec = factory(mpileaks_spec_string)