Fix optional handling for executables

* if an executable is optional, evaluate its dependencies to see if
  should be available
* if the executable isn't optional, always provide the binary

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

Author: rgrinberg

CHANGES.md

@@ -162,6 +162,10 @@ Unreleased
 - Allow depending on `ocamldoc` library when `ocamlfind` is not installed.
   (#4811, fixes #4809, @nojb)
 
+- Improve lookup of optional or disabled binaries. Previously, we'd treat every
+  executable with missing libraries as optional. Now, we treat make sure to
+  look at the library's optional or enabled_if status (#4786).
+
 2.9.1 (unreleased)
 ------------------
 

src/dune_rules/artifacts.mli

@@ -15,8 +15,6 @@ module Bin : sig
     -> Action.Prog.t Memo.Build.t
 
   val add_binaries : t -> dir:Path.Build.t -> File_binding.Expanded.t list -> t
-
-  val create : context:Context.t -> local_bins:Path.Build.Set.t -> t
 end
 
 module Public_libs : sig

src/dune_rules/expander.ml

@@ -731,6 +731,11 @@ module With_reduced_var_set = struct
   let expand_str_partial ~context ~dir sw =
     String_with_vars.expand_as_much_as_possible ~dir:(Path.build dir) sw
       ~f:(expand_pform_opt ~context ~bindings:Pform.Map.empty ~dir)
+
+  let eval_blang ~context ~dir blang =
+    Blang.eval
+      ~f:(expand_pform ~context ~bindings:Pform.Map.empty ~dir)
+      ~dir:(Path.build dir) blang
 end
 
 let expand_and_eval_set t set ~standard =
@@ -744,9 +749,7 @@ let expand_and_eval_set t set ~standard =
   Ordered_set_lang.eval set ~standard ~eq:String.equal ~parse:(fun ~loc:_ s ->
       s)
 
-let eval_blang t = function
-  | Blang.Const x -> Memo.Build.return x (* common case *)
-  | blang ->
-    Blang.eval blang ~dir:(Path.build t.dir) ~f:(No_deps.expand_pform t)
+let eval_blang t blang =
+  Blang.eval ~f:(No_deps.expand_pform t) ~dir:(Path.build t.dir) blang
 
 let find_package t pkg = t.find_package pkg

src/dune_rules/expander.mli

@@ -130,6 +130,9 @@ module With_reduced_var_set : sig
     -> dir:Path.Build.t
     -> String_with_vars.t
     -> String_with_vars.t Memo.Build.t
+
+  val eval_blang :
+    context:Context.t -> dir:Path.Build.t -> Blang.t -> bool Memo.Build.t
 end
 
 (** Expand forms of the form (:standard \ foo bar). Expansion is only possible

src/dune_rules/super_context.ml

@@ -461,29 +461,40 @@ let get_installed_binaries stanzas ~(context : Context.t) =
             binaries_from_install files
           | Dune_file.Executables
               ({ install_conf = Some { section = Section Bin; files; _ }; _ } as
-              exes) ->
-            let* compile_info =
-              let project = Scope.project d.scope in
-              let dune_version = Dune_project.dune_version project in
-              let+ pps =
-                Resolve.read_memo_build
-                  (Preprocess.Per_module.with_instrumentation
-                     exes.buildable.preprocess
-                     ~instrumentation_backend:
-                       (Lib.DB.instrumentation_backend (Scope.libs d.scope)))
-                >>| Preprocess.Per_module.pps
-              in
-              Lib.DB.resolve_user_written_deps_for_exes (Scope.libs d.scope)
-                exes.names exes.buildable.libraries ~pps ~dune_version
-                ~allow_overlaps:exes.buildable.allow_overlapping_dependencies
+              exes) -> (
+            let* enabled_if =
+              Expander.With_reduced_var_set.eval_blang ~context ~dir:d.ctx_dir
+                exes.enabled_if
             in
-            let available =
-              Resolve.is_ok (Lib.Compile.direct_requires compile_info)
-            in
-            if available then
-              binaries_from_install files
-            else
-              Memo.Build.return Path.Build.Set.empty
+            match enabled_if with
+            | false -> Memo.Build.return Path.Build.Set.empty
+            | true -> (
+              match exes.optional with
+              | false -> binaries_from_install files
+              | true ->
+                let* compile_info =
+                  let project = Scope.project d.scope in
+                  let dune_version = Dune_project.dune_version project in
+                  let+ pps =
+                    Resolve.read_memo_build
+                      (Preprocess.Per_module.with_instrumentation
+                         exes.buildable.preprocess
+                         ~instrumentation_backend:
+                           (Lib.DB.instrumentation_backend (Scope.libs d.scope)))
+                    >>| Preprocess.Per_module.pps
+                  in
+                  Lib.DB.resolve_user_written_deps_for_exes (Scope.libs d.scope)
+                    exes.names exes.buildable.libraries ~pps ~dune_version
+                    ~allow_overlaps:
+                      exes.buildable.allow_overlapping_dependencies
+                in
+                let available =
+                  Resolve.is_ok (Lib.Compile.direct_requires compile_info)
+                in
+                if available then
+                  binaries_from_install files
+                else
+                  Memo.Build.return Path.Build.Set.empty))
           | _ -> Memo.Build.return Path.Build.Set.empty)
       >>| Path.Build.Set.union_all)
   >>| Path.Build.Set.union_all

test/blackbox-tests/test-cases/optional-executable.t/run.t

@@ -120,13 +120,17 @@ present even if the binary is not optional.
 
   $ mkdir bin
   $ cat >bin/dunetestbar <<EOF
-  > /usr/bin/env bash
+  > #!/usr/bin/env bash
   > echo shadow
   > EOF
   $ chmod +x ./bin/dunetestbar
 
   $ PATH=./bin:$PATH dune build @run-x
-  binary path: $TESTCASE_ROOT/optional-binary-absent/./bin/dunetestbar
+  File "exe/dune", line 3, characters 12-29:
+  3 |  (libraries doesnotexistatall)
+                  ^^^^^^^^^^^^^^^^^
+  Error: Library "doesnotexistatall" not found.
+  [1]
 
 Optional on the executable should be respected:
 
@@ -139,6 +143,7 @@ Optional on the executable should be respected:
   > EOF
 
   $ PATH=./bin:$PATH dune build @run-x
+  binary path: $TESTCASE_ROOT/optional-binary-absent/./bin/dunetestbar
 
 In the same way as enabled_if:
 
@@ -149,11 +154,8 @@ In the same way as enabled_if:
   >  (name bar))
   > EOF
 
-  $ PATH=./bin:$PATH dune build @run-x
-  Error: No rule found for install bin/dunetestbar
-  -> required by %{bin:dunetestbar} at dune:3
-  -> required by alias run-x in dune:1
-  [1]
+  $ PATH=./bin:$PATH dune build @run-x --force
+  binary path: $TESTCASE_ROOT/optional-binary-absent/./bin/dunetestbar
 
   $ cd ..