bst-fmt: Allow greater control over node ordercoldtom/bst-fmt

Allow authors of plugins to utilise the Plugin.keyorder attribute to
define an order for the sub-headers in config. Modify `bst fmt` to
modify the yaml into that order. Add canonical keyorder for every plugin
already in core.

In terms of implementation, this adds a custom yaml dumper subclassed
from ruamel.yaml's RoundTripDumper, which will dump a dict in the order
defined in the keyorder attribute. The Plugin class is given a keyorder
attribute, which defines the order in which keys should be dumped. The
element and source classes add the common fields used in them, so as to
minimise work for plugin authors. Plugin authors should add their custom
keys at configure time.

This causes some stripping of comments, which seems to be due to a bug
in the ruamel.yaml RoundTripDumper, as that also strips the same
comments. It also will remove blank spaces, again probably due to
limitations in ruamel.

20 files changed, 67 insertions, 3 deletions

diff --git a/buildstream/_frontend/cli.py b/buildstream/_frontend/cli.py
index e9eecd932..c8df1810c 100644
--- a/buildstream/_frontend/cli.py
+++ b/buildstream/_frontend/cli.py
@@ -354,6 +354,7 @@ def build(app, elements, all_, track_, track_save, track_all, track_except, trac
                          track_cross_junctions=track_cross_junctions,
                          build_all=all_)
 
+
 ##################################################################
 #                         Format Command                         #
 ##################################################################
diff --git a/buildstream/_yaml.py b/buildstream/_yaml.py
index 8d7302b80..2d659c7a3 100644
--- a/buildstream/_yaml.py
+++ b/buildstream/_yaml.py
@@ -51,6 +51,31 @@ class ProvenanceFile():
         self.project = project
 
 
+# A custom yaml dumper to reorder keys in dicts to a canonical order, defined
+# in each plugin
+class BstFormatter(yaml.RoundTripDumper):
+
+    keyorder = []
+
+    @classmethod
+    def _iter_in_global_order(cls, mapping):
+        for key in cls.keyorder:
+            if key in mapping.keys():
+                yield key, mapping[key]
+        for key in sorted(mapping.keys()):
+            if key not in cls.keyorder:
+                yield key, mapping[key]
+
+    @classmethod
+    def _represent_dict(cls, dumper, mapping):
+        return dumper.represent_mapping('tag:yaml.org,2002:map', cls._iter_in_global_order(mapping))
+
+    def __init__(self, *args, **kwargs):
+        yaml.RoundTripDumper.__init__(self, *args, **kwargs)
+        self.no_newline = False
+        self.add_representer(dict, self._represent_dict)
+
+
 # Provenance tracks the origin of a given node in the parsed dictionary.
 #
 # Args:
@@ -244,15 +269,16 @@ def load_data(data, file=None, copy_tree=False):
 # Args:
 #    node (dict): A node previously loaded with _yaml.load() above
 #    filename (str): The YAML file to load
+#    dumper (yaml.Dumper): The yaml dumper to be used
 #
-def dump(node, filename=None):
+def dump(node, filename=None, dumper=yaml.RoundTripDumper):
     with ExitStack() as stack:
         if filename:
             from . import utils
             f = stack.enter_context(utils.save_file_atomic(filename, 'w'))
         else:
             f = sys.stdout
-        yaml.round_trip_dump(node, f)
+        yaml.round_trip_dump(node, f, Dumper=dumper)
 
 
 # node_decorated_copy()
diff --git a/buildstream/buildelement.py b/buildstream/buildelement.py
index 6ef060f12..0b83ea0e2 100644
--- a/buildstream/buildelement.py
+++ b/buildstream/buildelement.py
@@ -172,6 +172,7 @@ class BuildElement(Element):
         for command_name in _legacy_command_steps:
             if command_name in _command_steps:
                 self.__commands[command_name] = self.__get_commands(node, command_name)
+                self.keyorder.append(command_name)
             else:
                 self.__commands[command_name] = []
 
diff --git a/buildstream/element.py b/buildstream/element.py
index 9a9e0157d..a8ee747e4 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -269,6 +269,9 @@ class Element(Plugin):
                 # This will taint the artifact, disable pushing.
                 self.__sandbox_config_supported = False
 
+        self.keyorder = ['kind', 'description', 'depends', 'variables',
+                         'environment', 'config', 'public', 'sandbox', 'sources']
+
     def __lt__(self, other):
         return self.name < other.name
 
@@ -1338,7 +1341,17 @@ class Element(Plugin):
     #
     def _format(self):
         provenance = self._get_provenance()
-        _yaml.dump(provenance.toplevel, provenance.filename.name)
+
+        _yaml.BstFormatter.keyorder = self.keyorder
+
+        # We need to add the key orders for each source into the
+        # global keyorder, as sources are dumped with the element.
+        for s in self.sources():
+            for key in s.keyorder:
+                if key not in _yaml.BstFormatter.keyorder:
+                    _yaml.BstFormatter.keyorder += s.keyorder
+
+        _yaml.dump(provenance.toplevel, provenance.filename.name, dumper=_yaml.BstFormatter)
 
     # _prepare_sandbox():
     #
