Merge branch 'baserock/richardmaw/S10407/field-order'

Reviewed-by: Lars Wirzenius

13 files changed, 176 insertions, 204 deletions

diff --git a/morphlib/morph2.py b/morphlib/morph2.py
index fd72aa94..cc6ce926 100644
--- a/morphlib/morph2.py
+++ b/morphlib/morph2.py
@@ -238,7 +238,7 @@ class Morphology(object):
         to set default values during construction.
 
         '''
-        output_dict = OrderedDict()
+        output_dict = {}
 
         for key in live_dict.keys():
             if key.startswith('_orig_'):
diff --git a/morphlib/morph2_tests.py b/morphlib/morph2_tests.py
index ba90313f..d02aab1c 100644
--- a/morphlib/morph2_tests.py
+++ b/morphlib/morph2_tests.py
@@ -19,6 +19,8 @@ import json
 import StringIO
 import unittest
 
+import yaml
+
 import morphlib
 from morphlib.morph2 import Morphology
 
@@ -251,19 +253,6 @@ class MorphologyTests(unittest.TestCase):
     ]
 }'''
 
-    def test_writing_preserves_chunk_order(self):
-        text_lines = self.stratum_text.splitlines()
-        text_lines[6] = '            "ref": "new-ref",'
-
-        # Change one of the fields
-        morphology = Morphology(self.stratum_text)
-        morphology['chunks'][0]['ref'] = 'new-ref'
-
-        output = StringIO.StringIO()
-        morphology.update_text(self.stratum_text, output)
-        output_lines = output.getvalue().splitlines()
-        self.assertEqual(text_lines, output_lines)
-
     def test_writing_handles_added_chunks(self):
         text_lines = self.stratum_text.splitlines()
         text_lines = text_lines[0:16] + text_lines[8:17] + text_lines[17:]
@@ -276,8 +265,8 @@ class MorphologyTests(unittest.TestCase):
 
         output = StringIO.StringIO()
         morphology.update_text(self.stratum_text, output)
-        output_lines = output.getvalue().splitlines()
-        self.assertEqual(text_lines, output_lines)
+        d = yaml.load(output.getvalue())
+        self.assertEqual(d['chunks'][2]['name'], 'baz')
 
     def test_writing_handles_deleted_chunks(self):
         text_lines = self.stratum_text.splitlines()
@@ -289,8 +278,8 @@ class MorphologyTests(unittest.TestCase):
 
         output = StringIO.StringIO()
         morphology.update_text(self.stratum_text, output)
-        output_lines = output.getvalue().splitlines()
-        self.assertEqual(text_lines, output_lines)
+        d = yaml.load(output.getvalue())
+        self.assertEqual(len(d['chunks']), 1)
 
     system_text = '''{
     "kind": "system",
diff --git a/morphlib/morphloader.py b/morphlib/morphloader.py
index e4367fa1..cd005fae 100644
--- a/morphlib/morphloader.py
+++ b/morphlib/morphloader.py
@@ -205,6 +205,61 @@ class MultipleValidationErrors(morphlib.Error):
             self.msg += ('\t' + str(error))
 
 
+class OrderedDumper(yaml.SafeDumper):
+    keyorder = (
+        'name',
+        'kind',
+        'description',
+        'arch',
+        'strata',
+        'configuration-extensions',
+        'morph',
+        'repo',
+        'ref',
+        'unpetrify-ref',
+        'build-depends',
+        'build-mode',
+        'artifacts',
+        'max-jobs',
+        'products',
+        'chunks',
+        'build-system',
+        'pre-configure-commands',
+        'configure-commands',
+        'post-configure-commands',
+        'pre-build-commands',
+        'build-commands',
+        'post-build-commands',
+        'pre-install-commands',
+        'install-commands',
+        'post-install-commands',
+        'artifact',
+        'include',
+        'systems',
+        'deploy',
+        'type',
+        'location',
+    )
+
+    @classmethod
+    def _iter_in_global_order(cls, mapping):
+        for key in cls.keyorder:
+            if key in mapping:
+                yield key, mapping[key]
+        for key in sorted(mapping.iterkeys()):
+            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.SafeDumper.__init__(self, *args, **kwargs)
+        self.add_representer(dict, self._represent_dict)
+
+
 class MorphologyLoader(object):
 
     '''Load morphologies from disk, or save them back to disk.'''
@@ -324,7 +379,8 @@ class MorphologyLoader(object):
     def save_to_string(self, morphology):
         '''Return normalised textual form of morphology.'''
 
-        return yaml.safe_dump(morphology.data, default_flow_style=False)
+        return yaml.dump(morphology.data, Dumper=OrderedDumper,
+                         default_flow_style=False)
 
     def save_to_file(self, filename, morphology):
         '''Save a morphology object to a named file.'''
diff --git a/morphlib/morphloader_tests.py b/morphlib/morphloader_tests.py
index b8738804..a050e10b 100644
--- a/morphlib/morphloader_tests.py
+++ b/morphlib/morphloader_tests.py
@@ -505,9 +505,9 @@ build-system: dummy
         # The following verifies that the YAML is written in a normalised
         # fashion.
         self.assertEqual(text, '''\
-build-system: dummy
-kind: chunk
 name: foo
+kind: chunk
+build-system: dummy
 ''')
 
     def test_saves_to_file(self):
@@ -524,9 +524,9 @@ name: foo
         # The following verifies that the YAML is written in a normalised
         # fashion.
         self.assertEqual(text, '''\
-build-system: dummy
-kind: chunk
 name: foo
+kind: chunk
+build-system: dummy
 ''')
 
     def test_validate_does_not_set_defaults(self):
@@ -862,3 +862,35 @@ name: foo
                 self.assertEqual(warning.morphology_name, 'foo')
                 self.assertEqual(warning.stratum_name, 'bar')
                 self.assertEqual(warning.field, obsolete_field)
+
+    def test_unordered_asciibetically_after_ordered(self):
+        # We only get morphologies with arbitrary keys in clusters
+        m = morphlib.morph3.Morphology(
+            name='foo',
+            kind='cluster',
+            systems=[
+                {
+                    'morph': 'system-name',
+                    'repo': 'test:morphs',
+                    'ref': 'master',
+                    'deploy': {
+                        'deployment-foo': {
+                            'type': 'tarball',
+                            'location': '/tmp/path.tar',
+                            'HOSTNAME': 'aasdf',
+                        }
+                    }
+                }
+            ]
+        )
+        s = self.loader.save_to_string(m)
+        # root field order
+        self.assertLess(s.find('name'), s.find('kind'))
+        self.assertLess(s.find('kind'), s.find('systems'))
+        # systems field order
+        self.assertLess(s.find('morph'), s.find('repo'))
+        self.assertLess(s.find('repo'), s.find('ref'))
+        self.assertLess(s.find('ref'), s.find('deploy'))
+        # deployment keys field order
+        self.assertLess(s.find('type'), s.find('location'))
+        self.assertLess(s.find('location'), s.find('HOSTNAME'))
diff --git a/morphlib/yamlparse.py b/morphlib/yamlparse.py
index 99599618..726b4181 100644
--- a/morphlib/yamlparse.py
+++ b/morphlib/yamlparse.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2013  Codethink Limited
+# Copyright (C) 2013-2014  Codethink Limited
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -23,108 +23,14 @@ if morphlib.got_yaml: # pragma: no cover
 
 if morphlib.got_yaml: # pragma: no cover
 
-    class OrderedDictYAMLLoader(yaml.SafeLoader):
-        """A YAML loader that loads mappings into ordered dictionaries.
-
-       When YAML is loaded with this Loader, it loads mappings as ordered
-       dictionaries, so the order the keys were written in is maintained.
-
-       When combined with the OrderedDictYAMLDumper, this allows yaml documents
-       to be written out in a similar format to they were read.
-
-        """
-
-        def __init__(self, *args, **kwargs):
-            yaml.SafeLoader.__init__(self, *args, **kwargs)
-
-            # When YAML encounters a mapping (which YAML identifies with
-            # the given tag), it will use construct_yaml_map to read it as
-            # an OrderedDict.
-            self.add_constructor(u'tag:yaml.org,2002:map',
-                                 type(self).construct_yaml_map)
-
-        def construct_yaml_map(self, node):
-            data = OrderedDict()
-            yield data
-            value = self.construct_mapping(node)
-            data.update(value)
-
-        def construct_mapping(self, node, deep=False):
-            if isinstance(node, yaml.MappingNode):
-                self.flatten_mapping(node)
-            else:
-                raise yaml.constructor.ConstructorError(
-                    None, None,
-                    'expected a mapping node, but found %s' % node.id,
-                    node.start_mark)
-
-            mapping = OrderedDict()
-            for key_node, value_node in node.value:
-                key = self.construct_object(key_node, deep=deep)
-                try:
-                    hash(key)
-                except TypeError, exc:
-                    raise yaml.constructor.ConstructorError(
-                        'while constructing a mapping', node.start_mark,
-                        'found unacceptable key (%s)' % exc,
-                        key_node.start_mark)
-                value = self.construct_object(value_node, deep=deep)
-                mapping[key] = value
-            return mapping
-
-    class OrderedDictYAMLDumper(yaml.SafeDumper):
-        """A YAML dumper that will dump OrderedDicts as mappings.
-
-       When YAML is dumped with this Dumper, it dumps OrderedDicts as
-       mappings, preserving the key order, so the order the keys were
-       written in is maintained.
-
-       When combined with the OrderedDictYAMLDumper, this allows yaml documents
-       to be written out in a similar format to they were read.
-
-        """
-
-        def __init__(self, *args, **kwargs):
-            yaml.SafeDumper.__init__(self, *args, **kwargs)
-
-            # When YAML sees an OrderedDict, use represent_ordered_dict to
-            # dump it
-            self.add_representer(OrderedDict,
-                                 type(self).represent_ordered_dict)
-
-        def represent_ordered_dict(self, odict):
-            return self.represent_ordered_mapping(
-                u'tag:yaml.org,2002:map', odict)
-
-        def represent_ordered_mapping(self, tag, omap):
-            value = []
-            node = yaml.MappingNode(tag, value)
-            if self.alias_key is not None:
-                self.represented_objects[self.alias_key] = node
-            best_style = True
-            for item_key, item_value in omap.iteritems():
-                node_key = self.represent_data(item_key)
-                node_value = self.represent_data(item_value)
-                if not (isinstance(node_key, yaml.ScalarNode) and
-                        not node_key.style):
-                    best_style = False # pragma: no cover
-                if not (isinstance(node_value, yaml.ScalarNode) and
-                        not node_value.style):
-                    best_style = False # pragma: no cover
-                value.append((node_key, node_value))
-            if self.default_flow_style is not None:
-                node.flow_style = self.default_flow_style
-            else:
-                node.flow_style = best_style # pragma: no cover
-            return node
-
     def load(*args, **kwargs):
-        return yaml.load(Loader=OrderedDictYAMLLoader, *args, **kwargs)
+        return yaml.safe_load(*args, **kwargs)
 
     def dump(*args, **kwargs):
         if 'default_flow_style' not in kwargs:
             kwargs['default_flow_style'] = False
-        return yaml.dump(Dumper=OrderedDictYAMLDumper, *args, **kwargs)
+        return yaml.dump(Dumper=morphlib.morphloader.OrderedDumper,
+                         *args, **kwargs)
 
 else: # pragma: no cover
     def load(*args, **kwargs):
diff --git a/morphlib/yamlparse_tests.py b/morphlib/yamlparse_tests.py
index f5c2569b..38815168 100644
--- a/morphlib/yamlparse_tests.py
+++ b/morphlib/yamlparse_tests.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2013  Codethink Limited
+# Copyright (C) 2013-2014  Codethink Limited
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -41,14 +41,6 @@ build-system: manual
         ('build-system', 'manual'),
     ])
 
-    def test_loads_as_ordered_dict(self):
-        m = yamlparse.load(self.example_text)
-        self.assertEqual(type(m), OrderedDict)
-
-    def test_dumps_ordered_dicts(self):
-        self.assertEqual(self.example_text,
-                         yamlparse.dump(self.example_dict))
-
     def test_non_map_raises(self):
         incorrect_type = '''\
 !!map
diff --git a/tests.branching/edit-updates-stratum.stdout b/tests.branching/edit-updates-stratum.stdout
index ae1af132..a127f046 100644
--- a/tests.branching/edit-updates-stratum.stdout
+++ b/tests.branching/edit-updates-stratum.stdout
@@ -1,14 +1,13 @@
 diff --git a/hello-stratum.morph b/hello-stratum.morph
-index 73ed482..475fe0f 100644
+index e012b5f..cc0609a 100644
 --- a/hello-stratum.morph
 +++ b/hello-stratum.morph
-@@ -2,7 +2,8 @@ chunks:
- - build-depends: []
-   build-mode: test
-   name: hello
+@@ -3,6 +3,7 @@ kind: stratum
+ chunks:
+ - name: hello
+   repo: test:hello
 -  ref: master
 +  ref: newbranch
-   repo: test:hello
 +  unpetrify-ref: master
- kind: stratum
- name: hello-stratum
+   build-depends: []
+   build-mode: test
diff --git a/tests.branching/petrify-no-double-petrify.stdout b/tests.branching/petrify-no-double-petrify.stdout
index 2218f632..50da61ba 100644
--- a/tests.branching/petrify-no-double-petrify.stdout
+++ b/tests.branching/petrify-no-double-petrify.stdout
@@ -1,9 +1,9 @@
+name: hello-stratum
+kind: stratum
 chunks:
