pkg: build and make ocamlformat dev-tool available #10647

moyodiallo · 2024-06-13T10:47:47Z

Attempts to implement #10688.

Reopen the PR #10613 to target main branch instead of toolchain one.

To avoid dune capturing a dev-tool like ocamlformat when using dune build @fmt. This PR is about to build this tool independently from dune-project dependencies, to avoid conflict. After a successful build it will be directly available whenever dune build @fmt or dune fmt is invoked.

ocamlformat: build and make it available
~~prioritize dev tools to PATH when dune.lock dir is present in the project.~~
Solve ocamlformat when dune fmt invoke it. (you're right here @Leonidas-from-XIV, it is needed to keep the pkg tests using dune pkg lock intact or facilitate adding new test for this PR)
Fix the fetch of ocamlformat dev tool dependencies ( It turns out that the fetch rules are missing those packages)
add a cram test

The next works that would be done:

The ocamlformat stanza (ocamlformat <version>) could be added in another PR in order to make this one simple.
A PR for odoc tool will the same as this PR.

moyodiallo · 2024-06-18T12:41:54Z

This PR is not supposed to be merged before #10639, in order to test it properly. Or if we managed to add a cram test which validate it, we could go for that.

moyodiallo · 2024-06-27T10:30:19Z

I'm hitting a new issue, it about fetching the dev-tools dependencies, the solution I found is to fetch them in a different directory _fecth/.dev-tools/<dep>, because a regular pkg and dev-tool dep doesn't share the same pkg database so the fetch_rules need to differentiate.

moyodiallo · 2024-07-03T08:22:37Z

I'm hitting a new issue, it about fetching the dev-tools dependencies, the solution I found is to fetch them in a different directory _fecth/.dev-tools/<dep>, because a regular pkg and dev-tool dep doesn't share the same pkg database so the fetch_rules need to differentiate.

Another solution is used and is short than this one.

rgrinberg · 2024-07-03T09:39:54Z

Is there an issue I could read that describes the solution being implemented here in high level terms?

rgrinberg · 2024-07-03T09:42:01Z

src/dune_rules/pkg_rules.ml

  match components with
+  | [ ".ocamlformat"; pkg_name ] ->
+    setup_package_rules ctx ~component:(Dev_tool Ocamlformat) ~dir ~pkg_name
+  | [ ".ocamlformat" ] ->


Why do you need to introduce a special directory just for ocamlformat?

To avoid conflict with dune.lock packages. Like the ocamlformat could have the same dependency with different version, so it is built separately.

If you indeed have two versions of ocamlformat, which one are you going to let the user see then?

The priority is the one described in dune-project.

moyodiallo · 2024-07-03T10:06:31Z

Is there an issue I could read that describes the solution being implemented here in high level terms?

There's no opened issue. This is new feature about ocamlformat. When ocamlformat is not a dependency of a project, dune fmt will capture ocamlformat command from the PATH. So in the case of pkg management we could build ocamlformat and make it available to dune fmt.

rgrinberg · 2024-07-03T10:08:03Z

Okay, please write such an issue. In particular, it should describe how it resolves the conflict situation we discussed above.

moyodiallo · 2024-07-03T11:24:14Z

Okay, please write such an issue. In particular, it should describe how it resolves the conflict situation we discussed above.

Added here #10688.

Leonidas-from-XIV

I like that there is a test now that shows that the feature works! Functionality-wise I have one concern: the lockdir for the tools shouldn't be in the users source directory. Code-wise see my comments in the review.

Leonidas-from-XIV · 2024-07-03T09:12:39Z

src/dune_pkg/dev_tool.ml

+  }
+
+let to_local (t : t) : Local_package.t =
+  let open Local_package in


I think in Dune we usually try to avoid local opens (except for operators) and instead use Local_package.name = t.name.

src/dune_pkg/dev_tool.ml

Leonidas-from-XIV · 2024-07-03T09:20:58Z

test/blackbox-tests/test-cases/pkg/ocamlformat-dev-tool.t

+Make a package for the library:
+  $ mkpkg ocamlformat 0.26.2 <<EOF
+  > build: [
+  >   ["dune" "subst"] {dev}


This can be removed.

Leonidas-from-XIV · 2024-07-03T09:21:34Z

test/blackbox-tests/test-cases/pkg/ocamlformat-dev-tool.t

+  >     jobs
+  >     "@install"
+  >     "@runtest" {with-test}
+  >     "@doc" {with-doc}


-j jobs @runtest @doc can all be removed as they are not relevant for the test.

test/blackbox-tests/test-cases/pkg/ocamlformat-dev-tool.t

Leonidas-from-XIV · 2024-07-03T12:12:21Z

src/dune_rules/pkg_rules.ml

@@ -1688,10 +1722,184 @@ let gen_rules context_name (pkg : Pkg.t) =

 module Gen_rules = Build_config.Gen_rules

-let setup_package_rules context ~dir ~pkg_name : Gen_rules.result Memo.t =
+module Solver = struct
+  let repositories_of_workspace (workspace : Workspace.t) =


This seems to be duplicated from pkg_common

At some point I added a comment that indicate it is duplicated but I erased it because of a revert.

Leonidas-from-XIV · 2024-07-03T12:13:17Z

src/dune_rules/pkg_rules.ml

+      (* All the file IO side effects happen here: *)
+      Dune_pkg.Lock_dir.Write_disk.commit write_disk
+  ;;
+end


This needs to be deduplicated with the code from pkg_common.

The code from pkg_common is for more than one lockdir.

No solving should occur in pkg_rules

Leonidas-from-XIV · 2024-07-03T12:15:56Z

src/dune_rules/pkg_rules.ml

@@ -1726,26 +1934,38 @@ let setup_package_rules context ~dir ~pkg_name : Gen_rules.result Memo.t =
 ;;

 let setup_rules ~components ~dir ctx =
+  let open Rule_root in


This needs to deduplicate the paths. The issue is that .ocamlformat/.pkg are defined once in Rule_root and once here. e.g. to change it, one needs to change both Rule_root and this function, if only one changes things break.

You're right here.

Leonidas-from-XIV · 2024-07-03T12:16:32Z

src/dune_rules/pkg_rules.ml

+  let ocamlformat_artifact_and_deps =
+    Memo.lazy_ (fun () ->
+      let* () =
+        if not (Stdune.Path.exists (Path.source Dev_tool.Ocamlformat.lock_dir))


No need to reference Stdune here.

Leonidas-from-XIV · 2024-07-03T12:18:26Z

test/blackbox-tests/test-cases/pkg/ocamlformat-dev-tool.t

+  >   ]
+  > ]
+  > url {
+  >  src: "http://0.0.0.0:$PORT"


I think that needs to be 127.0.0.1 or maybe better localhost.

moyodiallo · 2024-07-03T13:38:43Z

I like that there is a test now that shows that the feature works! Functionality-wise I have one concern: the lockdir for the tools shouldn't be in the users source directory. Code-wise see my comments in the review.

I tried to hide the lockdir, but it needs more work to be done for this PR. At the moment dune load only lockdir inside source directory that need to be changed to allow loading from build directory.

rgrinberg · 2024-07-03T20:59:21Z

Some initial comments about the implementation:

There's an intentional layer of abstraction between solving for build plans and executing them. We should maintain it. In particular, solving is not a valid build rule because we do not correctly track its dependencies.
We should avoid hard coding things related to ocamlformat as much as possible. For us there's no difference between any of the dev tools apart from the stage where we generate the lock directories and we need to find the constraints. So you should pretend you have a "dev tools" lock directory and a few packages from where we pull binaries. No additional mentions of ocamlformat should be needed.

Other than that, things are looking good 👍

A few test cases we need to cover:

The conflicting dependencies case that we talked about.
Make sure that a dev tool's dependencies do not leak into the user's build environment.
Errors when solving for the dev tools
Errors when build dev tools

gridbugs · 2024-07-04T07:18:52Z

src/dune_rules/pkg_rules.ml

+    | Dev_tool Ocamlformat -> ".ocamlformat"
+  ;;
+
+  let equal t1 t2 = t1 = t2


We should avoid structural equality and instead explicitly compare variants by matching.

gridbugs · 2024-07-04T07:24:56Z

src/dune_rules/lock_dir.ml

@@ -140,6 +140,8 @@ let get (ctx : Context_name.t) : t Memo.t =
  lock_dir
 ;;

+let get_ocamlformat : t Memo.t = Load.load @@ Dev_tool.Ocamlformat.lock_dir


The @@ is unnecessary here.

gridbugs · 2024-07-05T01:55:23Z

src/dune_rules/pkg_rules.ml

+      Console.Status_line.add_overlay (Constant (Pp.text "Solving for Build Plan"))
+    in
+    let local_packages =
+      Package_name.Map.add Package_name.Map.empty local_package.name local_package


use Package_name.Map.singleton instead

moyodiallo · 2024-07-05T09:53:21Z

Some initial comments about the implementation:

There's an intentional layer of abstraction between solving for build plans and executing them. We should maintain it. In particular, solving is not a valid build rule because we do not correctly track its dependencies.

I get it, what do you think about having dune pkg lock --with-dev-tool=ocamlformat or something similar ?
cc: @Leonidas-from-XIV

We should avoid hard coding things related to ocamlformat as much as possible. For us there's no difference between any of the dev tools apart from the stage where we generate the lock directories and we need to find the constraints. So you should pretend you have a "dev tools" lock directory and a few packages from where we pull binaries. No additional mentions of ocamlformat should be needed.

Super, the idea could be having _build/private/.dev_tools/<name>/<package_deps> instead of _build/private/.ocamlformat/<package_deps>, that will avoid hard-coding ocamlformat. I could try to do this.

Other than that, things are looking good 👍

A few test cases we need to cover:

The conflicting dependencies case that we talked about.

Make sure that a dev tool's dependencies do not leak into the user's build environment.

Errors when solving for the dev tools

Errors when build dev tools

Thanks, those are good test cases.

rgrinberg · 2024-07-05T22:40:39Z

I get it, what do you think about having dune pkg lock --with-dev-tool=ocamlformat or something similar

Before you extend the command line, I suggest you try to accomplish what you'd like using workspace file configuration. Usually, command line options are better suited for things that need to be executed often and don't need to be shared. That does not seem to fit the use case here. Workspace files are a little more verbose, but offer more flexibility and most importantly, do not require to remembering long commands. If we ever need to add a shortcut, we can think of extending the command line later.

moyodiallo · 2024-07-06T16:12:48Z

I get it, what do you think about having dune pkg lock --with-dev-tool=ocamlformat or something similar

Before you extend the command line, I suggest you try to accomplish what you'd like using workspace file configuration. Usually, command line options are better suited for things that need to be executed often and don't need to be shared. That does not seem to fit the use case here. Workspace files are a little more verbose, but offer more flexibility and most importantly, do not require to remembering long commands. If we ever need to add a shortcut, we can think of extending the command line later.

Good point, we could skip this since we can take what we need from .ocamlformat and make the solve at the start of dune fmt command.

gridbugs · 2024-09-10T02:38:46Z

test/blackbox-tests/test-cases/pkg/ocamlformat/dev-tool-deps-conflict-project-deps.t

+
+Format foo.ml, 'dune fmt' uses printer.1.0 instead. There is no conflict with different 
+versions of the same dependency.
+  $ DUNE_CONFIG__LOCK_DEV_TOOL=enabled dune fmt


dune fmt --preview so you don't need to revert foo.ml below.

test/blackbox-tests/test-cases/pkg/ocamlformat/dev-tool-deps-conflict-project-deps.t

gridbugs · 2024-09-10T02:44:01Z

test/blackbox-tests/test-cases/pkg/ocamlformat/dev-tool-deps-conflict-project-deps.t

+  Solution for dune.lock:
+  (no dependencies to lock)
+
+The OCamlFormat binary from the dev-tool.


What is this section intended to communicate to readers?

To show that ocamlformat dev-tool is available.

Can you update the comment to make this clear.

test/blackbox-tests/test-cases/pkg/ocamlformat/dev-tool-deps-conflict-project-deps.t

gridbugs · 2024-09-10T02:47:19Z

test/blackbox-tests/test-cases/pkg/ocamlformat/dev-tool-deps-conflict-project-deps.t

+  >  (public_name foo))
+  > EOF
+
+The OCamlFormat binary from the dev-tool.


