diff options
| -rw-r--r-- | lib/spack/spack/architecture.py | 8 | ||||
| -rw-r--r-- | lib/spack/spack/spec.py | 8 | ||||
| -rw-r--r-- | lib/spack/spack/test/spec_yaml.py | 73 |
3 files changed, 81 insertions, 8 deletions
diff --git a/lib/spack/spack/architecture.py b/lib/spack/spack/architecture.py index 7552795cd2..378fb5d5d9 100644 --- a/lib/spack/spack/architecture.py +++ b/lib/spack/spack/architecture.py @@ -351,10 +351,10 @@ class OperatingSystem(object): return (self.name, self.version) def to_dict(self): - return { - 'name': self.name, - 'version': self.version - } + return syaml_dict([ + ('name', self.name), + ('version', self.version) + ]) @key_ordering diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index c553da796d..f983e5c559 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -1506,10 +1506,10 @@ class Spec(object): d['parameters'] = params if self.external: - d['external'] = { - 'path': self.external_path, - 'module': self.external_module - } + d['external'] = syaml.syaml_dict([ + ('path', self.external_path), + ('module', self.external_module), + ]) if not self._concrete: d['concrete'] = False diff --git a/lib/spack/spack/test/spec_yaml.py b/lib/spack/spack/test/spec_yaml.py index 7fd2a36469..96bed17b78 100644 --- a/lib/spack/spack/test/spec_yaml.py +++ b/lib/spack/spack/test/spec_yaml.py @@ -8,13 +8,20 @@ YAML format preserves DAG information in the spec. """ +import ast +import inspect import os from collections import Iterable, Mapping +import pytest + +import spack.architecture import spack.hash_types as ht +import spack.spec import spack.util.spack_json as sjson import spack.util.spack_yaml as syaml +import spack.version from spack import repo from spack.spec import Spec, save_dependency_spec_yamls @@ -204,6 +211,72 @@ def test_ordered_read_not_required_for_consistent_dag_hash( assert spec.full_hash() == round_trip_reversed_json_spec.full_hash() +@pytest.mark.parametrize("module", [ + spack.spec, + spack.architecture, + spack.version, +]) +def test_hashes_use_no_python_dicts(module): + """Coarse check to make sure we don't use dicts in Spec.to_node_dict(). + + Python dicts are not guaranteed to iterate in a deterministic order + (at least not in all python versions) so we need to use lists and + syaml_dicts. syaml_dicts are ordered and ensure that hashes in Spack + are deterministic. + + This test is intended to handle cases that are not covered by the + consistency checks above, or that would be missed by a dynamic check. + This test traverses the ASTs of functions that are used in our hash + algorithms, finds instances of dictionaries being constructed, and + prints out the line numbers where they occur. + + """ + class FindFunctions(ast.NodeVisitor): + """Find a function definition called to_node_dict.""" + def __init__(self): + self.nodes = [] + + def visit_FunctionDef(self, node): # noqa + if node.name in ("to_node_dict", "to_dict", "to_dict_or_value"): + self.nodes.append(node) + + class FindDicts(ast.NodeVisitor): + """Find source locations of dicts in an AST.""" + def __init__(self, filename): + self.nodes = [] + self.filename = filename + + def add_error(self, node): + self.nodes.append( + "Use syaml_dict instead of dict at %s:%s:%s" + % (self.filename, node.lineno, node.col_offset) + ) + + def visit_Dict(self, node): # noqa + self.add_error(node) + + def visit_Call(self, node): # noqa + name = None + if isinstance(node.func, ast.Name): + name = node.func.id + elif isinstance(node.func, ast.Attribute): + name = node.func.attr + + if name == 'dict': + self.add_error(node) + + find_functions = FindFunctions() + module_ast = ast.parse(inspect.getsource(module)) + find_functions.visit(module_ast) + + find_dicts = FindDicts(module.__file__) + for node in find_functions.nodes: + find_dicts.visit(node) + + # fail with offending lines if we found some dicts. + assert [] == find_dicts.nodes + + def reverse_all_dicts(data): """Descend into data and reverse all the dictionaries""" if isinstance(data, dict): |