pkg: build and make ocamlformat dev-tool available

[moyodiallo]

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.

• [X] ocamlformat: build and make it available
• [x] ~~prioritize dev tools to PATH when dune.lock dir is present in the project.~~
• [x] 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)
• [x] Fix the fetch of ocamlformat dev tool dependencies ( It turns out that the fetch rules are missing those packages)
• [x] 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]

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]

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]

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]

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

[moyodiallo]

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]

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

[moyodiallo]

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

Added here #10688.

[moyodiallo]

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]

Some initial comments about the implementation:

1. 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.

2. 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:

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

[moyodiallo]

Some initial comments about the implementation:

1. 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

2. 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:

1. The conflicting dependencies case that we talked about.

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

3. Errors when solving for the dev tools

4. Errors when build dev tools

Thanks, those are good test cases.

[rgrinberg]

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]

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.

[Leonidas-from-XIV]

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

I would avoid cluttering the CLI (and UI in general) with more things. The point about this feature is that it should work as transparent as possible, so not require the user to spend a lot of time learning about this feature, configuring options etc.

I would like to avoid locking the dev-tools before they're needed as this takes additional time and thus makes people pay that might not ever use it (especially if we add support for odoc, LSP, utop later, making the lock file generation longer just because people might use the tool later seems like a bad idea).

[moyodiallo]

Let summarize so far the state of the PR after applying some suggestions:

• There's no mention of ocamlformat in pkg_rules.
• The version of ocamlformat dev-tool is coming from .ocamlformat.
• There's no solving in pkg_rules, now it is done before running the action_builder system.
• More tests
  • make sure there's no conflict
  • make sure there's no leak to the user's build environment.
  • when the solving fails and the build of ocamlformat dev-tool also fails.
• Ocamlformat is chosen as the following order:
  • try to take the one inside dune.lock (coming from dune-project)
  • try to lock one, which the version is coming from .ocamlformat file.
  • when the 2 condition above are not met it locks the latest version.

This PR progressed a lot, thanks to the reviewers, it is ready for another round of review.

[moyodiallo]

Lot of suggestions are addressed and on the last commit https://github.com/ocaml/dune/pull/10647/commits/4a947900294fbdb19d24b267d43f41f3894a6dd1 there's a simplification how the lock of ocamlformat dev-tool is triggered.

what would follow this PR:

• re-locking when version inside .ocamlformat file is modified.
• and making sure that re-locking works with dune watch mode.
• hide dev-tools.locks files inside build directory (lock directories to be loaded from the build directory).
• support for with-dev-setup variable

I think, if we agree on the fundamentals, let's move on and bring the improvements later.

[rgrinberg]

re-locking when version inside .ocamlformat file is modified.

Isn't that pretty bad? So when I change some ocamlformat setting, I will trigger the solver?

and making sure that re-locking works with dune watch mode.

This needs to be removed and properly designed first before it's implemented

hide dev-tools.locks files inside build directory (lock directories to be loaded from the build directory).

Indeed this can follow in another PR

support for with-dev-setup variable

Agreed this can go in another PR as well.

I still find that the pkg_rules has far too much knowledge of this "dev tools" feature. The package rules are rather low level in the sense that all they know how to do is to build packages given a lock directory and assemble an appropriate build environment for downstream rules. Anything beyond is a layering violation.

To make the rules agnostic to "dev tools" simplest would be to allow for more than one lock directory to co-exist in a context. In the library database, every database can have a parent. Perhaps something like that can be done here as well?

[moyodiallo]

re-locking when version inside .ocamlformat file is modified.

Isn't that pretty bad? So when I change some ocamlformat setting, I will trigger the solver?

No, the old version is known or it could be stored in the build directory.

and making sure that re-locking works with dune watch mode.

This needs to be removed and properly designed first before it's implemented

I don't think, maybe after getting the feature, this could be naturally done. I'm not opposed to re-design it properly after.

I still find that the pkg_rules has far too much knowledge of this "dev tools" feature. The package rules are rather low level in the sense that all they know how to do is to build packages given a lock directory and assemble an appropriate build environment for downstream rules. Anything beyond is a layering violation.

To make the rules agnostic to "dev tools" simplest would be to allow for more than one lock directory to co-exist in a context. In the library database, every database can have a parent. Perhaps something like that can be done here as well?

