env: enforce predictable ordering when reading lockfile

Without some enforcement of spec ordering, python 2 produced
different results in the affected test than did python 3.  This
change makes the arbitrary but reproducible decision to sort
the specs by their lockfile key alphabetically.

2 files changed, 19 insertions, 9 deletions

diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 35e2956fdd..18a39dc1ad 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -1817,9 +1817,11 @@ class Environment(object):
         roots = d['roots']
         self.concretized_user_specs = [Spec(r['spec']) for r in roots]
         self.concretized_order = [r['hash'] for r in roots]
-        json_specs_by_hash = d['concrete_specs']
+        json_specs_by_hash = collections.OrderedDict()
+        for lockfile_key in sorted(d['concrete_specs']):
+            json_specs_by_hash[lockfile_key] = d['concrete_specs'][lockfile_key]
 
-        specs_by_hash = {}
+        specs_by_hash = collections.OrderedDict()
 
         for lockfile_key, node_dict in json_specs_by_hash.items():
             specs_by_hash[lockfile_key] = Spec.from_node_dict(node_dict)
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index a0608fecea..48ef699f33 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -2868,16 +2868,24 @@ def test_environment_query_spec_by_hash(mock_stage, mock_fetch, install_mockery)
 def test_read_legacy_lockfile_and_reconcretize(mock_stage, mock_fetch, install_mockery):
     """Make sure that when we read a legacy environment lock file with a hash
     conflict (one from before we switched to full hash), the behavior as to
-    which of the conflicting specs we pick is deterministic.  When we read
-    the lockfile, we process root specs in the order they appear in 'roots',
-    so we expect the dependencies of the last root in that list to be the
-    ones that appear in the environment before we forcefully re-concretize
-    the environment.  After we force reconcretization, we should see all
-    the dependencies again."""
+    which of the conflicting specs we pick is deterministic and reproducible.
+    When we read the lockfile, we (somewhat arbitrarily) process specs in
+    alphabetical order of their lockfile key.  Consequently, when reading an
+    old lockfile where two specs have a dag hash conflict we expect to keep the
+    second one we encounter.  After we force reconcretization, we should both of
+    the specs that originally conflicted present in the environment again."""
     legacy_lockfile_path = os.path.join(
         spack.paths.test_path, 'data', 'legacy_env', 'spack.lock'
     )
 
+    # In the legacy lockfile, we have two conflicting specs that differ only
+    # in a build-only dependency.  The lockfile keys and conflicting specs
+    # are:
+    #     wci7a3a -> dttop ^dtbuild1@0.5
+    #     5zg6wxw -> dttop ^dtbuild1@1.0
+    # So when we initially read the legacy lockfile, we expect to have kept
+    # the version of dttop that depends on dtbuild1@0.5
+
     env('create', 'test', legacy_lockfile_path)
     test = ev.read('test')
 
@@ -2888,7 +2896,7 @@ def test_read_legacy_lockfile_and_reconcretize(mock_stage, mock_fetch, install_m
 
     single_root = next(iter(test.specs_by_hash.values()))
 
-    assert single_root['dtbuild1'].version == Version('1.0')
+    assert single_root['dtbuild1'].version == Version('0.5')
 
     # Now forcefully reconcretize
     with ev.read('test'):