Fetch shared archive sources without checkums#6627
Conversation
kit-ty-kate
left a comment
There was a problem hiding this comment.
That looks fine at a glance, but i'm not that familiar yet with the code so i'll wait for @rjbou to make her assessment.
We'll need to add new reftests showing the new behaviour at least before we're able to merge this. I'll do that later whenever i have some time (unless you want to do it)
| (match url.OpamUrl.backend, checksums with | ||
| | (`http | `rsync ) , _::_ | ||
| (match url.OpamUrl.backend with | ||
| | (`http | `rsync ) |
There was a problem hiding this comment.
if this is the only thing needed for this to work then we could even just allow it for every backends not just http/rsync.
There was a problem hiding this comment.
Shared fetch current implementation is only working for archives. Git sharing is done via the git cache.
|
The tests issues did not happen on these runs, glad to see that |
|
What is the implication that no checksum is required? Some context in the commit message would help me what changes and why this leads to fewer downloads. |
d5cbe36 to
fd48407
Compare
|
Small typo in the the commit message ("time"). A more descriptive title could be: "Download and cache archives w/o checksum per run" |
4cfaec3 to
2bb5834
Compare
|
It's good for the environment ♻️, too - so would like to see this reviewed and merged. @rjbou |
|
@kit-ty-kate do you happen to know the maintainers and could help us to get this reviewed? If there is a reason to not merge this we can address it. |
|
@lindig I'm waiting on @rjbou who's currently on vacation (it is August after all). I don't personally see a reason to speedup the merge of this since the opam 2.5 freeze is planned for October anyway so we have plenty of time to review this. Is there a specific reason why you want to have this merged in master before the first 2.5 alpha release? |
rjbou
left a comment
There was a problem hiding this comment.
Thanks for the PR, lgtm!
I've added some test to follow this.
What is the implication that no checksum is required? Some context in the commit message would help me what changes and why this leads to fewer downloads.
I think the original idea was to have the checksums to check against in the first draft, but in the end the check of all checksums is done in another part of the code. I don't remember well.
| (match url.OpamUrl.backend, checksums with | ||
| | (`http | `rsync ) , _::_ | ||
| (match url.OpamUrl.backend with | ||
| | (`http | `rsync ) |
There was a problem hiding this comment.
Shared fetch current implementation is only working for archives. Git sharing is done via the git cache.
Without this change, sources that more than one pacakge share, but don't have any specified checksum are not cached and hence downloaded once per package. These can be downloaded only once per run instead of once per package, this change enabled the per-run cache to make this happen. For project like xapi, it means downloading a single tarball for 59 packages that are present in a single repository, every time it needs to be built, saving time. Closes ocaml#5638
2bb5834 to
b08f15b
Compare
kit-ty-kate
left a comment
There was a problem hiding this comment.
lgtm. I'll amend the last commit and merge before tomorrow.
b08f15b to
12dad48
Compare
12dad48 to
9e19aad
Compare
|
Thanks a lot! |
These are not cached between runs, but they can be downloaded only once per run instead of once per package.
Closes #5638
I'm drafting this because I want to run the CI: some tests fails in strange ways and don't know whether this change is the cause or my dev environment