I agreed and it could have been done in the beginning of the PR, at this stage I think we could improve with this in another PR.

[rgrinberg]

Undoing the undesirable changes should not be delayed to another PR. Additional features can be done in a different PR - those are additive and do not disturb anyone. But changes that obfuscate the current design will make it harder for everyone else to work on this meanwhile. So I will insist that this problem needs to be solved in this PR.

[moyodiallo]

Undoing the undesirable changes should not be delayed to another PR. Additional features can be done in a different PR - those are additive and do not disturb anyone. But changes that obfuscate the current design will make it harder for everyone else to work on this meanwhile. So I will insist that this problem needs to be solved in this PR.

I got the point.

[moyodiallo]

To make the rules agnostic to "dev tools" simplest would be to allow for more than one lock directory to co-exist in a context. In the library database, every database can have a parent. Perhaps something like that can be done here as well?

Would like to elaborate further, e.g how to build 2 lock directory independently ? Why a database can have a parent ? (we want the databases to be independent each other).

[rgrinberg]

You could build each environment independently, but you can provide a build environment composed of stacked locked directories. This stacking step should have no mention of "dev tools".

[gridbugs]

Heads up that I just rebased this onto the current tip of main. I'm going to do some work on this PR while @moyodiallo is away.

[rgrinberg]

Sounds good. I think a good start would be to introduce a separate PR with the changes to the build rules that would be required to implement "dev tools". In particular, the bit about creating a build environment for the rules out of more than one lock directory.

[gridbugs]

Can you elaborate a bit on what you mean by "stacked lockdirs". My understanding was that each dev tool would have its own package universe so that multiple non-coinstallable tools could be used by a project. This sounds like each build environment will be associated with a single lock directory, and the selected lock directory will depend on whether we are building a specific dev tool or the project itself.

[rgrinberg]

After you've built a dev tool, you need to make it available to user rules (as in the example with ocamlformat above). Therefore, the user rules need to be able to access artifacts from more than one lock directory at a time. This combined environment should be a first class notion.

[gridbugs]

After you've built a dev tool, you need to make it available to user rules (as in the example with ocamlformat above). Therefore, the user rules need to be able to access artifacts from more than one lock directory at a time. This combined environment should be a first class notion.

Ah gotcha. Yep that makes perfect sense.

[gridbugs]

Actually on second thought I'm not so sure it's necessary to add devtools to the build environments for other packages. When we talk about dev tools we're talking about programs that developers will use to write code, not tools that are used to build other packages. The most commonly discussed devtools are ocamlformat, ocaml-lsp-server and merlin, and I can't think of a case where any of these tools needs to be added to the build environment of packages. If a package needs a program in its build environment, it should be a regular dependency of that package and go in the same lockdir as the package.

For completeness, there's talk of treating odoc as a devtool, and odoc does need to be available for user rules or internal rules. I'm not so sure that odoc should be installed with the same mechanism as the other devtools.

[rgrinberg]

. The most commonly discussed devtools are ocamlformat, ocaml-lsp-server and merlin, and I can't think of a case where any of these tools needs to be added to the build environment of packages

How would you expect dune fmt to work? I suppose you could modify the formatting rules to be able to run against a specific lock directory.

[gridbugs]

How would you expect dune fmt to work?

I'm not super familiar with how it works currently, but I would have thought the only thing that needs to change is how it locates the ocamlformat binary. Assuming that the ocamlformat lockdir has been solved, and the ocamlformat package from that universe has been built, I'm imagining dune fmt can run in an environment with the appropriate PATH entry for the ocamlformat "component" inside _build.

[rgrinberg]

Maybe ocamlformat is also a tough example and you should perhaps focus on making this work for ocamllsp/merlin first since those don't require modifying any rules.

I'm not super familiar with how it works currently, but I would have thought the only thing that needs to change is how it locates the ocamlformat binary. Assuming that the ocamlformat lockdir has been solved, and the ocamlformat package from that universe has been built, I'm imagining dune fmt can run in an environment with the appropriate PATH entry for the ocamlformat "component" inside _build.

Yes, that sounds reasonable. You will need to modify the ocamlformat rules to understand this though.