diff --git a/buildstream/plugin.py b/buildstream/plugin.py
index 2f51c8807..181a31962 100644
--- a/buildstream/plugin.py
+++ b/buildstream/plugin.py
@@ -188,6 +188,8 @@ class Plugin():
         self.__kind = modulename.split('.')[-1]
         self.debug("Created: {}".format(self))
 
+        self.keyorder = []
+
     def __del__(self):
         # Dont send anything through the Message() pipeline at destruction time,
         # any subsequent lookup of plugin by unique id would raise KeyError.
diff --git a/buildstream/plugins/elements/compose.py b/buildstream/plugins/elements/compose.py
index d61a324cc..ed4da4dba 100644
--- a/buildstream/plugins/elements/compose.py
+++ b/buildstream/plugins/elements/compose.py
@@ -70,6 +70,8 @@ class ComposeElement(Element):
         self.exclude = self.node_get_member(node, list, 'exclude')
         self.include_orphans = self.node_get_member(node, bool, 'include-orphans')
 
+        self.keyorder += ['integrate', 'include', 'exclude', 'include-orphans']
+
     def preflight(self):
         pass
 
diff --git a/buildstream/plugins/elements/filter.py b/buildstream/plugins/elements/filter.py
index 672325304..9f86baacf 100644
--- a/buildstream/plugins/elements/filter.py
+++ b/buildstream/plugins/elements/filter.py
@@ -65,6 +65,8 @@ class FilterElement(Element):
         self.exclude = self.node_get_member(node, list, 'exclude')
         self.include_orphans = self.node_get_member(node, bool, 'include-orphans')
 
+        self.keyorder += ['include', 'exclude', 'include-orphans']
+
     def preflight(self):
         # Exactly one build-depend is permitted
         build_deps = list(self.dependencies(Scope.BUILD, recurse=False))
diff --git a/buildstream/plugins/elements/junction.py b/buildstream/plugins/elements/junction.py
index 7f9817359..2a5b17fdb 100644
--- a/buildstream/plugins/elements/junction.py
+++ b/buildstream/plugins/elements/junction.py
@@ -140,6 +140,7 @@ class JunctionElement(Element):
     def configure(self, node):
         self.path = self.node_get_member(node, str, 'path', default='')
         self.options = self.node_get_member(node, Mapping, 'options', default={})
+        self.keyorder += ['path', 'options']
 
     def preflight(self):
         pass
diff --git a/buildstream/plugins/elements/script.py b/buildstream/plugins/elements/script.py
index 4e422c5db..b695fade2 100644
--- a/buildstream/plugins/elements/script.py
+++ b/buildstream/plugins/elements/script.py
@@ -51,6 +51,7 @@ class ScriptElement(buildstream.ScriptElement):
         self.node_validate(node, [
             'commands', 'root-read-only', 'layout'
         ])
+        self.keyorder += ['layout', 'root-read-only', 'commands']
 
         cmds = self.node_subst_list(node, "commands")
         self.add_commands("commands", cmds)
diff --git a/buildstream/plugins/sources/_downloadablefilesource.py b/buildstream/plugins/sources/_downloadablefilesource.py
index f5c5b3d08..4cb2d50f5 100644
--- a/buildstream/plugins/sources/_downloadablefilesource.py
+++ b/buildstream/plugins/sources/_downloadablefilesource.py
@@ -82,6 +82,8 @@ class DownloadableFileSource(Source):
         self.url = self.translate_url(self.original_url)
         self._warn_deprecated_etag(node)
 
+        self.keyorder += ['url', 'ref', 'etag']
+
     def preflight(self):
         return
 
diff --git a/buildstream/plugins/sources/bzr.py b/buildstream/plugins/sources/bzr.py
index f52472918..544bf2dc9 100644
--- a/buildstream/plugins/sources/bzr.py
+++ b/buildstream/plugins/sources/bzr.py
@@ -68,6 +68,8 @@ class BzrSource(Source):
     def configure(self, node):
         self.node_validate(node, ['url', 'track', 'ref'] + Source.COMMON_CONFIG_KEYS)
 
+        self.keyorder += ['url', 'track', 'ref']
+
         self.original_url = self.node_get_member(node, str, 'url')
         self.tracking = self.node_get_member(node, str, 'track')
         self.ref = self.node_get_member(node, str, 'ref', None)
diff --git a/buildstream/plugins/sources/git.py b/buildstream/plugins/sources/git.py
index 74d632b6d..e93517c9e 100644
--- a/buildstream/plugins/sources/git.py
+++ b/buildstream/plugins/sources/git.py
@@ -506,6 +506,8 @@ class GitSource(Source):
                        'track-tags', 'tags']
         self.node_validate(node, config_keys + Source.COMMON_CONFIG_KEYS)
 
+        self.keyorder += config_keys
+
         tags_node = self.node_get_member(node, list, 'tags', [])
         for tag_node in tags_node:
             self.node_validate(tag_node, ['tag', 'commit', 'annotated'])
diff --git a/buildstream/plugins/sources/local.py b/buildstream/plugins/sources/local.py
index 55cdc14d3..c6b6963bc 100644
--- a/buildstream/plugins/sources/local.py
+++ b/buildstream/plugins/sources/local.py
@@ -53,6 +53,7 @@ class LocalSource(Source):
 
     def configure(self, node):
         self.node_validate(node, ['path'] + Source.COMMON_CONFIG_KEYS)
+        self.keyorder += ['path']
         self.path = self.node_get_project_path(node, 'path')
         self.fullpath = os.path.join(self.get_project_directory(), self.path)
 
diff --git a/buildstream/plugins/sources/ostree.py b/buildstream/plugins/sources/ostree.py
index 770cfd55f..1bc4ef6fc 100644
--- a/buildstream/plugins/sources/ostree.py
+++ b/buildstream/plugins/sources/ostree.py
@@ -65,6 +65,7 @@ class OSTreeSource(Source):
     def configure(self, node):
 
         self.node_validate(node, ['url', 'ref', 'track', 'gpg-key'] + Source.COMMON_CONFIG_KEYS)
+        self.keyorder += ['url', 'track', 'ref', 'gpg-key']
 
         self.original_url = self.node_get_member(node, str, 'url')
         self.url = self.translate_url(self.original_url)
diff --git a/buildstream/plugins/sources/patch.py b/buildstream/plugins/sources/patch.py
index 8e833b411..48aab13bd 100644
--- a/buildstream/plugins/sources/patch.py
+++ b/buildstream/plugins/sources/patch.py
@@ -53,6 +53,7 @@ class PatchSource(Source):
     # pylint: disable=attribute-defined-outside-init
 
     def configure(self, node):
+        self.keyorder += ['strip-level', 'path']
         self.path = self.node_get_project_path(node, 'path',
                                                check_is_file=True)
         self.strip_level = self.node_get_member(node, int, "strip-level", 1)
diff --git a/buildstream/plugins/sources/pip.py b/buildstream/plugins/sources/pip.py
index abef1fd0d..04e8bab4e 100644
--- a/buildstream/plugins/sources/pip.py
+++ b/buildstream/plugins/sources/pip.py
@@ -111,6 +111,7 @@ class PipSource(Source):
     def configure(self, node):
         self.node_validate(node, ['url', 'packages', 'ref', 'requirements-files'] +
                            Source.COMMON_CONFIG_KEYS)
+        self.keyorder += ['url', 'packages', 'ref', 'requirements-files']
         self.ref = self.node_get_member(node, str, 'ref', None)
         self.original_url = self.node_get_member(node, str, 'url', _PYPI_INDEX_URL)
         self.index_url = self.translate_url(self.original_url)
diff --git a/buildstream/plugins/sources/remote.py b/buildstream/plugins/sources/remote.py
index a6b02fd1c..9247c12fc 100644
--- a/buildstream/plugins/sources/remote.py
+++ b/buildstream/plugins/sources/remote.py
@@ -61,6 +61,7 @@ class RemoteSource(DownloadableFileSource):
 
     def configure(self, node):
         super().configure(node)
+        self.keyorder += ['filename']
 
         self.filename = self.node_get_member(node, str, 'filename', os.path.basename(self.url))
         self.executable = self.node_get_member(node, bool, 'executable', False)
diff --git a/buildstream/plugins/sources/tar.py b/buildstream/plugins/sources/tar.py
index 195c05958..a830df8d3 100644
--- a/buildstream/plugins/sources/tar.py
+++ b/buildstream/plugins/sources/tar.py
@@ -75,6 +75,7 @@ class TarSource(DownloadableFileSource):
         self.base_dir = self.node_get_member(node, str, 'base-dir', '*') or None
 
         self.node_validate(node, DownloadableFileSource.COMMON_CONFIG_KEYS + ['base-dir'])
+        self.keyorder += ['base-dir']
 
     def preflight(self):
         self.host_lzip = None
diff --git a/buildstream/plugins/sources/zip.py b/buildstream/plugins/sources/zip.py
index f5fac3a48..27e5fd915 100644
--- a/buildstream/plugins/sources/zip.py
+++ b/buildstream/plugins/sources/zip.py
@@ -75,6 +75,7 @@ class ZipSource(DownloadableFileSource):
         self.base_dir = self.node_get_member(node, str, 'base-dir', '*') or None
 
         self.node_validate(node, DownloadableFileSource.COMMON_CONFIG_KEYS + ['base-dir'])
+        self.keyorder += ['base-dir']
 
     def get_unique_key(self):
         return super().get_unique_key() + [self.base_dir]
diff --git a/buildstream/source.py b/buildstream/source.py
index bb54110ca..bc4bae9db 100644
--- a/buildstream/source.py
+++ b/buildstream/source.py
@@ -302,6 +302,8 @@ class Source(Plugin):
         # FIXME: Reconstruct a MetaSource from a Source instead of storing it.
         self.__meta = meta                              # MetaSource stored so we can copy this source later.
 
+        self.keyorder = ['directory'] + self.keyorder
+
         # Collect the composited element configuration and
         # ask the element to configure itself.
         self.__init_defaults(meta)