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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eaeed10
reftest: add admin cache test
rjbou Jul 17, 2024
648eaa9
Add every listed checksums of a given archive to the cache when fetch…
kit-ty-kate Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/scripts/cygwin.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ if "%1" equ "x86_64-pc-cygwin" (
:: libicu-devel is needed until an alternative to the uconv call in MungeSymlinks
:: is found
set CYGWIN_PACKAGES=make,patch,curl,diffutils,tar,unzip,git,gcc-g++,libicu-devel%CYGWIN_PACKAGES%
:: xxd is needed for reftests
set CYGWIN_PACKAGES=xxd,%CYGWIN_PACKAGES%

:: wget is needed for download.test OPAMFETCH/OPAMCURL testing
set CYGWIN_PACKAGES=wget,%CYGWIN_PACKAGES%
Expand Down
4 changes: 4 additions & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ users)

## Install
* Fix package name display for no agreement conflicts [#6055 @rjbou - fix #6030]
* Make fetching an archive from cache add missing symlinks [#6068 @kit-ty-kate - fix #6064]

## Remove

Expand Down Expand Up @@ -95,6 +96,7 @@ users)

## Admin
* Change hash cache location from `~/.cache` to `<opamroot>/download-cache/hash-cache` [#6103 @rjbou]
* Make `opam admin cache` add missing symlinks [#6068 @kit-ty-kate - fix #6064]

## Opam installer

Expand Down Expand Up @@ -125,6 +127,7 @@ users)
* Add test for filter operators in opam file [#5642 @rjbou]
* Update init test to make it no repo [#5327 @rjbou]
* Add a test in admin cache for hash cache [#6103 @rjbou]
* Add admin cache test [#6068 @rjbou]

### Engine

Expand All @@ -147,6 +150,7 @@ users)
## opam-client

## opam-repository
* `OpamRepository.fetch_from_cache`: when an archive is found, add a symlink (or copy) for the ones found in opam file but not in cache [#6068 @kit-ty-kate]

## opam-state
* `OpamStateConfig.opamroot_with_provenance`: restore previous behaviour to `OpamStateConfig.opamroot` for compatibility with third party code [#6047 @dra27]
Expand Down
11 changes: 6 additions & 5 deletions src/client/opamAdminCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,13 @@ let package_files_to_cache repo_root cache_dir cache_urls
OpamConsole.warning "[%s] no checksum, not caching"
(OpamConsole.colorise `green label);
Done errors
| best_chks :: _ ->
let cache_file =
OpamRepository.cache_file cache_dir best_chks
| _::_ ->
let cache_files =
List.map (OpamRepository.cache_file cache_dir) checksums
in
let error_opt =
if not recheck && OpamFilename.exists cache_file then Done None
if not recheck && List.for_all OpamFilename.exists cache_files then
Done None
else
OpamRepository.pull_file_to_cache label
~cache_urls ~cache_dir
Expand All @@ -190,7 +191,7 @@ let package_files_to_cache repo_root cache_dir cache_urls
let link =
OpamFilename.Op.(link_dir / OpamPackage.to_string nv // name)
in
OpamFilename.link ~relative:true ~target:cache_file ~link)
OpamFilename.link ~relative:true ~target:(List.hd cache_files) ~link)
link;
errors
in
Expand Down
47 changes: 27 additions & 20 deletions src/repository/opamRepository.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ let cache_file cache_dir checksum =
in
aux cache_dir (OpamHash.to_path checksum)

let link_files ~target f l =
List.iter (fun x ->
OpamFilename.link ~relative:true ~target ~link:(f x))
l
Comment on lines +48 to +50
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


let fetch_from_cache =
let currently_downloading = ref [] in
let rec no_concurrent_dls key f x =
Expand Down Expand Up @@ -93,21 +98,27 @@ let fetch_from_cache =
failwith "Version control not allowed as cache URL"
in
try
let hit_file =
OpamStd.List.find_map (fun ck ->
match
List.fold_left (fun (hit, misses) ck ->
let f = cache_file cache_dir ck in
if OpamFilename.exists f then Some f else None)
checksums
in
if List.for_all
(fun ck -> OpamHash.check_file (OpamFilename.to_string hit_file) ck)
checksums
then Done (Up_to_date (hit_file, OpamUrl.empty))
else mismatch hit_file
if OpamFilename.exists f
then (if hit = None then (Some f, misses) else (hit, misses))
else (hit, f :: misses))
(None, []) checksums
with
| None, _ -> raise Not_found
| Some hit_file, miss_files ->
if List.for_all
(fun ck -> OpamHash.check_file (OpamFilename.to_string hit_file) ck)
checksums
then begin
link_files ~target:hit_file Fun.id miss_files;
Done (Up_to_date (hit_file, OpamUrl.empty))
end else
mismatch hit_file
with Not_found -> match checksums with
| [] -> let m = "cache miss" in Done (Not_available (Some m, m))
| checksum::_ ->
(* Try all cache urls in order, but only the first checksum *)
| checksum::other_checksums ->
let local_file = cache_file cache_dir checksum in
let tmpfile = OpamFilename.add_extension local_file "tmp" in
let rec try_cache_dl = function
Expand All @@ -125,6 +136,7 @@ let fetch_from_cache =
checksums
then
(OpamFilename.move ~src:tmpfile ~dst:local_file;
link_files ~target:local_file (cache_file cache_dir) other_checksums;
Done (Result (local_file, root_cache_url)))
else mismatch tmpfile
in
Expand All @@ -151,14 +163,9 @@ let validate_and_add_to_cache label url cache_dir file checksums =
(let checksums = OpamHash.sort checksums in
match cache_dir, checksums with
| Some dir, best_chks :: others_chks ->
OpamFilename.copy ~src:file ~dst:(cache_file dir best_chks);
List.iter (fun checksum ->
let target = cache_file dir best_chks in
let link = cache_file dir checksum in
try
OpamFilename.link ~relative:true ~target ~link
with Sys_error _ -> ())
others_chks;
let target = cache_file dir best_chks in
OpamFilename.copy ~src:file ~dst:target;
link_files ~target (cache_file dir) others_chks;
| _ -> ());
true

Expand Down
3 changes: 2 additions & 1 deletion src/repository/opamRepository.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ val pull_file:
unit download OpamProcess.job

(** Same as [pull_file], but without a destination file: just ensures the file
is present in the cache. *)
is present in the cache and that every listed checksums are correctly linked
to the archive, otherwise it adds the missing links. *)
val pull_file_to_cache:
string -> cache_dir:dirname -> ?cache_urls:url list ->
OpamHash.t list -> url list -> string download OpamProcess.job
Expand Down
Loading
Loading