Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dune fmt requires ocamlc to be in path but does not guarantee that this is the case #11038

Open
gridbugs opened this issue Oct 22, 2024 · 9 comments

Comments

@gridbugs
Copy link
Collaborator

Running dune fmt with the dev tools feature enabled (e.g. in the developer preview) dune will install ocamlformat before using it to format the project. However, dune also runs ocamlc to get information about the current system. If ocamlc isn't already in PATH when running dune fmt, and no lockdir is present, the command will fail with:

Error: Program ocamlc not found in the tree or in PATH
 (context: default)
-> required by loading the OCaml compiler for context "default"

Instead, this command should make sure that an ocaml compiler is installed via dune toolchains and add it to the environment in the appropriate place.

Note that this only happens when no lockdir is present because the presence of a lockdir changes how dune finds the ocaml compiler. As a workaround we can require that a lockdir exists when running dune fmt.

@gridbugs
Copy link
Collaborator Author

It appears that dune always picks an ocaml toolchain when it starts, and part of that involves running ocamlc -config. If a lockdir is present then dune will use the ocaml toolchain implied by the lockdir and if there is no lockdir it will look for ocamlc in PATH. It's not obvious what dune should do when there is no lockdir present and you're running dune fmt with LOCK_DEV_TOOLS enabled.

One option would be use the ocaml toolchain implied by the ocamlformat lockdir. The problem with this is we'd like it to work when running dune fmt and dune build @fmt, and in the latter case that would mean choosing an ocaml toolchain based on the build target which feels messy.

Another option is to make dune invoke ocamlc -config just-in-time the first time that information is needed. This would involve threading Memo or Fiber throughout all the code that currently accesses the fields of Ocaml_toolchain.t which would be a lot of work.

The third option is to make it so that running dune fmt with LOCK_DEV_TOOLS requires a lockdir to be present, and to use the ocaml toolchain implied by the lockdir. This is the easiest to implement but would of course mean that running dune fmt without locking would print an error instructing the user to lock their project. We already do this with ocamllsp and odoc.

gridbugs added a commit to gridbugs/dune that referenced this issue Oct 29, 2024
This is a workaround for an issue where dune needs to locate an ocaml
toolchain in order to run `dune fmt`, and in the absence of a lockdir
dune will look in PATH for ocaml tools, which may not be present. The
consequence of this issue is that users of the developer preview can
have an ocaml toolchain installed by dune (in their
~/.cache/dune/toolchains directory), and running `dune fmt` will
complain about `ocamlc` not being found. Forcing the lockdir to exist
means that dune will always take the toolchain implied by the lockdir,
and there won't be a situation where the command fails due to `ocamlc`
being missing.

More info in this issue:
ocaml#11038

Signed-off-by: Stephen Sherratt <[email protected]>
@Leonidas-from-XIV
Copy link
Collaborator

What does Dune do with ocamlc -config?

From reading your investigation it seems that it is calling ocamlc too early and the solution is not to force an ocamlc dependency on ocamlformat but indeed threading the Memo or Fiber through Ocaml_toolchain.t because while #11049 is a workaround it's exactly that: it works around the wrong behavior of Dune and adds technical debt. I'm worried that down the road this will require more workarounds for all dev tools, at which point we might as well require ocamlc for all of them, which makes the dev tool experience just worse. However, I don't know how much work that would be, but it does sound like a significant amount :-(

@rgrinberg what do you think is the right way to do here?

@gridbugs
Copy link
Collaborator Author

I'll start working on deferring running ocamlc -config until it's needed to get a feel for how much work it will be.

@rgrinberg
Copy link
Member

I think I deferred running ocamlc -config as much as possible already. Maybe the formatting rules are forcing it unnecessarily?

@gridbugs
Copy link
Collaborator Author

gridbugs commented Oct 31, 2024

It looks like it's forced while setting up the rules of the project (which if I'm not mistaken happens on every dune build command regardless of the target). For example Scope.DB.lib_entries_of_package forces it, which called by Install_rules.gen_project_rules which is called by Gen_rules.gen_project_rules. It's a little tricky to trace control flow with memo (there are no stack traces so I need to printf-debug). The Dune_rules.Lib_config.t type is needed in a lot of places and this is parsed from the output of ocamlc -config. We could defer things further by representing Lib_config.t with a deferred value and wrapping each field with a getter that returns a _ Memo.t but that sounds like a lot of work and would complicate the code a bunch.

@gridbugs
Copy link
Collaborator Author

Here's another idea. Lib_config.t is defined like this:

type t =
  { has_native : bool
  ; ext_lib : Filename.Extension.t
  ; ext_obj : Filename.Extension.t
  ; os_type : Ocaml_config.Os_type.t
  ; architecture : string
  ; system : string
  ; model : string
  ; natdynlink_supported : Dynlink_supported.By_the_os.t
  ; ext_dll : string
  ; stdlib_dir : Path.t
  ; ccomp_type : Ocaml_config.Ccomp_type.t
  ; ocaml_version_string : string
  ; ocaml_version : Ocaml.Version.t
  }

Some of these fields are facts about the system, and other fields are facts about the ocaml compiler. We use ocamlc -config as an easy way to get all these facts, but the facts about the system could be computed by uname, and we already require uname to be available for other package management logic in dune.

For the facts about the ocaml compiler, it's possible that they're not needed at all by dune fmt, and by deferring their computation we could avoid needing a compiler to be available (I'll test this). We might still need them for dune fmt however, as sometimes dune will behave differently depending on the version of ocamlc that it finds. Because of this I'd argue that if an ocaml compiler is needed, we should make sure to use the same version of the compiler as will eventually be used to compile the project. The only way to know which version of the compiler will be used to compile the project is to look in the lockdir, and if there is no lockdir then the user will need to create one before running dune fmt.

@rgrinberg
Copy link
Member

We use ocamlc -config as an easy way to get all these facts, but the facts about the system could be computed by uname,

That would break cross compilation.

and we already require uname to be available for other package management logic in dune.

Because we don't support cross compilation for package management

For the facts about the ocaml compiler, it's possible that they're not needed at all by dune fmt, and by deferring their computation we could avoid needing a compiler to be available (I'll test this)

I hope that ends up being the case. Would be rather weird if the format rules had a direct dependency on the OCaml compiler.

@gridbugs
Copy link
Collaborator Author

gridbugs commented Nov 1, 2024

Ah I hadn't considered cross compilation. @rgrinberg what do you think about my previous comment about Lib_config.t being needed to set up the project rules? Is it the case that building any target would therefore need to invoke ocamlc -config?

@rgrinberg
Copy link
Member

Seems like it should be possible to drop completely from Lib.DB.t

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants