fix: show clear error when running uninstalled dev tool #12978

benodiwal · 2025-12-16T00:04:19Z

When running dune tools exec on a tool that wasn't installed, users would see confusing errors about lockdirs or missing ocaml packages. This made it unclear what the actual problem was and how to fix it.

This change makes dune tools exec only run tools that are already installed. If the tool is not installed, it now shows a clear error message: "The tool X is not installed" with a hint to run dune tools install X.

This separates the responsibilities between the two commands:

dune tools install handles locking, building, and installing
dune tools exec only runs already-installed tools

Fixes #12975

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

benodiwal · 2025-12-16T15:03:23Z

@shonfeder @Alizter I have made some changes

Alizter · 2025-12-18T13:11:18Z

test/blackbox-tests/test-cases/pkg/merlin/dev-tool-merlin-basic.t

+  Error: The tool ocamlmerlin is not installed.
+  Hint: Try running 'dune tools install ocamlmerlin'


That looks incorrect to me. Shouldn't the fake ocamlmerlin be picked up here?

The test needed DUNE_CONFIG__LOCK_DEV_TOOL=enabled to trigger auto-install behavior and hence fake ocamlmerlin to be picked up. Added in the latest commit

shonfeder

Thanks for this contribution! 🎉

I don't understand why so many tests need changing, and in particular why they require enabling a feature that was not enabled before.

That makes me worry the logic introduced here is breaking current behavior we want to maintain. My understanding of the issue is that only the error messages should be changing as a result of the change (and only in case DUNE_CONFIG__LOCK_DEV_TOOL is not enabled and tools exec is run when a tool is not installed), nothing else about the behavior should change, afaiu.

shonfeder · 2025-12-19T00:09:57Z

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

-  $ dune tools exec ocamllsp
+  $ dune tools install ocamllsp
  Solution for dev-tools.locks/ocaml-lsp-server:
  - ocaml.5.2.0
  - ocaml-lsp-server.0.0.1
+
+  $ dune tools exec ocamllsp


Why is this test being changed?

shonfeder · 2025-12-19T00:12:18Z

test/blackbox-tests/test-cases/pkg/merlin/dev-tool-merlin-basic.t

  > EOF

-  $ dune tools exec ocamlmerlin
+  $ DUNE_CONFIG__LOCK_DEV_TOOL=enabled dune tools exec ocamlmerlin


Why does this need to be added?

benodiwal · 2025-12-19T11:32:12Z

Thanks for this contribution! 🎉

I don't understand why so many tests need changing, and in particular why they require enabling a feature that was not enabled before.

That makes me worry the logic introduced here is breaking current behavior we want to maintain. My understanding of the issue is that only the error messages should be changing as a result of the change (and only in case DUNE_CONFIG__LOCK_DEV_TOOL is not enabled and tools exec is run when a tool is not installed), nothing else about the behavior should change, afaiu.

This behavior change was discussed earlier in this PR. @Leonidas-from-XIV suggested exec should always auto-install, but I asked whether exec should behave differently based on the DUNE_CONFIG__LOCK_DEV_TOOL flag (auto-install when enabled, show error when disabled), and @Alizter agreed with that approach.

The tests need the flag because the default behavior intentionally changed - exec no longer auto-installs by default, it requires the flag. This matches the separation described in the PR: dune tools install handles locking/building/installing, while dune tools exec runs already-installed tools (unless the flag enables auto-install).

Happy to revert to always auto-installing if that's preferred, just want to confirm since both approaches were discussed here.

shonfeder

Thank you for clarifying! I did read the discussion, but didn't fully grasp it because I assumed the flag would actually do what it said, rather than mostly being ignored and only being checked in a few particular cases.

Now that I have reviewed the code, I see we have not had the correct behavior around this feature flag (IMO), and that this PR fixes that, along with improving the error message in the few cases where the flag was being respected.

I do have a suggestion to make the idiom here match the other places we check this config value tho.

shonfeder · 2025-12-19T20:41:37Z

bin/tools/tools_common.ml

    and+ args = Arg.(value & pos_all string [] (info [] ~docv:"ARGS" ~doc:None)) in
    let common, config = Common.init builder in
-    lock_build_and_run_dev_tool ~common ~config builder dev_tool ~args
+    match Config.get Dune_rules.Compile_time.lock_dev_tools with


If we are adding the check here (and I still do not understand why weren't already checking this), I suspect we should be using the interface provided by

dune/bin/lock_dev_tool.mli

Line 3 in 4c70ef1

val is_enabled : bool Lazy.t

Suggested change

match Config.get Dune_rules.Compile_time.lock_dev_tools with

match Lazy.force Lock_dev_tool.is_enabled with

this follows prior usage at

dune/bin/fmt.ml

Lines 16 to 26 in 4c70ef1

let lock_ocamlformat () =

if Lazy.force Lock_dev_tool.is_enabled

then

(* Note that generating the ocamlformat lockdir here means

that it will be created when a user runs `dune fmt` but not

when a user runs `dune build @fmt`. It's important that

this logic remain outside of `dune build`, as `dune

build` is intended to only build targets, and generating

a lockdir is not building a target. *)

Lock_dev_tool.lock_dev_tool Ocamlformat |> Memo.run

else Fiber.return ()

and

dune/bin/ocaml/utop.ml

Lines 16 to 19 in 4c70ef1

let lock_utop_if_dev_tool_enabled () =

match Lazy.force Lock_dev_tool.is_enabled with

| false -> Memo.return ()

| true -> Lock_dev_tool.lock_dev_tool Utop

@shonfeder made the change

Leonidas-from-XIV

I am honestly confused about the semantics of exec/install and LOCK_DEV_TOOL in this PR.

The way I was explained it the semantics should be:

If LOCK_DEV_TOOL is set, then commands that use the dev tool should automatically lock and use the dev-tool (so dune fmt etc). If it is not set, the commands attempt to use an installed dev-tool and fail if there is none in the PATH.
Unless it is dune tools then it always locks and installs the dev tool.

This also implies that dune tools exec <foo> would install <foo> which is a nice property, because then the user does not need to track whether <foo> is installed nor run another command before to make sure it is installed. And also mirrors how dune exec knows how to build the executable and will build it instead of erroring out.

Leonidas-from-XIV · 2025-12-22T10:12:47Z

bin/tools/tools_common.ml

+    match Lazy.force Lock_dev_tool.is_enabled with
+    | true -> lock_build_and_run_dev_tool ~common ~config builder dev_tool ~args
+    | false ->
+      if Path.exists exe_path


Will this not prevent any new installation of a dev tool forever (or unless the user deletes the file in _build)?

Leonidas-from-XIV · 2025-12-22T10:14:14Z

test/blackbox-tests/test-cases/pkg/merlin/dev-tool-merlin-relock-on-ocaml-version-change.t

       Running 'ocamlmerlin'
  hello from fake ocamlmerlin
-  $ grep "version" "${dev_tool_lock_dir}"/ocaml-base-compiler.pkg
+  $ cat "${dev_tool_lock_dir}"/ocaml-base-compiler.pkg


Why this change? I think it is nice not to spam the tests with output that is irrelevant (and makes them more fragile, since every change will now break this test).

benodiwal · 2025-12-22T12:38:26Z

Thanks for the review @Leonidas-from-XIV!
I might be misunderstanding something, so just wanted to double-check. Earlier, @Alizter and I discussed exec behaving differently based on the flag (auto-install when enabled, and erroring when disabled), but from your feedback it sounds like exec should always auto-install, regardless of the flag.
Could you help clarify the intended behavior here?? Happy to update either way.
cc @shonfeder

When running `dune tools exec <tool>`: - If DUNE_CONFIG__LOCK_DEV_TOOL=enabled: auto-install and run the tool - If the flag is disabled (default): show clear error if tool not installed This addresses the feedback that `exec` should auto-install when the config flag is enabled, matching the behavior of other commands like `dune tools install`. Co-Authored-By: Sachin <sachinbeniwal0101@gmail.com> Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

The DUNE_CONFIG__LOCK_DEV_TOOL flag only affects `dune tools exec` behavior. Remove it from `dune tools install` and `dune tools env` commands where it has no effect. Co-Authored-By: Sachin <sachinbeniwal0101@gmail.com> Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

Test that: - Without the flag (default): exec fails if tool not installed - With flag enabled: exec auto-installs and runs - Once installed, exec works without the flag Co-Authored-By: Sachin <sachinbeniwal0101@gmail.com> Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

Leonidas-from-XIV · 2025-12-22T13:48:58Z

I did a bit of digging and my original understanding was that LOCK_DEV_TOOL means that the lock rules functionality is only enabled when the flag is set, which is what I implemented in #12489 by setting the flag on every command invocation that would generate dev tool lock rules.

However in #12575 this was undone, so LOCK_DEV_TOOL does not enable or disable lock rules when run in the context of dune tools.

That said, at this point I am not sure what the actual semantics of this flag should be. This seems to be the core of the back and forth in this PR.

But given the evidence that even within the Dune developers we don't have a consensus of what this flag should or should not enable, I'd rather suggest determining what the semantics of dev tool locking should be and removing the flag. If we can't explain what the flag does, how are users supposed to understand it?

A removal of this flag ties would tie in with our focus on removing the feature flags. We removed most of them by either upstreaming the feature or removing it altogether and I would argue that this would also make sense with LOCK_DEV_TOOL.

benodiwal force-pushed the improve-tools-exec-error-message branch from 9bd82ac to 47543b2 Compare December 16, 2025 00:04

benodiwal mentioned this pull request Dec 16, 2025

feat: Add merlin support to dune tools #12972

Merged

Alizter reviewed Dec 16, 2025

View reviewed changes

test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-wrapper.t Show resolved Hide resolved

benodiwal force-pushed the improve-tools-exec-error-message branch 3 times, most recently from 0ae9bfc to 0650ed3 Compare December 16, 2025 14:44

benodiwal force-pushed the improve-tools-exec-error-message branch 6 times, most recently from 047559d to e4ed7a9 Compare December 18, 2025 13:03

Alizter reviewed Dec 18, 2025

View reviewed changes

shonfeder reviewed Dec 19, 2025

View reviewed changes

benodiwal force-pushed the improve-tools-exec-error-message branch 3 times, most recently from 151f9aa to 1cbe18e Compare December 20, 2025 12:54

Leonidas-from-XIV reviewed Dec 22, 2025

View reviewed changes

benodiwal and others added 8 commits December 22, 2025 18:08

fix: merlin tests

ccf342f

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: use explicit install in merlin env test

78fd258

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: enable auto-install flag in merlin tests

0716b28

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

refactor: use Lock_dev_tool.is_enabled for config check

603b28f

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: update ocamllsp tests for new setup style

77f4e3d

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

fix: merlin tests

679dc84

Signed-off-by: Sachin Beniwal <s474996633@gmail.com>

benodiwal force-pushed the improve-tools-exec-error-message branch from 1cbe18e to 679dc84 Compare December 22, 2025 12:38

		Error: The tool ocamlmerlin is not installed.
		Hint: Try running 'dune tools install ocamlmerlin'

	match Config.get Dune_rules.Compile_time.lock_dev_tools with
	match Lazy.force Lock_dev_tool.is_enabled with

	let lock_ocamlformat () =
	if Lazy.force Lock_dev_tool.is_enabled
	then
	(* Note that generating the ocamlformat lockdir here means
	that it will be created when a user runs `dune fmt` but not
	when a user runs `dune build @fmt`. It's important that
	this logic remain outside of `dune build`, as `dune
	build` is intended to only build targets, and generating
	a lockdir is not building a target. *)
	Lock_dev_tool.lock_dev_tool Ocamlformat \|> Memo.run
	else Fiber.return ()

	let lock_utop_if_dev_tool_enabled () =
	match Lazy.force Lock_dev_tool.is_enabled with
	\| false -> Memo.return ()
	\| true -> Lock_dev_tool.lock_dev_tool Utop

fix: show clear error when running uninstalled dev tool #12978

Are you sure you want to change the base?

fix: show clear error when running uninstalled dev tool #12978

Uh oh!

Conversation

benodiwal commented Dec 16, 2025

Uh oh!

Uh oh!

benodiwal commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shonfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benodiwal commented Dec 19, 2025

Uh oh!

shonfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benodiwal commented Dec 22, 2025

Uh oh!

Leonidas-from-XIV commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants