fix: remove expansion limitation

Previously, we'd limit the variables that we could expand in the install
stanzas. This limitation was needed before we had memo and cycle
detection. Now that we have those, we can just rely on those mechanisms
and let users expand whatever they want.

Signed-off-by: Rudi Grinberg <[email protected]>

<!-- ps-id: 2422ab8d-0dbf-41a9-8e5d-f9f7ff81ebbb -->

doc/changes/10160.md

@@ -0,0 +1,3 @@
+- Remove some unnecessary limitations in the expansions of percent forms in
+  install stanza. For example, the `%{env:..}` form can be used to select files
+  to be installed. (#10160, @rgrinberg)

src/dune_rules/artifacts.ml

@@ -89,10 +89,7 @@ let binary t ?hint ?(where = Install_dir) ~loc name =
        Memo.return @@ Ok (Path.build @@ Path.Build.append_local install_dir dst)
      | Original_path ->
        let+ expanded =
-         File_binding.Unexpanded.expand
-           binding
-           ~dir
-           ~f:(Fdecl.get expand ~context:t.context ~dir)
+         File_binding.Unexpanded.expand binding ~dir ~f:(Fdecl.get expand ~dir)
        in
        let src = File_binding.Expanded.src expanded in
        Ok (Path.build src))

src/dune_rules/artifacts.mli

@@ -42,5 +42,4 @@ val create
   -> local_bins:origin Appendable_list.t Filename.Map.t Memo.Lazy.t
   -> t
 
-val expand
-  : (context:Context.t -> dir:Path.Build.t -> String_with_vars.t -> string Memo.t) Fdecl.t
+val expand : (dir:Path.Build.t -> String_with_vars.t -> string Memo.t) Fdecl.t

src/dune_rules/artifacts_db.ml

@@ -35,28 +35,39 @@ let available_exes ~dir (exes : Executables.t) =
   Resolve.is_ok available
 ;;
 
+let expander = Fdecl.create Dyn.opaque
+
 let get_installed_binaries ~(context : Context.t) stanzas =
   let merge _ x y = Some (Appendable_list.( @ ) x y) in
   let open Memo.O in
