Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add every listed checksums of a given archive to the cache when fetching from it #6068

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jul 3, 2024

Fixes #6064

Backported in 2.2 by #6069

tests/reftests/admin.test Outdated Show resolved Hide resolved
@rjbou rjbou force-pushed the admin-cache-all-checksums branch 2 times, most recently from 66b4821 to d152299 Compare July 15, 2024 18:11
@rjbou
Copy link
Collaborator

rjbou commented Jul 15, 2024

I've updated

  • the test (in admin-cache.*.test now) for linux/mac and windows: they show more easily the different behaviour, the links, etc.
    • add an md5 corrupted/collision test too
  • the changelog
  • commit messages to be more accurate with current PR content.

@rjbou rjbou changed the title Make opam admin cache store a symlink to every checksums, not just the best one On fetching an archive, add missing symlinks that are present in opam file Jul 15, 2024
msg="$pkg : $kind :"

if [ -L $arch ]; then
link=`readlink "$arch"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if readlink is portable enough. I need here to retrieve the target of the link, not the fully resolved one

@rjbou rjbou force-pushed the admin-cache-all-checksums branch from d152299 to 77d6516 Compare July 15, 2024 18:17
@kit-ty-kate kit-ty-kate changed the title On fetching an archive, add missing symlinks that are present in opam file Make opam admin cache store a symlink to every checksums, not just the best one Jul 15, 2024
@kit-ty-kate kit-ty-kate changed the title Make opam admin cache store a symlink to every checksums, not just the best one Add every listed checksums of a given archive to the cache when fetching from it Jul 15, 2024
tests/reftests/archive.test Outdated Show resolved Hide resolved
@rjbou rjbou force-pushed the admin-cache-all-checksums branch from 0622d17 to 828846e Compare July 16, 2024 16:01
@rjbou
Copy link
Collaborator

rjbou commented Jul 16, 2024

there is a regression in extrasource

--- a/_build/default/tests/reftests/extrasource.test
+++ b/_build/default/tests/reftests/extrasource.out
@@ -1177,16 +1177,12 @@ The following actions will be performed:
   - install good-sha256-good-md5 1
 
 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
-[ERROR] Conflicting file hashes, or broken or compromised cache!
-          - sha256=hash (MISMATCH)
-          - md5=hash (MISMATCH)
-
--> retrieved good-sha256-good-md5.1  (file://${BASEDIR}/p.patch)
+-> retrieved good-sha256-good-md5.1  (cached)
 -> installed good-sha256-good-md5.1
 Done.
 ### sh check-cache.sh
-MD5: link, to sha256 patch
-SHA256: patch, with matching checksum
+MD5: patch, with matching checksum
+SHA256: patch, with mismatching checksum

@kit-ty-kate
Copy link
Member Author

there is a regression in extrasource

i don't think this is a regression. Only a change of behaviour stemming from the fact that now the chosen file from the cache is the last one in the list instead of the first one. We can keep the old behaviour but it requires more code. I don't think it's broken per-say.

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +48 to +50
List.iter (fun x ->
OpamFilename.link ~relative:true ~target ~link:(f x))
l
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpamSystem.link removes the existing link to create a new one. Do we want that behaviour or check beforehand here if the symlink checksum -> target exists, otherwise create it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should do this separately in OpamSystem.link directly. I've opened #6113 to track this

@kit-ty-kate
Copy link
Member Author

kit-ty-kate commented Jul 24, 2024

both done

@kit-ty-kate kit-ty-kate requested a review from rjbou July 24, 2024 13:30
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@kit-ty-kate kit-ty-kate merged commit f7b3559 into ocaml:master Jul 24, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the admin-cache-all-checksums branch July 24, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opam admin cache doesn't create a symlink for sha256 to sha512
2 participants