-- build-depends: []
-  build-mode: test
-  name: hello
-  ref: 6c7ddb7a9c0c5bf4ee02a8de030f0892a399c6bb
+- name: hello
   repo: test:hello
+  ref: 293fa0b08f0382c63181c36b6efa602876aa8c87
   unpetrify-ref: master
-kind: stratum
-name: hello-stratum
+  build-depends: []
+  build-mode: test
diff --git a/tests.branching/petrify.stdout b/tests.branching/petrify.stdout
index 041b21d9..f9f35342 100644
--- a/tests.branching/petrify.stdout
+++ b/tests.branching/petrify.stdout
@@ -1,31 +1,31 @@
 Petrified:
+name: hello-stratum
+kind: stratum
 chunks:
-- build-depends: []
-  build-mode: test
-  name: hello
-  ref: 6c7ddb7a9c0c5bf4ee02a8de030f0892a399c6bb
+- name: hello
   repo: test:hello
+  ref: 293fa0b08f0382c63181c36b6efa602876aa8c87
   unpetrify-ref: master
-- build-depends: []
+  build-depends: []
   build-mode: test
-  name: goodbye
-  ref: 717c4a523fb5a94ca2f0a61e665fbc2da6a1f6ac
+- name: goodbye
   repo: test:goodbye
+  ref: 717c4a523fb5a94ca2f0a61e665fbc2da6a1f6ac
   unpetrify-ref: test/petrify
-kind: stratum
-name: hello-stratum
+  build-depends: []
+  build-mode: test
 
 Unpetrified:
+name: hello-stratum
+kind: stratum
 chunks:
-- build-depends: []
-  build-mode: test
-  name: hello
-  ref: master
+- name: hello
   repo: test:hello
-- build-depends: []
+  ref: master
+  build-depends: []
   build-mode: test
-  name: goodbye
-  ref: test/petrify
+- name: goodbye
   repo: test:goodbye
-kind: stratum
-name: hello-stratum
+  ref: test/petrify
+  build-depends: []
+  build-mode: test
diff --git a/tests.branching/setup b/tests.branching/setup
index 9dfe27df..06a05e90 100755
--- a/tests.branching/setup
+++ b/tests.branching/setup
@@ -48,22 +48,22 @@ mkdir "$DATADIR/morphs"
 ln -s "$DATADIR/morphs" "$DATADIR/morphs.git"
 
 cat <<EOF > "$DATADIR/morphs/hello-system.morph"
-arch: $(uname -m)
-kind: system
 name: hello-system
+kind: system
+arch: $(uname -m)
 strata:
 - morph: hello-stratum
 EOF
 
 cat <<EOF > "$DATADIR/morphs/hello-stratum.morph"
+name: hello-stratum
+kind: stratum
 chunks:
-- build-depends: []
-  build-mode: test
-  name: hello
-  ref: master
+- name: hello
   repo: test:hello
-kind: stratum
-name: hello-stratum
+  ref: master
+  build-depends: []
+  build-mode: test
 EOF
 
 scripts/run-git-in "$DATADIR/morphs" init
@@ -83,9 +83,9 @@ scripts/run-git-in "$DATADIR/morphs" checkout master
 mkdir "$DATADIR/hello"
 
 cat <<EOF > "$DATADIR/hello/hello.morph"
-build-system: dummy
-kind: chunk
 name: hello
+kind: chunk
+build-system: dummy
 EOF
 
 scripts/run-git-in "$DATADIR/hello" init
diff --git a/tests.branching/setup-second-chunk b/tests.branching/setup-second-chunk
index 32de7d55..058fb222 100755
--- a/tests.branching/setup-second-chunk
+++ b/tests.branching/setup-second-chunk
@@ -1,5 +1,5 @@
 #!/bin/sh
-# Copyright (C) 2012-2013  Codethink Limited
+# Copyright (C) 2012-2014  Codethink Limited
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -45,16 +45,16 @@ cat <<EOF > hello-stratum.morph
 name: hello-stratum
 kind: stratum
 chunks:
-- build-depends: []
-  build-mode: test
-  name: hello
-  ref: master
+- name: hello
   repo: test:hello
-- build-depends: []
-  build-mode: test
-  name: goodbye
   ref: master
+  build-depends: []
+  build-mode: test
+- name: goodbye
   repo: test:goodbye
+  ref: master
+  build-depends: []
+  build-mode: test
 EOF
 
 git commit -q --all -m "Add goodbye to hello-stratum"
diff --git a/tests.branching/tag-creates-commit-and-tag.stdout b/tests.branching/tag-creates-commit-and-tag.stdout
index e1e2126a..335e6a31 100644
--- a/tests.branching/tag-creates-commit-and-tag.stdout
+++ b/tests.branching/tag-creates-commit-and-tag.stdout
@@ -5,29 +5,28 @@ Date:   Tue Jul 31 16:51:54 2012 +0000
 
 Message
 
-commit 53a2d5f00aead3f01c001da4b2b5677e25970b06
+commit 74b7fcdd21ac4756e473eb8a577caaabeab208b0
 Author: developer <developer@example.com>
 Date:   Tue Jul 31 16:51:54 2012 +0000
 
     Message
 
 diff --git a/hello-stratum.morph b/hello-stratum.morph
-index 73ed482..2218f63 100644
+index e012b5f..50da61b 100644
 --- a/hello-stratum.morph
 +++ b/hello-stratum.morph
-@@ -2,7 +2,8 @@ chunks:
- - build-depends: []
-   build-mode: test
-   name: hello
--  ref: master
-+  ref: 6c7ddb7a9c0c5bf4ee02a8de030f0892a399c6bb
+@@ -3,6 +3,7 @@ kind: stratum
+ chunks:
+ - name: hello
    repo: test:hello
+-  ref: master
++  ref: 293fa0b08f0382c63181c36b6efa602876aa8c87
 +  unpetrify-ref: master
- kind: stratum
- name: hello-stratum
+   build-depends: []
+   build-mode: test
 
 test:morphs
-commit 53a2d5f00aead3f01c001da4b2b5677e25970b06
+commit 74b7fcdd21ac4756e473eb8a577caaabeab208b0
 Author:     developer <developer@example.com>
 AuthorDate: Tue Jul 31 16:51:54 2012 +0000
 Commit:     developer <developer@example.com>
@@ -35,7 +34,7 @@ CommitDate: Tue Jul 31 16:51:54 2012 +0000
 
     Message
 
-commit acb64a107d0ebdbbf0b6850b8272301a66c0a4b7
+commit dc323150575be26e3df0d2dab678560c04281b9f
 Author:     developer <developer@example.com>
 AuthorDate: Tue Jul 31 16:51:54 2012 +0000
 Commit:     developer <developer@example.com>
diff --git a/tests.branching/tag-tag-works-as-expected.stdout b/tests.branching/tag-tag-works-as-expected.stdout
index 9525db0b..c3d723d7 100644
--- a/tests.branching/tag-tag-works-as-expected.stdout
+++ b/tests.branching/tag-tag-works-as-expected.stdout
@@ -9,29 +9,28 @@ Date:   Tue Jul 31 16:51:54 2012 +0000
 
 Second
 
-commit 332fc39a03b63a9586950e0826374dc6fa4ceaf8
+commit ed4fa3a98076e92d61983202ed44455b3689bc16
 Author: developer <developer@example.com>
 Date:   Tue Jul 31 16:51:54 2012 +0000
 
     Second
 
 diff --git a/hello-stratum.morph b/hello-stratum.morph
-index 73ed482..2218f63 100644
+index e012b5f..50da61b 100644
 --- a/hello-stratum.morph
 +++ b/hello-stratum.morph
-@@ -2,7 +2,8 @@ chunks:
- - build-depends: []
-   build-mode: test
-   name: hello
--  ref: master
-+  ref: 6c7ddb7a9c0c5bf4ee02a8de030f0892a399c6bb
+@@ -3,6 +3,7 @@ kind: stratum
+ chunks:
+ - name: hello
    repo: test:hello
+-  ref: master
++  ref: 293fa0b08f0382c63181c36b6efa602876aa8c87
 +  unpetrify-ref: master
- kind: stratum
- name: hello-stratum
+   build-depends: []
+   build-mode: test
 
 test:morphs
-commit 332fc39a03b63a9586950e0826374dc6fa4ceaf8
+commit ed4fa3a98076e92d61983202ed44455b3689bc16
 Author:     developer <developer@example.com>
 AuthorDate: Tue Jul 31 16:51:54 2012 +0000
 Commit:     developer <developer@example.com>
@@ -39,7 +38,7 @@ CommitDate: Tue Jul 31 16:51:54 2012 +0000
 
     Second
 
-commit acb64a107d0ebdbbf0b6850b8272301a66c0a4b7
+commit dc323150575be26e3df0d2dab678560c04281b9f
 Author:     developer <developer@example.com>
 AuthorDate: Tue Jul 31 16:51:54 2012 +0000
 Commit:     developer <developer@example.com>