Fix location in spec.yaml where we look for full_hash (#19132)

scottwittenburg · web-flow · commit 7cfdf61979bd · 2020-10-02T15:37:47.000-06:00
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from #18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
@@ -1407,23 +1407,38 @@ def needs_rebuild(spec, mirror_url, rebuild_on_errors=False):
 
     spec_yaml = syaml.load(yaml_contents)
 
+    yaml_spec = spec_yaml['spec']
+    name = spec.name
+
+    # The "spec" key in the yaml is a list of objects, each with a single
+    # key that is the package name.  While the list usually just contains
+    # a single object, we iterate over the list looking for the object
+    # with the name of this concrete spec as a key, out of an abundance
+    # of caution.
+    cached_pkg_specs = [item[name] for item in yaml_spec if name in item]
+    cached_target = cached_pkg_specs[0] if cached_pkg_specs else None
+
     # If either the full_hash didn't exist in the .spec.yaml file, or it
     # did, but didn't match the one we computed locally, then we should
     # just rebuild.  This can be simplified once the dag_hash and the
     # full_hash become the same thing.
-    if ('full_hash' not in spec_yaml or
-            spec_yaml['full_hash'] != pkg_full_hash):
-        if 'full_hash' in spec_yaml:
+    rebuild = False
+    if not cached_target or 'full_hash' not in cached_target:
+        reason = 'full_hash was missing from remote spec.yaml'
+        rebuild = True
+    else:
+        full_hash = cached_target['full_hash']
+        if full_hash != pkg_full_hash:
             reason = 'hash mismatch, remote = {0}, local = {1}'.format(
-                spec_yaml['full_hash'], pkg_full_hash)
-        else:
-            reason = 'full_hash was missing from remote spec.yaml'
+                full_hash, pkg_full_hash)
+            rebuild = True
+
+    if rebuild:
         tty.msg('Rebuilding {0}, reason: {1}'.format(
             spec.short_spec, reason))
         tty.msg(spec.tree())
-        return True
 
-    return False
+    return rebuild
 
 
 def check_specs_against_mirrors(mirrors, specs, output_file=None,
diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py
@@ -19,6 +19,7 @@
 import spack.cmd.install as install
 import spack.cmd.uninstall as uninstall
 import spack.cmd.mirror as mirror
+from spack.main import SpackCommand
 import spack.mirror
 import spack.util.gpg
 from spack.directory_layout import YamlDirectoryLayout
@@ -31,6 +32,11 @@
 mirror_path_def = None
 mirror_path_rel = None
 
+mirror_cmd = SpackCommand('mirror')
+install_cmd = SpackCommand('install')
+uninstall_cmd = SpackCommand('uninstall')
+buildcache_cmd = SpackCommand('buildcache')
+
 
 @pytest.fixture(scope='function')
 def cache_directory(tmpdir):
@@ -569,3 +575,37 @@ def test_built_spec_cache(tmpdir,
     margs = mparser.parse_args(
         ['rm', '--scope', 'site', 'test-mirror-rel'])
     mirror.mirror(mparser, margs)
+
+
+def test_spec_needs_rebuild(install_mockery_mutable_config, mock_packages,
+                            mock_fetch, monkeypatch, tmpdir):
+    """Make sure needs_rebuild properly comares remote full_hash
+    against locally computed one, avoiding unnecessary rebuilds"""
+
+    # Create a temp mirror directory for buildcache usage
+    mirror_dir = tmpdir.join('mirror_dir')
+    mirror_url = 'file://{0}'.format(mirror_dir.strpath)
+
+    mirror_cmd('add', 'test-mirror', mirror_url)
+
+    s = Spec('libdwarf').concretized()
+
+    # Install a package
+    install_cmd(s.name)
+
+    # Put installed package in the buildcache
+    buildcache_cmd('create', '-u', '-a', '-d', mirror_dir.strpath, s.name)
+
+    rebuild = bindist.needs_rebuild(s, mirror_url, rebuild_on_errors=True)
+
+    assert not rebuild
+
+    # Now monkey patch Spec to change the full hash on the package
+    def fake_full_hash(spec):
+        print('fake_full_hash')
+        return 'tal4c7h4z0gqmixb1eqa92mjoybxn5l6'
+    monkeypatch.setattr(spack.spec.Spec, 'full_hash', fake_full_hash)
+
+    rebuild = bindist.needs_rebuild(s, mirror_url, rebuild_on_errors=True)
+
+    assert rebuild
