Fix location in spec.yaml where we look for full_hash#19132
Fix location in spec.yaml where we look for full_hash#19132scottwittenburg merged 1 commit intodevelopfrom
Conversation
|
@haampie @eugeneswalker This fixes the If one of you can verify it fixes the issue for you as well, that would be great. |
becker33
left a comment
becker33
left a comment
There was a problem hiding this comment.
Mostly looks good. A couple small requests.
| 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 |
There was a problem hiding this comment.
This list comprehension is doing a lot of heavy lifting. Can you add a comment to explain why it works for future maintainability. At first glance it's not clear why you're using a list at all, although I think I've convinced myself it's necessary.
There was a problem hiding this comment.
I don't think I know why the spec in the buildcache spec.yaml file is a list. I know I wrote the schema for that thing not too long ago, but I just made it match the pre-existing structure I saw in those files already. In the database, spec is instead an object, with what appears to ever only be a single key, the package name.
@gartung Were you involved in coming up with the buildcache spec.yaml file format? Or are you otherwise aware of any reason the spec in there should be a list? As far as I know, it's supposed to correspond to the binary package for a single concrete spec.
At any rate, because it's a list, I took the approach above to get my hands on the spec so I could get at the full_hash. I've only ever seen that list have a single element, so I guess I could also just assume there will ever be only one element and just grab the first one.
@becker33 Let me know if you want me to rework this a bit, otherwise I'll just add a comment summarizing the above.
There was a problem hiding this comment.
The comment looks good. I was fine with the code, it just took long enough to parse that it warranted a comment.
There was a problem hiding this comment.
As far as I recall, the spec is taken from spec.yaml in .spaclk for the patckage
spack/lib/spack/spack/binary_distribution.py
Line 702 in 7cfdf61
There was a problem hiding this comment.
Then the buildcache tarball checksum and whether it uses relative rpaths is added as dictionary entries to the spec
spack/lib/spack/spack/binary_distribution.py
Line 765 in 7cfdf61
I am not sure why it is being interpreted as a list. Maybe my understanding of how yaml files are interpreted was incorrect.
lib/spack/spack/test/bindist.py
Outdated
| uninstall_cmd('-y', s.name) | ||
| mirror_cmd('rm', 'test-mirror') |
There was a problem hiding this comment.
I think the install_mockery and mutable_config fixtures will take care of this. If there aren't existing fixtures that can handle this, we need to add new ones. This code will not be run if the test fails.
There was a problem hiding this comment.
It seems the fixture the test is using now, install_mockery_mutable_config, does in fact take care of this, so I removed those lines.
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.
scottwittenburg
force-pushed
the
bindist-fix-needs-rebuild
branch
from
28a815d to
373938b
Compare
|
@becker33 Mind approving when you get a minute, assuming you're ok with the changes? |
This reverts commit 7cfdf61.
This reverts #18359 and follow-on PRs intended to address issues with #18359 because that PR changes the hash of all specs. A future PR will reintroduce the changes. * Revert "Fix location in spec.yaml where we look for full_hash (#19132)" * Revert "Fix fetch of spec.yaml files from buildcache (#19101)" * Revert "Merge pull request #18359 from scottwittenburg/add-binary-distribution-cache-manager"
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.
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 spack#18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
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 spack#18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
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 spack#18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
3 participants
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.