From 6947951aaf9954b1dfd12ca7a9266d7335f07105 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Wed, 23 Dec 2020 20:29:38 -0800 Subject: bugfix: do not write empty default dicts/lists in envs (#20526) Environment yaml files should not have default values written to them. To accomplish this, we change the validator to not add the default values to yaml. We rely on the code to set defaults for all values (and use defaulting getters like dict.get(key, default)). Includes regression test. --- lib/spack/spack/environment.py | 7 +++--- lib/spack/spack/schema/__init__.py | 44 -------------------------------------- lib/spack/spack/test/cmd/env.py | 34 +++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 47 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py index c098ed4b36..698ea91870 100644 --- a/lib/spack/spack/environment.py +++ b/lib/spack/spack/environment.py @@ -686,7 +686,7 @@ class Environment(object): else: self.spec_lists[name] = user_specs - spec_list = config_dict(self.yaml).get(user_speclist_name) + spec_list = config_dict(self.yaml).get(user_speclist_name, []) user_specs = SpecList(user_speclist_name, [s for s in spec_list if s], self.spec_lists.copy()) self.spec_lists[user_speclist_name] = user_specs @@ -708,10 +708,11 @@ class Environment(object): self.views = {} # Retrieve the current concretization strategy configuration = config_dict(self.yaml) - self.concretization = configuration.get('concretization') + # default concretization to separately + self.concretization = configuration.get('concretization', 'separately') # Retrieve dev-build packages: - self.dev_specs = configuration['develop'] + self.dev_specs = configuration.get('develop', {}) for name, entry in self.dev_specs.items(): # spec must include a concrete version assert Spec(entry['spec']).version.concrete diff --git a/lib/spack/spack/schema/__init__.py b/lib/spack/spack/schema/__init__.py index 8c485a4bfd..1ddca90cc6 100644 --- a/lib/spack/spack/schema/__init__.py +++ b/lib/spack/spack/schema/__init__.py @@ -4,9 +4,6 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) """This module contains jsonschema files for all of Spack's YAML formats.""" -import copy -import re - import six import llnl.util.lang @@ -18,45 +15,6 @@ import spack.spec # and increases the start-up time def _make_validator(): import jsonschema - _validate_properties = jsonschema.Draft4Validator.VALIDATORS["properties"] - _validate_pattern_properties = jsonschema.Draft4Validator.VALIDATORS[ - "patternProperties" - ] - - def _set_defaults(validator, properties, instance, schema): - """Adds support for the 'default' attribute in 'properties'. - - ``jsonschema`` does not handle this out of the box -- it only - validates. This allows us to set default values for configs - where certain fields are `None` b/c they're deleted or - commented out. - """ - for property, subschema in six.iteritems(properties): - if "default" in subschema: - instance.setdefault( - property, copy.deepcopy(subschema["default"])) - for err in _validate_properties( - validator, properties, instance, schema): - yield err - - def _set_pp_defaults(validator, properties, instance, schema): - """Adds support for the 'default' attribute in 'patternProperties'. - - ``jsonschema`` does not handle this out of the box -- it only - validates. This allows us to set default values for configs - where certain fields are `None` b/c they're deleted or - commented out. - """ - for property, subschema in six.iteritems(properties): - if "default" in subschema: - if isinstance(instance, dict): - for key, val in six.iteritems(instance): - if re.match(property, key) and val is None: - instance[key] = copy.deepcopy(subschema["default"]) - - for err in _validate_pattern_properties( - validator, properties, instance, schema): - yield err def _validate_spec(validator, is_spec, instance, schema): """Check if the attributes on instance are valid specs.""" @@ -101,8 +59,6 @@ def _make_validator(): return jsonschema.validators.extend( jsonschema.Draft4Validator, { "validate_spec": _validate_spec, - "properties": _set_defaults, - "patternProperties": _set_pp_defaults, "deprecatedProperties": _deprecated_properties } ) diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index 49a2126877..c2d75d9d1f 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -2162,6 +2162,40 @@ def test_env_write_only_non_default(): assert yaml == ev.default_manifest_yaml +@pytest.mark.regression('20526') +def test_env_write_only_non_default_nested(tmpdir): + # setup an environment file + # the environment includes configuration because nested configs proved the + # most difficult to avoid writing. + filename = 'spack.yaml' + filepath = str(tmpdir.join(filename)) + contents = """\ +env: + specs: + - matrix: + - [mpileaks] + packages: + mpileaks: + compiler: [gcc] + view: true +""" + + # create environment with some structure + with open(filepath, 'w') as f: + f.write(contents) + env('create', 'test', filepath) + + # concretize + with ev.read('test') as e: + concretize() + e.write() + + with open(e.manifest_path, 'r') as f: + manifest = f.read() + + assert manifest == contents + + @pytest.fixture def packages_yaml_v015(tmpdir): """Return the path to an existing manifest in the v0.15.x format -- cgit v1.2.3-60-g2f50