I don't think it's necessary to test that the ocamlformat binary works correctly after each step.

Always the same, the intention behind is to be sure ocamlformat dev-tool is not cleaned at some point.

Can you update the comment to explain this.

I removed it and preparing to merge some other concern with this.

test/blackbox-tests/test-cases/pkg/ocamlformat/e2e.t

gridbugs · 2024-09-10T02:48:31Z

test/blackbox-tests/test-cases/pkg/ocamlformat/locking-second-time.t

This file needs a description.

test/blackbox-tests/test-cases/pkg/ocamlformat/solving-fails.t

gridbugs · 2024-09-10T02:50:22Z

test/blackbox-tests/test-cases/pkg/ocamlformat/solving-fails.t

+  > version = 0.26.9
+  > EOF
+
+An important cleaning here


A more meaningful comment would explain why it's important to clean here.

Normally this clean is not necessary.

We need to fix this in the PR, because when the dev-tool is disabled, even if dev-tools.locks/ocamlformat exists dune fmt is not supposed wokrs as if dev-tool is enabled.

Makes sense. Can you update the comment to explain this.

We could just extends the check for dev-tool and remove this cleaning. I could do that.

For now let's just prioritize getting this PR merged, so I'd say just update the comment to clarify and make additional changes in a later PR.

You're right.

