`dune build @fmt` exits with 1 if `ocamformat` is not installed

shonfeder commented 5 months ago

Expected Behavior

If no .ocamlformat file is present and ocamlformat is not installed, then dune build @fmt should run all other formatting rules associated with the @fmt alias (e.g., formatting dune files, or running custom formatting rules) and then exit cleanly.

Actual Behavior

Assuming you have first uninstalled any ocamlformat in your path and you are in a dune project with an .ml file:

$ dune build @fmt
Error: Program ocamlformat not found in the tree or in PATH
 (context: default)
-> required by _build/default/api/.formatted/client.ml
-> required by alias api/.formatted/fmt
-> required by alias api/fmt

Reproduction

https://github.com/ocaml/dune/pull/10579

Specifications

Version of dune (output of dune --version): 3.15.0, 3.15.2, 3,15.3, and on 0a3f50aac7db87650e66b7a6ca5b13722b5b6c60 (the current head).
Version of ocaml (output of ocamlc --version): 4.14.1 and 4.08
Operating system (distribution and version): 6.6.30-2-MANJARO and debian-12-4.08

Additional information

To my mind the main reason why we shouldn't want such a tight coupling between dune build @fmt and ocamlformat is that it unnecessarily limits the @fmt alias, which is still useful via custom rules and formatting of dune files.

But, it is also causing problems for ocaml-ci, as per https://github.com/ocurrent/ocaml-ci/issues/936

$ ./dune.exe build @fmt --verbose
Shared cache: enabled
Shared cache location: ~/.cache/dune/db
Workspace root: ~/Sync/oss/dune
Auto-detected concurrency: 8
Dune context:
 { name = "default"
 ; kind = "default"
 ; profile = Dev
 ; merlin = true
 ; fdo_target_exe = None
 ; build_dir = In_build_dir "default"
 ; instrument_with = []
 }
Actual targets:        
- recursive alias @fmt
Error: Program ocamlformat not found in the tree or in PATH
 (context: default)
-> required by _build/default/otherlibs/dyn/.formatted/dyn.ml
-> required by alias otherlibs/dyn/.formatted/fmt
-> required by alias otherlibs/dyn/fmt

With the current behavior, we either have to burden every CI job with an install of ocamlformat, even when its not relevant, or we cannot support any formatting at all via the @fmt alias. If we decide for the former course, then we risk breakage of a future version of ocamlformat starts deciding it will run formatting without requiring an .ocamlformat file at the root.

Related to #3836

emillon commented 4 months ago

If no .ocamlformat file is present and ocamlformat is not installed, then dune build @fmt should run all other formatting rules associated with the @fmt alias (e.g., formatting dune files, or running custom formatting rules) and then exit cleanly.

I'm not sure about that. The formatting rules are set up by default for the ocaml dialect, and these depend on ocamlformat. If that's not wanted for a project, it's possible to disable formatting rules for ocaml files.

shonfeder commented 4 months ago

Here is the current documentation of the @fmt alias:

@fmt
====

This alias is used by formatting rules: when it is built, code formatters will
be executed (using :doc:`promotion </concepts/promotion>`).

``dune fmt`` is a shortcut for ``dune build @fmt --auto-promote``.

It is possible to build on top of this convention. If some actions are manually
attached to the ``fmt`` alias, they will be executed by ``dune fmt``.

Example:

.. code:: dune

   (rule
    (with-stdout-to
     data.json.formatted
     (run jq . %{dep:data.json})))

   (rule
    (alias fmt)
    (action
     (diff data.json data.json.formatted)))

note that ocamlformat is not mentioned at all. IMO, expectation that the @fmt alias does not require ocamlformat in the presence of .ml files is natural given the documentation. If users are meant to expect alias to fail because they have not installed a particularly binary, I think that should be noted very prominently in the documentation. If that is the design decision you go with, then I'd consider correcting/updating the docs to close this issue.

IMO, an explicit opt-in approach (whether indicated by the presence of an .ocamlformat file or some flag in the dune-project) would be more compatible with https://github.com/ocaml/dune/issues/3836 . I would also just find it to be less surprising behavior: currently, I cannot set up a simple, explicit rule to format, e.g., a json file, and have that associated with @fmt without first adding an additional configuration saying I don't want to format something else that I never asked for! :)

Leonidas-from-XIV commented 2 months ago

My perspective is a bit different on this. If I have formatting for OCaml files enabled (which is the default since Dune 3.0 IIRC) and call dune build @fmt I would expect:

Return code 0 if formatting succeeded and my files have been formatted
Return code non-0 if there was an issue formatting and my action has not been done.

As an analogy, if we take dune build and have no ocaml, should dune build return 0 despite it not actually having built anything? I believe we all agree that this behavior would be quite surprising, bordering on malicious compliance territory.

My proposal on the other hand would be to update the documentation and state that @fmt will depend on ocamlformat in the presence of .ml files and refmt in the presence of .re files. In the future, Dune might be able to retrieve and build these tools on its own (#10647) but silently doing the wrong thing seems to me to be a bad move.

rgrinberg commented 2 months ago

Lots of interesting discussion here, but to summarize a few key points:

Aliases already have well defined semantics. Adding ad-hoc rules like:

should run all other formatting rules associated with the @fmt alias (e.g., formatting dune files, or running custom formatting rules) and then exit cleanly.

Isn't an option. A couple of other options to explore:

Adding separate formatting aliases for different dialects
Adding this "success on missing ocamlformat" behavior to dune fmt.

Or maybe using aliases for formatting is not the right module for formatting in the first place?

I would also just find it to be less surprising behavior: currently, I cannot set up a simple, explicit rule to format, e.g., a json file, and have that associated with @fmt without first adding an additional configuration saying I don't want to format something else that I never asked for! :)

I understand the concern, but unfortunately a default is always going to leave a subset of users unsatisfied. Our default was chosen based on what would be the appropriate behavior for the majority of users.

shonfeder commented 2 months ago

My proposal on the other hand would be to update the documentation and state that @fmt will depend on ocamlformat in the presence of .ml files and refmt in the presence of .re files

That makes sense to me.

ocaml / dune