-  let expand ~dir sw = Expander.With_reduced_var_set.expand ~context ~dir sw in
-  let expand_str ~dir sw = Expander.With_reduced_var_set.expand_str ~context ~dir sw in
-  let expand_str_partial ~dir sw =
-    Expander.With_reduced_var_set.expand_str_partial ~context ~dir sw
-  in
-  let eval_blang ~dir = Expander.With_reduced_var_set.eval_blang ~dir ~context in
   Memo.List.map stanzas ~f:(fun d ->
     let dir = Path.Build.append_source (Context.build_dir context) (Dune_file.dir d) in
+    let* expander = (Fdecl.get expander) ~dir in
+    let expand_value sw =
+      Expander.expand expander ~mode:Single sw
+      |> Action_builder.evaluate_and_collect_facts
+      >>| fst
+    in
+    let expand_str sw =
+      Expander.expand_str expander sw |> Action_builder.evaluate_and_collect_facts >>| fst
+    in
+    let expand_str_partial sw =
+      Expander.expand_str_partial expander sw
+      |> Action_builder.evaluate_and_collect_facts
+      >>| fst
+    in
+    let eval_blang = Expander.eval_blang expander in
     let binaries_from_install ~enabled_if files =
       let* unexpanded_file_bindings =
-        Install_entry.File.to_file_bindings_unexpanded files ~expand:(expand ~dir) ~dir
+        Install_entry.File.to_file_bindings_unexpanded files ~expand:expand_value ~dir
       in
       Memo.List.map unexpanded_file_bindings ~f:(fun fb ->
         let+ p =
           File_binding.Unexpanded.destination_relative_to_install_path
             fb
             ~section:Bin
-            ~expand:(expand_str ~dir)
-            ~expand_partial:(expand_str_partial ~dir)
+            ~expand:expand_str
+            ~expand_partial:expand_str_partial
         in
         let dst = Path.Local.of_string (Install.Entry.Dst.to_string p) in
         if Path.Local.is_root (Path.Local.parent_exn dst)
@@ -75,13 +86,13 @@ let get_installed_binaries ~(context : Context.t) stanzas =
     |> Memo.List.map ~f:(fun stanza ->
       match Stanza.repr stanza with
       | Install_conf.T { section = _loc, Section Bin; files; enabled_if; _ } ->
-        let enabled_if = eval_blang ~dir enabled_if in
+        let enabled_if = eval_blang enabled_if in
         binaries_from_install ~enabled_if files
       | Executables.T
           ({ install_conf = Some { section = _loc, Section Bin; files; _ }; _ } as exes)
         ->
         let enabled_if =
-          let enabled_if = eval_blang ~dir exes.enabled_if in
+          let enabled_if = eval_blang exes.enabled_if in
           match exes.optional with
           | false -> enabled_if
           | true ->

src/dune_rules/artifacts_db.mli

@@ -1,3 +1,6 @@
 (* This module is separate from [Artifacts] to avoid cycles *)
 
+open Stdune
+
+val expander : (dir:Path.Build.t -> Expander.t Memo.t) Fdecl.t
 val get : Context.t -> Artifacts.t Memo.t

src/dune_rules/expander.ml

@@ -712,26 +712,6 @@ let expand_pform t ~source pform =
     ~human_readable_description:(fun () -> describe_source ~source)
 ;;
 
-let expand_pform_no_deps t ~source pform =
-  Memo.push_stack_frame
-    (fun () ->
-      match
-        match
-          expand_pform_gen
-            ~context:t.context
-            ~bindings:t.bindings
-            ~dir:t.dir
-            ~source
-            pform
-        with
-        | Direct v -> v
-        | Need_full_expander f -> f t
-      with
-      | With _ -> isn't_allowed_in_this_position ~source
-      | Without x -> x)
-    ~human_readable_description:(fun () -> describe_source ~source)
-;;
-
 let expand t ~mode template =
   String_expander.Action_builder.expand
     ~dir:(Path.build t.dir)
@@ -740,6 +720,13 @@ let expand t ~mode template =
     ~f:(expand_pform t)
 ;;
 
+let expand_str_partial t template =
+  String_expander.Action_builder.expand_as_much_as_possible
+    ~dir:(Path.build t.dir)
+    template
+    ~f:(fun ~source pform -> expand_pform t ~source pform >>| Option.some)
+;;
+
 let make_root
   ~scope
   ~scope_host
@@ -785,6 +772,26 @@ let expand_str t sw =
 module No_deps = struct
   open Memo.O
 
+  let expand_pform_no_deps t ~source pform =
+    Memo.push_stack_frame
+      (fun () ->
+        match
+          match
+            expand_pform_gen
+              ~context:t.context
+              ~bindings:t.bindings
+              ~dir:t.dir
+              ~source
+              pform
+          with
+          | Direct v -> v
+          | Need_full_expander f -> f t
+        with
+        | With _ -> isn't_allowed_in_this_position ~source
+        | Without x -> x)
+      ~human_readable_description:(fun () -> describe_source ~source)
+  ;;
+
   let expand_pform = expand_pform_no_deps
 
   let expand t ~mode sw =
@@ -835,60 +842,6 @@ module With_deps_if_necessary = struct
   ;;
 end
 
-module With_reduced_var_set = struct
-  open Memo.O
-
-  let expand_pform_opt ~context ~bindings ~dir ~source pform =
-    let open Memo.O in
-    Memo.push_stack_frame
-      (fun () ->
-        match expand_pform_gen ~context ~bindings ~dir ~source pform with
-        | Need_full_expander _ | Direct (With _) -> Memo.return None
-        | Direct (Without x) -> x >>| Option.some)
-      ~human_readable_description:(fun () -> describe_source ~source)
-  ;;
-
-  let expand_pform ~context ~bindings ~dir ~source pform =
-    expand_pform_opt ~context ~bindings ~dir ~source pform
-    >>| function
-    | Some v -> v
-    | None -> isn't_allowed_in_this_position ~source
-  ;;
-
-  let expand ~context ~dir sw =
-    String_expander.Memo.expand
-      ~dir:(Path.build dir)
-      ~mode:Single
-      sw
-      ~f:(expand_pform ~context ~bindings:Pform.Map.empty ~dir)
-  ;;
-
-  let expand_str ~context ~dir sw =
-    let+ v =
-      String_expander.Memo.expand
-        ~dir:(Path.build dir)
-        ~mode:Single
-        sw
-        ~f:(expand_pform ~context ~bindings:Pform.Map.empty ~dir)
-    in
-    Value.to_string v ~dir:(Path.build dir)
-  ;;
-
-  let expand_str_partial ~context ~dir sw =
-    String_expander.Memo.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_expand.eval
-      ~f:(expand_pform ~context ~bindings:Pform.Map.empty ~dir)
-      ~dir:(Path.build dir)
-      blang
-  ;;
-end
-
 let expand_ordered_set_lang =
   let module Expander =
     Ordered_set_lang.Unexpanded.Expand (struct
@@ -925,8 +878,3 @@ let expand_lock ~base expander (Locks.Lock sw) =
 let expand_locks ~base expander locks =
   Memo.List.map locks ~f:(expand_lock ~base expander) |> Action_builder.of_memo
 ;;
-
-let () =
-  Fdecl.set Artifacts.expand (fun ~context ~dir sw ->
-    With_reduced_var_set.expand_str ~context ~dir sw)
-;;

src/dune_rules/expander.mli

@@ -67,6 +67,7 @@ val expand
 val expand_path : t -> String_with_vars.t -> Path.t Action_builder.t
 val expand_str : t -> String_with_vars.t -> string Action_builder.t
 val expand_pform : t -> Value.t list Action_builder.t String_with_vars.expander
+val expand_str_partial : t -> String_with_vars.t -> String_with_vars.t Action_builder.t
 
 module No_deps : sig
   (** Same as [expand_xxx] but disallow percent forms that introduce action
@@ -90,28 +91,6 @@ module With_deps_if_necessary : sig
   val expand_path : t -> String_with_vars.t -> Path.t list Deps.t
 end
 
-module With_reduced_var_set : sig
-  val expand
-    :  context:Context.t
-    -> dir:Path.Build.t
-    -> String_with_vars.t
-    -> Value.t Memo.t
-
-  val expand_str
-    :  context:Context.t
-    -> dir:Path.Build.t
-    -> String_with_vars.t
-    -> string Memo.t
-
-  val expand_str_partial
-    :  context:Context.t
-    -> dir:Path.Build.t
-    -> String_with_vars.t
-    -> String_with_vars.t Memo.t
-
-  val eval_blang : context:Context.t -> dir:Path.Build.t -> Blang.t -> bool Memo.t
-end
-
 val expand_ordered_set_lang
   :  Ordered_set_lang.Unexpanded.t
   -> dir:Path.t

src/dune_rules/super_context.ml

@@ -423,3 +423,15 @@ module As_memo_key = struct
     let to_dyn (s, p) = Dyn.Tuple [ to_dyn s; Package.Name.to_dyn p ]
   end
 end
+
+let () =
+  Fdecl.set Artifacts_db.expander (fun ~dir ->
+    let* ctx = Context.DB.by_dir dir in
+    let* t = find_exn (Context.name ctx) in
+    expander t ~dir);
+  Fdecl.set Artifacts.expand (fun ~dir sw ->
+    let* ctx = Context.DB.by_dir dir in
+    let* t = find_exn (Context.name ctx) in
+    let* expander = expander t ~dir in
+    Expander.expand_str expander sw |> Action_builder.evaluate_and_collect_facts >>| fst)
+;;

test/blackbox-tests/test-cases/dynamic-include-stanza/cycle.t

@@ -15,8 +15,7 @@
 
   $ dune build
   Error: Dependency cycle between:
-     dynamic_include b/dune in directory a
-  -> dynamic_include a/dune in directory b
+     dynamic_include a/dune in directory b
   -> dynamic_include b/dune in directory a
-  -> required by alias default
+  -> dynamic_include a/dune in directory b
   [1]

test/blackbox-tests/test-cases/install-with-var.t/run.t

@@ -34,11 +34,6 @@ them could cause a dependency cycle (also, most of them make no sense in [dst] a
   > EOF
 
   $ dune build @install
-  File "dune", line 3, characters 24-41:
-  3 |  (files (foobar.txt as "%{env:FOO=foobar}/foo.txt")))
-                              ^^^^^^^^^^^^^^^^^
-  Error: %{env:..} isn't allowed in this position.
-  [1]
 
 This is not a problem outside of bin section:
 
@@ -61,12 +56,6 @@ extension of [src]:
   > EOF
 
   $ dune build @install
-  File "dune", line 3, characters 9-30:
-  3 |  (files (%{env:FOO=foobar.txt} as foo.txt)))
-               ^^^^^^^^^^^^^^^^^^^^^
-  Error: Because this file is installed in the 'bin' section, you cannot use
-  the macro %{env:..} in its basename.
-  [1]
 
 This is fine if the destination extension is already .exe:
 
@@ -97,12 +86,6 @@ Exe basename needs to be fully known if dst is missing though:
   > EOF
 
   $ dune build @install
-  File "dune", line 3, characters 8-25:
-  3 |  (files %{env:FOO=foobar}.txt))
-              ^^^^^^^^^^^^^^^^^
-  Error: Because this file is installed in the 'bin' section, you cannot use
-  the macro %{env:..} in its basename.
-  [1]
 
 When basename is fully known, all is well: