Allow %{read:...} in more places such as enabled_if (ocaml#4994)

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>

Author: 2 people

CHANGES.md

@@ -227,6 +227,9 @@ Unreleased
   cram test cannot escape the sandbox and pick up some random git or
   mercurial repository on the file system (#4996, @jeremiedimino)
 
+- Allow `%{read:...}` in more places such as `(enabled_if ...)`
+  (#4994, @jeremiedimino)
+
 - Run each action in its own process group so that we don't leave
   stray processes behind when killing actions (#4998, @jeremiedimino)
 

doc/concepts.rst

@@ -249,6 +249,51 @@ generated by an OCaml program via:
 
     List.iter (fun s -> print_string (String.escaped s)) l
 
+#. Dealing with circular dependencies introduced by variables
+
+If you ever see Dune reporting a dependency cycle that involves a
+variable such as `%{read:<path>}`, try to move `<path>` to a different
+directory.
+
+The reason you might see such dependency cycle is because Dune is
+trying to evaluate the `%{read:<path>}` too early. For instance, let's
+consider the following example:
+
+.. code:: lisp
+
+    (rule
+     (targets x)
+     (enabled_if %{read:y})
+     (action ...)
+
+    (rule
+     (with-stdout-to y (...)))
+
+When Dune loads and interprets this file, it decides whether the
+first rule is enabled by evaluating ``%{read:y}``. To
+evaluate ``%{read:y}``, it must build ``y``. To build ``y``, it must
+figure out the build rule that produces ``y``, and in order to do that, it must
+first load and evaluate the above ``dune`` file. You can see how this
+creates a cycle.
+
+Some cycles might be more complex. In any case, when you see such an
+error, the easiest thing to do is move the file that's being read
+to a different directory, preferably a standalone one. You can use the
+:ref:`subdir` stanza to keep the logic self-contained in the same
+``dune`` file:
+
+.. code:: lisp
+
+    (rule
+     (targets x)
+     (enabled_if %{read:dir-for-y/y})
+     (action ...)
+
+    (subdir
+     dir-for-y
+     (rule
+      (with-stdout-to y (...))))
+
 #. Expansion of lists
 
 Forms that expands to list of items, such as ``%{cc}``, ``%{deps}``,

doc/dune-files.rst

@@ -1707,6 +1707,8 @@ run this toplevel with:
 of `library`_. Currently, ``action`` and ``future_syntax`` are not supported
 in the toplevel.
 
+.. _subdir:
+
 subdir
 ------
 

src/dune_engine/action_builder.ml

@@ -95,21 +95,6 @@ let lines_of p =
           (x, Dep.Map.empty))
     }
 
-let strings p =
-  let f x =
-    match Scanf.unescaped x with
-    | Error () ->
-      User_error.raise
-        [ Pp.textf "Unable to parse %s" (Path.to_string_maybe_quoted p)
-        ; Pp.textf
-            "This file must be a list of lines escaped using OCaml's \
-             conventions"
-        ]
-    | Ok s -> s
-  in
-  let+ l = lines_of p in
-  List.map l ~f
-
 let read_sexp p =
   let+ s = contents p in
   Dune_lang.Parser.parse_string s ~fname:(Path.to_string p) ~mode:Single

src/dune_engine/action_builder.mli

@@ -130,10 +130,6 @@ val contents : Path.t -> string t
     of the file at [path] as a list of lines. *)
 val lines_of : Path.t -> string list t
 
-(** [strings path] is like [lines_of path] except each line is unescaped using
-    the OCaml conventions. *)
-val strings : Path.t -> string list t
-
 (** Load an S-expression from a file *)
 val read_sexp : Path.t -> Dune_lang.Ast.t t
 

src/dune_rules/expander.ml

@@ -266,6 +266,23 @@ let[@inline never] invalid_use_of_target_variable t
         ~field:multiplicity ~variable:var_multiplicity;
       assert false)
 
+let expand_read_macro ~dir ~source s ~read ~pack =
+  let path = relative ~source dir s in
+  let read =
+    let open Memo.Build.O in
+    let+ x = Build_system.read_file path ~f:read in
+    pack x
+  in
+  Need_full_expander
+    (fun t ->
+      if Dune_project.dune_version (Scope.project t.scope) >= (3, 0) then
+        Without read
+      else
+        (* To prevent it from working in certain position before Dune 3.0. It'd
+           be nice if we could invite the user to upgrade to (lang dune 3.0),
+           but this is a bigger refactoring. *)
+        With (Action_builder.memo_build read))
+
 let expand_pform_gen ~(context : Context.t) ~bindings ~dir ~source
     (pform : Pform.t) : expansion_result =
   match Pform.Map.find bindings pform with
@@ -571,18 +588,23 @@ let expand_pform_gen ~(context : Context.t) ~bindings ~dir ~source
               (let open Memo.Build.O in
               let+ b = Artifacts.Bin.binary_available t.bin_artifacts_host s in
               b |> string_of_bool |> string))
-      | Read ->
-        let path = relative ~source dir s in
-        Direct
-          (With (Action_builder.map (Action_builder.contents path) ~f:string))
+      | Read -> expand_read_macro ~dir ~source s ~read:Io.read_file ~pack:string
       | Read_lines ->
-        let path = relative ~source dir s in
-        Direct
-          (With (Action_builder.map (Action_builder.lines_of path) ~f:strings))
+        expand_read_macro ~dir ~source s ~read:Io.lines_of_file ~pack:strings
       | Read_strings ->
-        let path = relative ~source dir s in
-        Direct
-          (With (Action_builder.map (Action_builder.strings path) ~f:strings))))
+        expand_read_macro ~dir ~source s ~read:Io.lines_of_file
+          ~pack:(fun lines ->
+            strings
+              (List.map lines ~f:(fun line ->
+                   match Scanf.unescaped line with
+                   | Error () ->
+                     User_error.raise
+                       ~loc:(Loc.in_file (relative ~source dir s))
+                       [ Pp.textf
+                           "This file must be a list of lines escaped using \
+                            OCaml's conventions"
+                       ]
+                   | Ok s -> s)))))
 
 (* Make sure to delay exceptions *)
 let expand_pform_gen ~context ~bindings ~dir ~source pform =

test/blackbox-tests/test-cases/enabled_if/read-macro.t

@@ -0,0 +1,34 @@
+Check that %{read:...} is allowed in enabled_if from Dune 3.0
+
+Doesn't work with < 3.0:
+
+  $ echo '(lang dune 2.9)' > dune-project
+  $ cat >dune <<"EOF"
+  > (rule
+  >  (alias default)
+  >  (enabled_if %{read:x/x})
+  >  (action (echo "Hello, world!\n")))
+  > EOF
+
+We have to use a sub-directory to avoid a circular dependency. The
+circular dependency exist because "enabled_if" fields are evaluated at
+"rule production time" rather than "rule execution time".
+
+  $ mkdir x
+  $ cat >x/dune <<"EOF"
+  > (rule
+  >  (with-stdout-to x (system "printf true")))
+  > EOF
+
+  $ dune build
+  File "dune", line 3, characters 13-24:
+  3 |  (enabled_if %{read:x/x})
+                   ^^^^^^^^^^^
+  Error: %{read:..} isn't allowed in this position.
+  [1]
+
+But works with 3.0:
+
+  $ echo '(lang dune 3.0)' > dune-project
+  $ dune build
+  Hello, world!