Defer computation of local_bins

This fixes a memo dependency cycle between evaluating globs in install
stanzas and populating the artifacts database. Populating the artifacts
database involves enumerating all files installed in the "bin" section
which involves expanding globs as these files can be specified as globs
rather than literal files. Expanding globs in the install stanza
requires loading the rules for the directory containing the glob, and
doing so depends on the artifacts database.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>

CHANGES.md

@@ -79,6 +79,9 @@ Unreleased
 - Fix *js_of_ocaml* separate compilation rules when `--enable=effects`
   or `--enable=use-js-string` is used. (#6714, @hhugo)
 
+- Fix dependency cycle when installing files to the bin section with
+  `glob_files` (#6764, fixes #6708, @gridbugs)
+
 3.6.1 (2022-11-24)
 ------------------
 

src/dune_rules/artifacts.ml

@@ -9,15 +9,19 @@ module Bin = struct
   type t =
     { context : Context.t
     ; (* Mapping from executable names to their actual path in the workspace.
-         The keys are the executable names without the .exe, even on Windows. *)
-      local_bins : Path.Build.t String.Map.t
+         The keys are the executable names without the .exe, even on Windows.
+         Enumerating binaries from install stanzas may involve expanding globs,
+         but the artifacts database is depended on by the logic which expands
+         globs. This field is a memo to break the cycle. *)
+      local_bins : Path.Build.t String.Map.t Memo.t
     }
 
   let binary t ?hint ~loc name =
     if not (Filename.is_relative name) then
       Memo.return (Ok (Path.of_filename_relative_to_initial_cwd name))
     else
-      match String.Map.find t.local_bins name with
+      let* local_bins = t.local_bins in
+      match String.Map.find local_bins name with
       | Some path -> Memo.return (Ok (Path.build path))
       | None -> (
         Context.which t.context name >>| function
@@ -32,7 +36,8 @@ module Bin = struct
       Path.of_filename_relative_to_initial_cwd name
       |> Path.as_outside_build_dir_exn |> Fs_memo.file_exists
     else
-      match String.Map.find t.local_bins name with
+      let* local_bins = t.local_bins in
+      match String.Map.find local_bins name with
       | Some _ -> Memo.return true
       | None -> (
         Context.which t.context name >>| function
@@ -41,7 +46,8 @@ module Bin = struct
 
   let add_binaries t ~dir l =
     let local_bins =
-      List.fold_left l ~init:t.local_bins ~f:(fun acc fb ->
+      let+ local_bins = t.local_bins in
+      List.fold_left l ~init:local_bins ~f:(fun acc fb ->
           let path = File_binding.Expanded.dst_path fb ~dir:(local_bin dir) in
           String.Map.set acc (Path.Build.basename path) path)
     in

src/dune_rules/artifacts.mli

@@ -24,7 +24,7 @@ module Bin : sig
     val create : Path.Build.Set.t -> t
   end
 
-  val create : context:Context.t -> local_bins:Local.t -> t
+  val create : context:Context.t -> local_bins:Local.t Memo.t -> t
 
   val add_binaries : t -> dir:Path.Build.t -> File_binding.Expanded.t list -> t
 end
@@ -46,4 +46,5 @@ type t = private
   ; bin : Bin.t
   }
 
-val create : Context.t -> public_libs:Lib.DB.t -> local_bins:Bin.Local.t -> t
+val create :
+  Context.t -> public_libs:Lib.DB.t -> local_bins:Bin.Local.t Memo.t -> t

src/dune_rules/artifacts_db.ml

@@ -86,8 +86,8 @@ let all =
       let artifacts =
         Memo.lazy_ @@ fun () ->
         let* public_libs = Scope.DB.public_libs context in
-        let* stanzas = Only_packages.filtered_stanzas context in
-        let+ local_bins = get_installed_binaries ~context stanzas in
+        let+ stanzas = Only_packages.filtered_stanzas context in
+        let local_bins = get_installed_binaries ~context stanzas in
         Artifacts.create context ~public_libs ~local_bins
       in
       (context.name, artifacts))

test/blackbox-tests/test-cases/install-glob/install-glob-bin-section-recursive.t

@@ -0,0 +1,33 @@
+Test that files can be installed with recursive globs in the bin section. The
+bin section is a special case in that installable binaries are added to the
+artifact database early in the build which can lead to dependency cycles.
+
+  $ cat >dune-project <<EOF
+  > (lang dune 3.6)
+  > (package (name foo))
+  > EOF
+
+  $ cat >dune <<EOF
+  > (install
+  >  (section bin)
+  >  (files (glob_files_rec a/*.sh)))
+  > EOF
+
+  $ mkdir -p a/b a/c
+  $ touch a/foo.sh a/b/bar.sh a/c/baz.sh
+
+  $ dune build @install
+  $ find _build/install | sort
+  _build/install
+  _build/install/default
+  _build/install/default/bin
+  _build/install/default/bin/a
+  _build/install/default/bin/a/b
+  _build/install/default/bin/a/b/bar.sh
+  _build/install/default/bin/a/c
+  _build/install/default/bin/a/c/baz.sh
+  _build/install/default/bin/a/foo.sh
+  _build/install/default/lib
+  _build/install/default/lib/foo
+  _build/install/default/lib/foo/META
+  _build/install/default/lib/foo/dune-package

test/blackbox-tests/test-cases/install-glob/install-glob-bin-section.t

@@ -0,0 +1,30 @@
+Test that files can be installed with globs in the bin section. The bin section
+is a special case in that installable binaries are added to the artifact
+database early in the build which can lead to dependency cycles.
+
+  $ cat >dune-project <<EOF
+  > (lang dune 3.6)
+  > (package (name foo))
+  > EOF
+
+  $ cat >dune <<EOF
+  > (install
+  >  (section bin)
+  >  (files (glob_files *.sh)))
+  > EOF
+
+  $ cat >hello.sh <<EOF
+  > #!/bin/sh
+  > echo "Hello, World!"
+  > EOF
+
+  $ dune build @install
+  $ find _build/install | sort
+  _build/install
+  _build/install/default
+  _build/install/default/bin
+  _build/install/default/bin/hello.sh
+  _build/install/default/lib
+  _build/install/default/lib/foo
+  _build/install/default/lib/foo/META
+  _build/install/default/lib/foo/dune-package