Conservative lockfile generation #7732

gridbugs · 2023-05-15T14:10:38Z

This is a conservative implementation of generating lockfiles (lock directories) in dune. It adds a new command dune pkg lock that solves the dependencies of packages listed in dune-project and creates a lock directory containing the solution.

This is missing some features which I'll add in later changes. Opening this now to get early feedback.

Currently you must specify a path to a local checkout of the opam-repository. I'll have it default to using the current global opam installation's repo in a later change.
Also currently only the bare minimum package fields are added to lockfiles.
There's also no way to override opam variables other than clearing them completely.
Need to add tests that we can round-trip encoding/decoding lockfiles

Relates to #7705 but doesn't completely resolve it.

christinerose

Minor suggestions to match style guide. Approving so I don't block the merge.

CHANGES.md

test/blackbox-tests/test-cases/pkg/lockfile-generation.t/run.t

src/dune_rules/pkg_rules.mli

src/dune_rules/package.ml

rgrinberg · 2023-05-15T16:28:06Z

bin/pkg.ml

+     searched before falling back to packages defined in a directory in the
+     style of opam-repository. *)
+  module Solver_context = struct
+    module Dir_context = Opam_0install.Dir_context


My idea for organizing this was to have most of the internal logic in dune_pkg. While the executable should mostly know about command line arguments and how to use the dune_pkg library to generate the lock file.

I just realized that this isn't quite possible because it will create circular dependencies between dune_rules and dune_pkg. But in any case, we should try to move as much code as possible to dune_pkg.

I've moved all the non-command-line stuff into src/dune_pkg/opam.ml and src/dune_rules/package.ml

Alizter · 2023-05-16T08:00:20Z

.ocamlformat-ignore

@@ -1,2 +1,3 @@
 boot/libs.ml
 src/dune_rules/assets.ml
+vendor/**


Can you make this a separate change? We already have an ocamlformat ignore in vendor, but it doesn't cover all the projects.

Ah I missed that file. Here's a PR that adds opam #7744

rgrinberg · 2023-05-16T08:52:25Z

CHANGES.md

@@ -159,6 +159,9 @@ Unreleased
 - Bump minimum version of the dune language for the melange syntax extension
  from 3.7 to 3.8 (#7665, @jchavarri)

+- Conservative implementation of lock file generation (#7732 partially fixes


Since we're developing this feature in private, we shouldn't tell the user about this (yet)

rgrinberg · 2023-05-16T08:56:53Z

src/dune_pkg/opam.ml

+  let context = Solver_context.create ~env ~repo ~local_packages in
+  let result =
+    try Solver.solve context (OpamPackage.Name.Map.keys local_packages)
+    with exn ->


Is this reachable at all? It seems weird that this function both raises and returns result.

Solver.solve returns Error when it can't find a solution to the dependencies but it can also raise exceptions, for example if opam is unable to parse an opam file in the package repo. I'll add a comment to this effect to clarify why this is needed.

Seems like a weird choice for an API to me.

In any case, if some exceptions are expected we should list them here and have proper dedicated messages for them. Doing Exn.to_dyn exn |> Dyn.to_string is going to show something rather ugly to users.

Some tests for these exceptions would be quite handy as well.

Ok cool, I'll add a list of exceptions. Would it be reasonable to keep the Exn.to_dyn exn |> Dyn.to_string for exceptions not in the list though, as the alternative is that it will crash dune in this case? It will be hard to be sure we're handling all possible exceptions from opam, and we might update our opam port one day and find that it added a new exception without us noticing.

I think it would be reasonable to ask opam (or the solver) to allow us to get a readable user message from any exception that might be user facing.

and we might update our opam port one day and find that it added a new exception without us noticing

That would mean we have a poor error message for an error state that is reachable by the user. Which is a bug our end and a Code_error would be appropriate.

Cool. For this PR I'll catch all the exceptions from opam that I'm aware of and produce a readable User_error for each, and then have a catch all that produces a Code_error indicating that there's a user reachable error state that we haven't accounted for.

rgrinberg · 2023-05-16T08:59:31Z

src/dune_pkg/lock_dir.mli

 val path : Path.Source.t

 val metadata : Filename.t

 module Metadata : Dune_sexp.Versioned_file.S with type data := unit
+
+val instantiate_on_disk : lock_dir_path:Path.Source.t -> t -> unit


How about write_disk? instantiate sounds like we're doing more than just some serialization.

src/dune_pkg/opam.mli

rgrinberg · 2023-05-16T09:03:28Z

src/dune_pkg/lock_dir.ml

+      ; field_o Fields.build Dune_lang.Action.encode build_command
+      ; field_l Fields.deps Package_name.encode deps
+      ; field_o Fields.source Source.encode source
+      ; field Fields.dev bool dev


Just a stylistic choice, but I would omit fields when they're equal to their default value. All the defaults are controlled by the language version, so this is safe to do.

This was an actual bug. decode decodes this field as a field with no argument (ie. (dev)) whereas encode was writing a field with a name and boolean value ((dev true)). Switching to using the field_b function to match the decoder. I still need to add tests that the encoder and decoder round trip.

Hmm, well I think that (dev true) or (dev false) should still be allowed. I'll take a look

rgrinberg · 2023-05-16T09:04:26Z

src/dune_pkg/opam.ml

+    then
+      User_error.raise
+        [ Pp.textf
+            "\"%s\" doesn't look like a path to an opam repository as it lacks \


Should be %s and Path.to_maybe_string_quoted opam_repo_dir_path to be consistent with our other messages.

(using String.maybe_quoted which works for Filename.ts)

Leonidas-from-XIV · 2023-05-16T09:03:45Z

src/dune_pkg/opam.ml

+    let packages_dir_path = opam_repo_dir_path / "packages" in
+    if
+      not
+        (Sys.file_exists packages_dir_path && Sys.is_directory packages_dir_path)


I think this should check for the repo file too, which as far as I am aware needs to contin at least opam-version: "2.0".

Is it not sufficient to let opam check the opam-version in the opam files in the repo as it's parsing them? Or would this just be to improve UX in that we could catch the problem earlier and print a more meaningful error message than the error opam would produce while parsing files? I left this file out as it's not necessary to solve dependencies but it would be easy to add. Do you happen to know if the opam libraries provide an easy way to parse this file?

I would say we should require the same format for the repo as OPAM to not introduce dialects ("this repo only works with dune"/"this repo only works with OPAM"). If I see correctly, opam-format has OpamFile.Repo which defines accessors to the metadata that can be found in the repo file so I would say as long as it can parse it it's a valid repo.

Leonidas-from-XIV · 2023-05-16T09:04:25Z

src/dune_pkg/opam.ml

+        ];
+    { packages_dir_path }
+
+  (* Return the path to an "opam" file describing a paticular package


Suggested change

(* Return the path to an "opam" file describing a paticular package

(* Return the path to an "opam" file describing a particular package

Leonidas-from-XIV · 2023-05-16T09:06:41Z

src/dune_pkg/opam.ml

+    let env name = Env.find_by_name env ~name in
+    let { Repo.packages_dir_path } = repo in
+    let dir_context =
+      Dir_context.create ~prefer_oldest:true


So we're going with MVS?

Ah good question! MVS would be my preference but this merits further discussion. @rgrinberg thoughts?

It's a good default, but we could make this configurable.

I think it is an interesting choice for the prototype and solves a lot of "lower bounds" failures that we frequently have in OPAM, so I'm in favor of trying and collecting feedback.

Leonidas-from-XIV · 2023-05-16T09:07:52Z

src/dune_pkg/opam.ml

+      (Pp.tag User_message.Style.Success
+         (Pp.text "Selected the following packages:")
+      :: List.map t.opam_packages_to_lock ~f:(fun package ->
+             Pp.textf "%s" (OpamPackage.to_string package)))


Suggested change

Pp.textf "%s" (OpamPackage.to_string package)))

Pp.text (OpamPackage.to_string package)))

Leonidas-from-XIV · 2023-05-16T09:08:49Z

src/dune_pkg/opam.ml

+  let result =
+    try Solver.solve context (OpamPackage.Name.Map.keys local_packages)
+    with exn ->
+      User_error.raise [ Pp.textf "%s" (Exn.to_dyn exn |> Dyn.to_string) ]


Suggested change

User_error.raise [ Pp.textf "%s" (Exn.to_dyn exn |> Dyn.to_string) ]

User_error.raise [ Pp.text (Exn.to_dyn exn |> Dyn.to_string) ]

Leonidas-from-XIV · 2023-05-16T09:09:22Z

src/dune_pkg/opam.ml

+      User_error.raise [ Pp.textf "%s" (Exn.to_dyn exn |> Dyn.to_string) ]
+  in
+  match result with
+  | Error e -> User_error.raise [ Pp.textf "%s" (Solver.diagnostics e) ]


Suggested change

| Error e -> User_error.raise [ Pp.textf "%s" (Solver.diagnostics e) ]

| Error e -> User_error.raise [ Pp.text (Solver.diagnostics e) ]

Leonidas-from-XIV · 2023-05-16T09:11:08Z

src/dune_pkg/opam.mli

+  (** Create a [t] from a path to a local directory containing an opam
+      repository. This should be a directory with a subdirectory named
+      "packages" with one subdirectory for each package name, each with
+      subdirectories for each version. *)


I think stating "this should be a valid opam repository" saves us from defining what an opam repository is and potentially creating our own version of opam-repositories that don't work with OPAM.

Good point. I'll update the comment and help message. This is also a good reason to enforce that a "repo" file is present inside the opam repository even if it doesn't contribute to solving.

Leonidas-from-XIV · 2023-05-16T09:15:16Z

test/blackbox-tests/test-cases/pkg/lockfile-generation.t/run.t

+  bar.0.4.0
+  baz.0.1.0
+  foo.0.0.1
+  lockfile_generation_test.LOCAL


I am a bit confused why the lockfile would contain its own project. Am I missing something?

Ah good point there is no reason for the packages defined locally to be present in the lockfile. I'll remove them.

gridbugs · 2023-05-19T06:47:53Z

Applied feedback from review. I still need to add tests for error cases when a user specifies an invalid opam repo. I'll do this next week.

rgrinberg · 2023-05-22T09:38:43Z

@gridbugs this PR doesn't need to be perfect to be merged. Just let me know when you think it's at a good point and I can merge it.

gridbugs · 2023-05-23T06:50:45Z

@gridbugs this PR doesn't need to be perfect to be merged. Just let me know when you think it's at a good point and I can merge it.

Ok cool, I think let's merge this now to avoid future merge conflicts. I'll add tests that the expected error message is raised for invalid opam repos in a later PR.

rgrinberg · 2023-05-23T08:19:38Z

There seems to be a relevant failure in the windows CI:

./_boot/dune.exe build @install -p dune --profile dune-bootstrap
Fatal error: exception Failure("Windows only. This function isn't implemented.")
Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
Called from OpamStateConfig.default in file "vendor/opam/src/state\\opamStateConfig.ml", line 86, characters 34-73
make: *** [Makefile:[23](https://github.com/ocaml/dune/actions/runs/5054113855/jobs/9068678005?pr=7732#step:8:24)5: bootstrap] Error 2

gridbugs · 2023-05-24T01:30:16Z

There seems to be a relevant failure in the windows CI:

./_boot/dune.exe build @install -p dune --profile dune-bootstrap
Fatal error: exception Failure("Windows only. This function isn't implemented.")
Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
Called from OpamStateConfig.default in file "vendor/opam/src/state\\opamStateConfig.ml", line 86, characters 34-73
make: *** [Makefile:[23](https://github.com/ocaml/dune/actions/runs/5054113855/jobs/9068678005?pr=7732#step:8:24)5: bootstrap] Error 2

oh woops, thanks for pointing that out. Will investigate.

gridbugs · 2023-05-24T13:11:48Z

Heads up that I pushed a change to this branch that hacks the opam_state library to lazily compute the default root directory to see if it fixes the crash on windows. If this works I'll push the change upstream to dune's opam port rather than leaving the change in dune.

rgrinberg · 2023-05-24T13:17:34Z

Heads up that I pushed a change to this branch that hacks the opam_state library to lazily compute the default root directory to see if it fixes the crash on windows. If this works I'll push the change upstream to dune's opam port rather than leaving the change in dune.

That's fine, but can we make sure that ocaml-dune/opam contains the commit that does this? Otherwise anyone that will pull will accidentally override.

rgrinberg · 2023-05-24T13:22:02Z

Just a general comment: we can modify upstream libraries all we want and upstream things at our own pace. There's really no pressure to do so. But we should always make sure that every change has a corresponding commit in ocaml-dune/$repo.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

Adds a command `dune pkg lock` which generates a lock directory. Currently the user must specify a path to a local checkout of opam-repository, there is no way to override opam variables (though it's possible to clear them all), and only a subset of lockfile fields are set. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

gridbugs · 2023-05-24T14:50:23Z

Just a general comment: we can modify upstream libraries all we want and upstream things at our own pace. There's really no pressure to do so. But we should always make sure that every change has a corresponding commit in ocaml-dune/$repo.

Make sense. My change seemed to fix the problem. I've pushed the fix to https://github.com/ocaml-dune/opam/ and updated and re-run the update script for opam and squashed the result into the existing commit in this PR that already updated the opam version.

rgrinberg · 2023-05-26T09:59:33Z

src/dune_pkg/lock_dir.ml

+
+let write_disk ~lock_dir_path t =
+  let lock_dir_path = Path.source lock_dir_path in
+  Path.rm_rf lock_dir_path;


Before deleting, how about we make sure this is a real lock file directory if it exists to avoid unfortunate accidents. For example, we check that it has a dune.lock file.

ooh good idea! Will implement

gridbugs requested a review from christinerose as a code owner May 15, 2023 14:10

gridbugs force-pushed the conservative-lockfile-generation branch from 08a7da7 to fc2ba8e Compare May 15, 2023 14:11

gridbugs requested review from Leonidas-from-XIV and rgrinberg May 15, 2023 14:12

christinerose approved these changes May 15, 2023

View reviewed changes

rgrinberg reviewed May 15, 2023

View reviewed changes

src/dune_rules/pkg_rules.mli Outdated Show resolved Hide resolved

rgrinberg reviewed May 15, 2023

View reviewed changes

src/dune_rules/package.ml Outdated Show resolved Hide resolved

rgrinberg reviewed May 15, 2023

View reviewed changes

gridbugs force-pushed the conservative-lockfile-generation branch 2 times, most recently from 9e8a907 to 32f4971 Compare May 16, 2023 07:43

Alizter reviewed May 16, 2023

View reviewed changes

rgrinberg reviewed May 16, 2023

View reviewed changes

src/dune_pkg/opam.mli Outdated Show resolved Hide resolved

rgrinberg reviewed May 16, 2023

View reviewed changes

Leonidas-from-XIV reviewed May 16, 2023

View reviewed changes

rgrinberg added the package management label May 17, 2023

gridbugs force-pushed the conservative-lockfile-generation branch from 32f4971 to 568ac76 Compare May 19, 2023 06:46

gridbugs force-pushed the conservative-lockfile-generation branch from 568ac76 to 6d86727 Compare May 23, 2023 06:49

Update vendored opam

37cf1b1

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

gridbugs force-pushed the conservative-lockfile-generation branch from fb48ce9 to 2ab6ac7 Compare May 24, 2023 14:48

rgrinberg enabled auto-merge (rebase) May 24, 2023 16:09

rgrinberg disabled auto-merge May 24, 2023 16:09

rgrinberg merged commit 32c33a9 into ocaml:main May 24, 2023
22 checks passed

rgrinberg reviewed May 26, 2023

View reviewed changes

rgrinberg mentioned this pull request May 26, 2023

Lock directory regeneration safety #7807

Closed

gridbugs mentioned this pull request May 31, 2023

Test errors for invalid opam repositories #7830

Merged

gridbugs mentioned this pull request Jun 14, 2023

Generate lockdir from current switch #7863

Merged

gridbugs deleted the conservative-lockfile-generation branch October 11, 2023 02:43

	(* Return the path to an "opam" file describing a paticular package
	(* Return the path to an "opam" file describing a particular package

	Pp.textf "%s" (OpamPackage.to_string package)))
	Pp.text (OpamPackage.to_string package)))

	User_error.raise [ Pp.textf "%s" (Exn.to_dyn exn \|> Dyn.to_string) ]
	User_error.raise [ Pp.text (Exn.to_dyn exn \|> Dyn.to_string) ]

	\| Error e -> User_error.raise [ Pp.textf "%s" (Solver.diagnostics e) ]
	\| Error e -> User_error.raise [ Pp.text (Solver.diagnostics e) ]

Conservative lockfile generation #7732

Conservative lockfile generation #7732

Conversation

gridbugs commented May 15, 2023

christinerose left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gridbugs commented May 19, 2023

rgrinberg commented May 22, 2023

gridbugs commented May 23, 2023

rgrinberg commented May 23, 2023

gridbugs commented May 24, 2023

gridbugs commented May 24, 2023

rgrinberg commented May 24, 2023

rgrinberg commented May 24, 2023

gridbugs commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment