fix: support package pinning via http+tar #11446

maiste · 2025-02-05T14:16:42Z

This PR reduces the gap between opam and dune package management functionnalities. It allows users to ping a tar file (gz or bz) using http (and https) protocol.

Closes #10121

Leonidas-from-XIV

Thanks. The ZIP file changes can be added in a separate PR if you want, but I think that it would be useful to support it given it is a rather common format, especially on Windows.

bin/pkg/pkg_common.ml

src/dune_pkg/mount.ml

src/dune_pkg/opamUrl0.ml

src/dune_pkg/source.ml

test/blackbox-tests/test-cases/pkg/pin-depends.t

src/dune_pkg/opamUrl0.mli

test/blackbox-tests/test-cases/pkg/pin-depends.t

Leonidas-from-XIV · 2025-02-07T11:18:50Z

@maiste I've looked into the describe.t failure and when I run it locally the issue does not happen, so it seems like that might be another CI failure.

rgrinberg · 2025-02-08T23:39:04Z

src/dune_pkg/mount.ml

@@ -23,6 +23,29 @@ let of_opam_url loc url =
      >>| User_error.ok_exn
    in
    Git rev
+  | `Tar ->
+    let dir = Path.Build.(L.relative root [ "_fetch"; "url" ]) |> Path.build in


Why do we put this into _fetch? Is this to attempt to make the build system to use this?

Yes, exactly. Is there a better to do it?

I don't think there's a way to do this at the moment. Whatever you write here will be removed by dune anyway. Might as well use a temp dir.

I try to respect the structure for Dune so it doesn't have to redo the work twice. Isn't it supposed to consider it?

I don't think that will work. Dune writes a metadata file describing its state of the workspace local cache. If you didn't update this file, it will just delete what you have put in there.

rgrinberg · 2025-02-08T23:40:55Z

src/dune_pkg/mount.ml

+       raise (User_error.E message)
+     | Ok output ->
+       let target =
+         let file_digest = Digest.file (Path.to_string output) |> Digest.to_hex in


Path.to_string |> Digest.file |> Digest.to_hex

rgrinberg · 2025-02-08T23:43:48Z

src/dune_pkg/tar.ml

@@ -46,3 +46,14 @@ let extract ~archive ~target =
    Path.rename target_in_temp target;
    Ok ()
 ;;
+
+let load_or_untar ~target ~archive =
+  match Path.exists target with


What if the tar file is updated remotely?

I still don't understand the point of this check

rgrinberg

Do we plan to support tarballs on the file system?

maiste · 2025-02-10T17:22:48Z

Do we plan to support tarballs on the file system?

No, we expect people to untar directory themselves on the file system and point to the path. I don't think it is worth adding the machinery for this as there is already one for file://.

What if the tar file is updated remotely?

If the file is updated remotely:

you will have to re-run dune pkg lock if you clean the _build or,
it will keep using the version in the _build without updating it.

I don't think we have a proper when to address it. If we checked on every run, people would have to download the tar file every time they run dune build. In the current configuration, people will have a checksum error if they do dune clean && dune build which is the equivalent of the behavior (good or bad, idk) with opam: you have to run opam reinstall to get the changes.

I would expect people to want to be aware when the checksum change, but having to explicitly state when they want to synchronize the checksum. It would prevent them from downloading tar they don't expect to change.

rgrinberg · 2025-02-10T18:54:34Z

I think we should re-download the tarball and construct the checksum every time we run dune pkg lock. This is consistent with how we treat all other sources that lack a checksum. Anything else will be bound to create confusion that will have to be solved by reminding everyone to do the equivalent of opam update.

You could still avoid re-downloading if you use curl with the correct e-tag incantations.

maiste · 2025-02-11T08:48:32Z

I agree with you, and this is what happens in the current situation. If you run dune pkg lock, it will download the tar and update the checksum. What I meant is that it is not going to change if you run dune build and the file changed remotely.

Regarding the etag, @Leonidas-from-XIV suggested this. However, many of the packages on opam-repository live in GitHub Release and from the requests I tried with curl, it does not seem to provide an ETag header. It is still an optimization we can do later.

rgrinberg · 2025-02-11T10:14:50Z

Okay, thanks. Is there a test that demonstrates that subsequent dune pkg lock re-download it? We should have one.

maiste · 2025-02-11T10:58:38Z

I'm rewriting my test to show the behavior. It should be ready for a next review round by the end of the day 👍

Signed-off-by: Etienne Marais <[email protected]>

Co-authored-by: ArthurW <[email protected]> Signed-off-by: Etienne Marais <[email protected]>

Signed-off-by: Etienne Marais <[email protected]>

Co-authored-by: Marek Kubica <[email protected]> Signed-off-by: Etienne Marais <[email protected]>

Signed-off-by: Etienne Marais <[email protected]>

maiste requested review from Leonidas-from-XIV and rgrinberg February 5, 2025 14:17

maiste added the package management label Feb 5, 2025

Leonidas-from-XIV reviewed Feb 5, 2025

View reviewed changes

test/blackbox-tests/test-cases/pkg/pin-depends.t Outdated Show resolved Hide resolved

maiste requested a review from Leonidas-from-XIV February 5, 2025 15:48

Leonidas-from-XIV reviewed Feb 6, 2025

View reviewed changes

test/blackbox-tests/test-cases/pkg/pin-depends.t Outdated Show resolved Hide resolved

maiste force-pushed the fix/pinning-10121-bis branch from 2182db3 to 5992d48 Compare February 6, 2025 16:07

rgrinberg reviewed Feb 8, 2025

View reviewed changes

rgrinberg requested changes Feb 8, 2025

View reviewed changes

maiste and others added 8 commits February 11, 2025 13:57

test: rework test

4db9317

Signed-off-by: Etienne Marais <[email protected]>

feat: support pinning tar file

21e9dbf

Co-authored-by: ArthurW <[email protected]> Signed-off-by: Etienne Marais <[email protected]>

test: update the result of the test

072ffd2

Signed-off-by: Etienne Marais <[email protected]>

fix: typo apply suggestions from code review

1bde175

Co-authored-by: Marek Kubica <[email protected]> Signed-off-by: Etienne Marais <[email protected]>

fix: apply @Leonidas-from-XIV suggestion

f0ed71f

Signed-off-by: Etienne Marais <[email protected]>

test: make test reproducible

4b0807b

Signed-off-by: Etienne Marais <[email protected]>

test: check for redownload

99329cc

Signed-off-by: Etienne Marais <[email protected]>

fix: apply reviews

6a41320

Signed-off-by: Etienne Marais <[email protected]>

maiste force-pushed the fix/pinning-10121-bis branch from 5992d48 to 6a41320 Compare February 11, 2025 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support package pinning via http+tar #11446

fix: support package pinning via http+tar #11446

maiste commented Feb 5, 2025

Leonidas-from-XIV left a comment

Leonidas-from-XIV commented Feb 7, 2025

rgrinberg Feb 8, 2025

maiste Feb 10, 2025

rgrinberg Feb 10, 2025

maiste Feb 10, 2025

rgrinberg Feb 10, 2025

rgrinberg Feb 8, 2025

rgrinberg Feb 8, 2025

rgrinberg Feb 11, 2025

rgrinberg left a comment

maiste commented Feb 10, 2025 •

edited

Loading

rgrinberg commented Feb 10, 2025

maiste commented Feb 11, 2025

rgrinberg commented Feb 11, 2025

maiste commented Feb 11, 2025 •

edited

Loading

fix: support package pinning via http+tar #11446

Are you sure you want to change the base?

fix: support package pinning via http+tar #11446

Conversation

maiste commented Feb 5, 2025

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Leonidas-from-XIV commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg left a comment

Choose a reason for hiding this comment

maiste commented Feb 10, 2025 • edited Loading

rgrinberg commented Feb 10, 2025

maiste commented Feb 11, 2025

rgrinberg commented Feb 11, 2025

maiste commented Feb 11, 2025 • edited Loading

maiste commented Feb 10, 2025 •

edited

Loading

maiste commented Feb 11, 2025 •

edited

Loading