binary cache: do not create separate spec.json.sig files#34350
binary cache: do not create separate spec.json.sig files#34350haampie wants to merge 4 commits intospack:developfrom
spec.json.sig files#34350Conversation
spackbot-app
bot
added
binary-packages
core
haampie
force-pushed
the
binary_cache/do-not-create-separate-spec.json.sig-files
branch
from
ce971b4 to
77e0c5c
Compare
haampie
changed the title
spec.json.sig files
haampie
force-pushed
the
binary_cache/do-not-create-separate-spec.json.sig-files
branch
4 times, most recently
from
d4a1e90 to
0174649
Compare
|
Ping @blue42u 🙃 |
haampie
force-pushed
the
binary_cache/do-not-create-separate-spec.json.sig-files
branch
from
0174649 to
896890d
Compare
There's no reason to generate a separate spec.json.sig file, instead we can peek into the spec.json file and see if it is clearsigned json. In th future we can stop using spec.json.sig files in general to avoid 404s. This also changes the iteration error: outer loop ieraters over extension types, inner loop over mirrors, since in time we'd expect *.json to always be correct, whether signed or unsigned.
haampie
force-pushed
the
binary_cache/do-not-create-separate-spec.json.sig-files
branch
from
896890d to
b8eb655
Compare
scottwittenburg
left a comment
scottwittenburg
left a comment
There was a problem hiding this comment.
Thanks @haampie this seems like a good idea 👍 I just have a couple questions, but otherwise like the change.
Co-authored-by: Scott Wittenburg <[email protected]>
|
@scottwittenburg how about this: I'll restore the In a separate PR I'll make Offline in the sense that it should only query the local (possibly outdated) info we have about the remote mirror. As I understand it, that's used as an optimization to improve the order in which mirrors are considered. That's great. What's not great: if we locally have no data about the remote mirror, it's strictly more work to update the local cache than it is to directly fetch in default order. So, obviously this optimization only works given local data, and if there's no local data, the best you can do is fall back to going over the mirror in default order. |
This statement is false when there are multiple buildcaches configured and all of them are small (< a few MB), which is exactly our use case in HPCToolkit's CI. But it is true that the worst-case scenario is worse if you update the local cache compared to using a default order, so not opposed. |
|
This should be ready for review. Only concern is that it's not forward compatible (old spack will fail to deal with clearsigned spec.json file without sig extension), but as far as I know, Spack is never forward compatible. |
|
It occurs to me that if we don't want @kotfic Can you chime in on whether you think this will be problematic? Looking around, I think the notary job script needs changed here so we no longer write back I'll make a PR to fix all the stacks signing jobs where they refer to the extension, e.g. here. Another case where it will be great to have #34272. I think due to the way this is implemented, it could be merged before we fix how the notary job signs things with the reputational key, as spack can still handle |
|
Closing this in favor of #34490 to get better compatibility backward/forward compat |
haampie
deleted the
binary_cache/do-not-create-separate-spec.json.sig-files
branch
3 participants
There's no reason to generate a separate
spec.json.sigfile, instead wecan peek into the
spec.jsonfile and see if it is clearsigned json.This PR also changes the iteration order: outer loop iterates over
extension types, inner loop over mirrors, since in time we'd expect
*.json to always be correct, whether signed or unsigned, so on a
(binary) cache hit, at most 1 request per mirror for the specfile is
necessary.
In the future we can stop using
spec.json.sigfiles in general to avoid404s. (In the near future we could also just add a config option to stop
looking for these
spec.json.sigfiles.)