From f7195123d44f873a9f5fbca805cfd5a65aa8726c Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Wed, 23 Dec 2020 20:29:38 -0800 Subject: [PATCH] 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(-) diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py index 0b63bad6a7f..38c2c4cdb7d 100644 --- a/lib/spack/spack/environment.py +++ b/lib/spack/spack/environment.py @@ -685,7 +685,7 @@ def _read_manifest(self, f, raw_yaml=None): 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 @@ -707,10 +707,11 @@ def _read_manifest(self, f, raw_yaml=None): 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 8c485a4bfdb..1ddca90cc64 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 @@ # 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 _deprecated_properties(validator, deprecated, instance, schema): 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 49a21268774..c2d75d9d1f7 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