environments: only write when necessary (#13546)

This changes Spack environments so that the YAML file associated with the environment is *only* written when necessary (i.e., if it is changed *by spack*).  The lockfile is still written out as before.

There is a larger question here of which part of Spack should be responsible for setting defaults in config files, and how we can get rid of empty lists and data structures currently cluttering files like `compilers.yaml`.  But that probably requires a rework of the default-setting validator in `spack.config`, as well as the code that uses `spack.config`.  This will at least help for `spack.yaml`.

lib/spack/spack/cmd/release_jobs.py

@@ -409,9 +409,7 @@ def find_matching_config(spec, ci_mappings):
 def release_jobs(parser, args):
     env = ev.get_env(args, 'release-jobs', required=True)
 
-    # FIXME: What's the difference between one that opens with 'spack'
-    # and one that opens with 'env'?  This will only handle the former.
-    yaml_root = env.yaml['spack']
+    yaml_root = ev.config_dict(env.yaml)
 
     if 'gitlab-ci' not in yaml_root:
         tty.die('Environment yaml does not have "gitlab-ci" section')

lib/spack/spack/environment.py

@@ -70,8 +70,7 @@
 # configuration settings.
 spack:
   # add package specs to the `specs` list
-  specs:
-  -
+  specs: []
   view: true
 """
 #: regex for validating enviroment names
@@ -391,20 +390,31 @@ def all_environments():
 
 
 def validate(data, filename=None):
+    # validating changes data by adding defaults. Return validated data
+    validate_data = copy.deepcopy(data)
+    # HACK to fully copy ruamel CommentedMap that doesn't provide copy method
+    import ruamel.yaml as yaml
+    setattr(
+        validate_data,
+        yaml.comments.Comment.attrib,
+        getattr(data, yaml.comments.Comment.attrib, yaml.comments.Comment())
+    )
+
     import jsonschema
     try:
-        spack.schema.Validator(spack.schema.env.schema).validate(data)
+        spack.schema.Validator(spack.schema.env.schema).validate(validate_data)
     except jsonschema.ValidationError as e:
         raise spack.config.ConfigFormatError(
             e, data, filename, e.instance.lc.line + 1)
+    return validate_data
 
 
 def _read_yaml(str_or_file):
     """Read YAML from a file for round-trip parsing."""
     data = syaml.load_config(str_or_file)
     filename = getattr(str_or_file, 'name', None)
-    validate(data, filename)
-    return data
+    default_data = validate(data, filename)
+    return (data, default_data)
 
 
 def _write_yaml(data, str_or_file):
@@ -444,6 +454,13 @@ def __init__(self, root, projections={}, select=[], exclude=[],
                                             for e in self.exclude)
         self.link = link
 
+    def __eq__(self, other):
+        return all([self.root == other.root,
+                    self.projections == other.projections,
+                    self.select == other.select,
+                    self.exclude == other.exclude,
+                    self.link == other.link])
+
     def to_dict(self):
         ret = {'root': self.root}
         if self.projections:
@@ -540,7 +557,7 @@ def __init__(self, path, init_file=None, with_view=None):
                     self._read_lockfile(f)
                     self._set_user_specs_from_lockfile()
                 else:
-                    self._read_manifest(f)
+                    self._read_manifest(f, raw_yaml=default_manifest_yaml)
         else:
             default_manifest = not os.path.exists(self.manifest_path)
             if default_manifest:
@@ -573,9 +590,13 @@ def __init__(self, path, init_file=None, with_view=None):
         # If with_view is None, then defer to the view settings determined by
         # the manifest file
 
-    def _read_manifest(self, f):
+    def _read_manifest(self, f, raw_yaml=None):
         """Read manifest file and set up user specs."""
-        self.yaml = _read_yaml(f)
+        if raw_yaml:
+            _, self.yaml = _read_yaml(f)
+            self.raw_yaml, _ = _read_yaml(raw_yaml)
+        else:
+            self.raw_yaml, self.yaml = _read_yaml(f)
 
         self.spec_lists = OrderedDict()
 
@@ -1311,6 +1332,9 @@ def write(self):
         # ensure path in var/spack/environments
         fs.mkdirp(self.path)
 
+        yaml_dict = config_dict(self.yaml)
+        raw_yaml_dict = config_dict(self.raw_yaml)
+
         if self.specs_by_hash:
             # ensure the prefix/.env directory exists
             fs.mkdirp(self.env_subdir_path)
@@ -1345,8 +1369,7 @@ def write(self):
                 # The primary list is handled differently
                 continue
 
-            conf = config_dict(self.yaml)
-            active_yaml_lists = [l for l in conf.get('definitions', [])
+            active_yaml_lists = [l for l in yaml_dict.get('definitions', [])
                                  if name in l and
                                  _eval_conditional(l.get('when', 'True'))]
 
@@ -1370,10 +1393,11 @@ def write(self):
         # put the new user specs in the YAML.
         # This can be done directly because there can't be multiple definitions
         # nor when clauses for `specs` list.
-        yaml_spec_list = config_dict(self.yaml).setdefault(user_speclist_name,
-                                                           [])
+        yaml_spec_list = yaml_dict.setdefault(user_speclist_name,
+                                              [])
         yaml_spec_list[:] = self.user_specs.yaml_list
 
+        # Construct YAML representation of view
         default_name = default_view_name
         if self.views and len(self.views) == 1 and default_name in self.views:
             path = self.default_view.root
@@ -1390,18 +1414,26 @@ def write(self):
         else:
             view = False
 
-        yaml_dict = config_dict(self.yaml)
-        if view is not True:
-            # The default case is to keep an active view inside of the
-            # Spack environment directory. To avoid cluttering the config,
-            # we omit the setting in this case.
-            yaml_dict['view'] = view
-        elif 'view' in yaml_dict:
-            del yaml_dict['view']
+        yaml_dict['view'] = view
+
+        # Remove yaml sections that are shadowing defaults
+        # construct garbage path to ensure we don't find a manifest by accident
+        bare_env = Environment(os.path.join(self.manifest_path, 'garbage'),
+                               with_view=self.view_path_default)
+        keys_present = list(yaml_dict.keys())
+        for key in keys_present:
+            if yaml_dict[key] == config_dict(bare_env.yaml).get(key, None):
+                if key not in raw_yaml_dict:
+                    del yaml_dict[key]
 
         # if all that worked, write out the manifest file at the top level
-        with fs.write_tmp_and_move(self.manifest_path) as f:
-            _write_yaml(self.yaml, f)
+        # Only actually write if it has changed or was never written
+        changed = self.yaml != self.raw_yaml
+        written = os.path.exists(self.manifest_path)
+        if changed or not written:
+            self.raw_yaml = copy.deepcopy(self.yaml)
+            with fs.write_tmp_and_move(self.manifest_path) as f:
+                _write_yaml(self.yaml, f)
 
         # TODO: for operations that just add to the env (install etc.) this
         # could just call update_view

lib/spack/spack/schema/env.py

@@ -63,6 +63,7 @@
                 {
                     'include': {
                         'type': 'array',
+                        'default': [],
                         'items': {
                             'type': 'string'
                         },

lib/spack/spack/test/cmd/env.py

@@ -1749,3 +1749,13 @@ def test_duplicate_packages_raise_when_concretizing_together():
 
     with pytest.raises(ev.SpackEnvironmentError, match=r'cannot contain more'):
         e.concretize()
+
+
+def test_env_write_only_non_default():
+    print(env('create', 'test'))
+
+    e = ev.read('test')
+    with open(e.manifest_path, 'r') as f:
+        yaml = f.read()
+
+    assert yaml == ev.default_manifest_yaml