[packages] Make manifest and dependency digests mandatory in lock files #13487
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
There was a problem hiding this comment.
Some nits, but this looks good to go! Thanks @awelc :D I'm pleased with how neatly this slotted in.
I also remember that there was an explicit opt-out for caching where external resolvers were involved -- is that still the case, and if so, does that need to be removed (in a future PR) or is that now defunct and/or gone?
external-crates/move/tools/move-package/tests/test_sources/resolvers/successful.sh
Outdated
Show resolved
Hide resolved
| ), | ||
| deps_digest: None, | ||
| manifest_digest: "40ECD99EA24DDE388D25DBAB6EFAAF04543D55F0AD108BA6F489B0C71BB618AB", | ||
| deps_digest: "", |
There was a problem hiding this comment.
I'm guessing we get an empty string here because there are no deps -- thoughts on making this the digest of the empty list instead? I'm not wedded to it, but I'm trying to anticipate difficulties in interpreting this as a digest elsewhere (in other tools) if it could also be an empty string.
There was a problem hiding this comment.
Indeed! And I don't have a strong opinion here so made it a digest of an empty list
external-crates/move/tools/move-package/tests/test_lock_file.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/tests/test_dependency_graph.rs
Outdated
Show resolved
Hide resolved
external-crates/move/tools/move-package/src/resolution/dependency_graph.rs
Outdated
Show resolved
Hide resolved
awelc
force-pushed
the
aw/dep-graph-mandatory-digests
branch
from
52ad36c
to
b1b2559
Compare
I have to say I don't remember this... The only type of hashing I remember was dependency hashing. Do you recall other details on this? Also, I am sorry for letting it hang for so long, but I missed the feedback notifications somehow... |
Description
This is a follow-up to #12575 where making manifest and dependency digests mandatory was left for future work (#12575 (comment))
One issue here is that now external resolvers must also output content for both of these fields (though digests can actually be empty).
Test Plan
All existing tests must pass after the mandatory field adjustment.
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
Manifest file digest and dependency digest fields in Move.lock files are are now mandatory.