gridbugs

I've left a bunch of suggestions throughout the tests.

I think the files beginning with "binary-" should be renamed to remove that prefix as it doesn't seem relevant to what the tests are doing.

All the tests need to be renamed to start with "ocamlformat-" or similar. This is because when you run a single test from the command line, you run dune build @test/<filename-without-the-extension>. If there are multiple tests in different directories with the same filename, all those tests will run. For example there is already a test named e2e.t, so now running dune build @test/e2e will run the existing test and the one introduced here. Adding an explicit prefix ocamlformat- to each of these test files will reduce the risk of accidental name collisions in the future.

christinerose · 2024-09-10T14:21:12Z

test/blackbox-tests/test-cases/pkg/ocamlformat/binary-avoid-from-project-deps.t

+  $ cd ..
+  $ tar -czf ocamlformat-0.26.2.tar.gz ocamlformat
+
+Make a fake ocamlformat.0.26.3:


I see these written both like OCamlFormat 0.26.3 and ocamlformat.0.26.3. If the latter is correct, it will need to be in monospace ocamlformat.0.26.3.

Let me know which format is correct, and I'll go through and make them all consistent.

It is "ocamlformat.0.26.3". we are also trying to avoid quoting with ``.

Thanks for the response! To clarify, you want it written in monospace or without? I believe the correct way would be in monospace since it appears in the code, no? Let me know and I'll make all of them consistent.

Hi Christine, The backquotes syntax for code is only for files that are in the markdown format. Our tests are not written in markdown, so the backticks aren't needed.

Ah! Thank you for that explanation. Some of them are in .ml, .mli. and .t, all which have their own way to put things in code block. Shall I do that?

If you don't want the things normally in monospace formatted that way in these (code, libraries, specific files, etc.), how do you want them formatted to indicate what they are? I saw a lot of them were in " " -- but not all of them. Whichever you'd like, please let me know so I can make all of them consistent.

moyodiallo · 2024-09-11T11:08:45Z

@christinerose your last changes was added. I took what is needed.

moyodiallo · 2024-09-11T11:09:51Z

@gridbugs I applied lot of your concerns, maybe all of them.

christinerose · 2024-09-11T11:20:20Z

@christinerose your last changes was added. I took what is needed.

Great! I went through the new changes and made the formatting consistent.

Part of my job is to make formatting consistent across all documentation. The changes I make are based on previously-decided formatting from the maintainers. If the maintainers have changed in the way these are formatted, please let me know so I can update my style guide.

gridbugs

Looks good to me. @rgrinberg is there anything more that this PR needs from your end?

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>

moyodiallo mentioned this pull request Jun 13, 2024

pkg: build and make ocamlformat dev-tool available #10613

Closed

3 tasks

moyodiallo requested a review from Leonidas-from-XIV June 13, 2024 10:50

moyodiallo added the package management label Jun 13, 2024

moyodiallo force-pushed the dev-tool-ocamlformat branch 4 times, most recently from b22f0ee to e71cb15 Compare July 2, 2024 17:34

moyodiallo marked this pull request as ready for review July 2, 2024 17:35

moyodiallo force-pushed the dev-tool-ocamlformat branch from e71cb15 to 5affb57 Compare July 2, 2024 18:59

rgrinberg reviewed Jul 3, 2024

View reviewed changes

moyodiallo mentioned this pull request Jul 3, 2024

pkg: avoid dune fmt capturing ocamlformat from the PATH. #10688

Open

Leonidas-from-XIV requested changes Jul 3, 2024

View reviewed changes

Leonidas-from-XIV requested a review from gridbugs July 3, 2024 13:40

moyodiallo force-pushed the dev-tool-ocamlformat branch from 20b25e7 to 869fb54 Compare July 3, 2024 18:15

gridbugs reviewed Jul 4, 2024

View reviewed changes

gridbugs reviewed Jul 5, 2024

View reviewed changes

moyodiallo force-pushed the dev-tool-ocamlformat branch from 0fff5e2 to d4f5b23 Compare July 8, 2024 19:05