fix(pkg): fix extra files with autolocking

src/dune_pkg/lock_dir.ml

@@ -1116,6 +1116,10 @@ let to_dyn
     ]
 ;;
 
+(* CR-someday Alizter: Remove this when portable lock directories are
+   consolidated with non-portable lock directories. *)
+let uses_versioned_paths t = not (List.is_empty (snd t.solved_for_platforms))
+
 type missing_dependency =
   { dependant_package : Pkg.t
   ; dependency : Package_name.t

src/dune_pkg/lock_dir.mli

@@ -117,6 +117,11 @@ val remove_locs : t -> t
 val equal : t -> t -> bool
 val to_dyn : t -> Dyn.t
 
+(** Returns whether this lock directory uses versioned paths for package
+    files directories. Portable lock directories use versioned paths to
+    handle multiple versions of the same package. *)
+val uses_versioned_paths : t -> bool
+
 (** [create_latest_version packages ~ocaml ~repos
     ~expanded_solver_variable_bindings] raises a [Code_error] if [packages] is
     not closed under the "depends on" relationship between packages. Every

src/dune_rules/pkg_rules.ml

@@ -86,12 +86,13 @@ module Package_universe = struct
     match t with
     | Dependencies ctx -> Lock_dir.get_path ctx
     | Dev_tool dev_tool ->
-      (* CR-Leonidas-from-XIV: It probably isn't always [Some] *)
-      dev_tool
-      |> Lock_dir.dev_tool_external_lock_dir
-      |> Path.external_
-      |> Option.some
-      |> Memo.return
+      Lock_dir.dev_tool_lock_dir dev_tool |> Option.some |> Memo.return
+  ;;
+
+  let lock_dir t =
+    match t with
+    | Dependencies ctx -> Lock_dir.get_exn ctx
+    | Dev_tool dev_tool -> Lock_dir.of_dev_tool dev_tool
   ;;
 end
 
@@ -450,7 +451,7 @@ module Pkg = struct
     ; info : Pkg_info.t
     ; paths : Path.t Paths.t
     ; write_paths : Path.Build.t Paths.t
-    ; files_dir : Path.Build.t option
+    ; files_dir : Path.Build.t
     ; pkg_digest : Pkg_digest.t
     ; mutable exported_env : string Env_update.t list
     }
@@ -1527,58 +1528,14 @@ end = struct
             in
             resolve db dep_loc dep_pkg_digest package_universe)
       and+ files_dir =
-        let* lock_dir =
+        let+ lock_dir_path =
           Package_universe.lock_dir_path package_universe >>| Option.value_exn
+        and+ lock_dir = Package_universe.lock_dir package_universe in
+        let version =
+          Option.some_if (Dune_pkg.Lock_dir.uses_versioned_paths lock_dir) info.version
         in
-        let+ files_dir =
-          let module Pkg = Dune_pkg.Lock_dir.Pkg in
-          (* TODO(steve): simplify this once portable lockdirs become the
-             default. This logic currently handles both the cases where
-             lockdirs are non-portable (the files dir won't have a version
-             number in its name) and the case where lockdirs are portable (the
-             solution may have multiple versions of the same package
-             necessitating version numbers in files dirs to prevent
-             collisions). *)
-          let path_with_version =
-            Pkg.source_files_dir info.name (Some info.version) ~lock_dir
-          in
-          let* path_with_version_exists =
-            Fs_memo.dir_exists (Path.Outside_build_dir.In_source_dir path_with_version)
-          in
-          match path_with_version_exists with
-          | true ->
-            Memo.return @@ Some (Pkg.files_dir info.name (Some info.version) ~lock_dir)
-          | false ->
-            let path_without_version = Pkg.source_files_dir info.name None ~lock_dir in
-            let+ path_without_version_exists =
-              Fs_memo.dir_exists
-                (Path.Outside_build_dir.In_source_dir path_without_version)
-            in
-            (match path_without_version_exists with
-             | true -> Some (Pkg.files_dir info.name None ~lock_dir)
-             | false -> None)
-        in
-        files_dir
-        |> Option.map ~f:(fun (p : Path.t) ->
-          match p with
-          | External e ->
-            let source_path = Dune_pkg.Pkg_workspace.dev_tool_path_to_source_dir e in
-            (match Path.Source.explode source_path with
-             | [ "_build"; ".dev-tools.locks"; dev_tool; files_dir ] ->
-               Path.Build.L.relative
-                 Private_context.t.build_dir
-                 [ "default"; ".dev-tool-locks"; dev_tool; files_dir ]
-             | components ->
-               Code_error.raise
-                 "Package files directory is external source directory, this is \
-                  unsupported"
-                 [ "external", Path.External.to_dyn e
-                 ; "source", Path.Source.to_dyn source_path
-                 ; "components", Dyn.(list string) components
-                 ])
-          | In_source_tree s ->
-            Code_error.raise "Unexpected files_dir path" [ "dir", Path.Source.to_dyn s ]
-          | In_build_dir b -> b)
+        Dune_pkg.Lock_dir.Pkg.files_dir info.name version ~lock_dir:lock_dir_path
+        |> Path.as_in_build_dir_exn
       in
       let id = Pkg.Id.gen () in
       let write_paths =
@@ -2150,6 +2107,9 @@ let rec scan_contents p =
   let module P = Path.Build in
   let dir_contents =
     match Readdir.read_directory_with_kinds (P.to_string p) with
+    | Error (Unix.ENOENT, _, _) ->
+      (* Directory doesn't exist - package has no extra files *)
+      []
     | Ok dir_contents -> dir_contents
     | Error e ->
       Code_error.raise
@@ -2201,21 +2161,15 @@ let build_rule context_name ~source_deps (pkg : Pkg.t) =
           let+ () = Memo.return () in
           let open Action_builder.O in
           [ Action_builder.with_no_targets
-            @@ ((match pkg.files_dir with
-                 | Some files_dir -> Action_builder.path (Path.build files_dir)
-                 | None -> Action_builder.return ())
-                >>> Action_builder.of_memo
-                      (Memo.of_thunk (fun () ->
-                         match pkg.files_dir with
-                         | None -> Memo.return (Path.Set.empty, Dep.Set.empty)
-                         | Some files_dir ->
-                           let deps, source_deps = files files_dir in
-                           Memo.return (source_deps, deps)))
+            @@ (Action_builder.of_memo
+                  (Memo.of_thunk (fun () ->
+                     let deps, source_deps = files pkg.files_dir in
+                     Memo.return (source_deps, deps)))
                 |> Action_builder.dyn_deps
                 >>= fun source_deps ->
                 Path.Set.to_list_map source_deps ~f:(fun src ->
                   let dst =
-                    let prefix = pkg.files_dir |> Option.value_exn |> Path.build in
+                    let prefix = Path.build pkg.files_dir in
                     let local_path = Path.drop_prefix_exn src ~prefix in
                     Path.Build.append_local pkg.write_paths.source_dir local_path
                   in

test/blackbox-tests/test-cases/pkg/autolocking-extra-files.t

@@ -36,15 +36,8 @@ Test to make sure that autlocking with extra files works
   $ dune pkg enabled && echo "package management enabled"
   package management enabled
 
-When trying to install the packages it reports the extra files are not found
+Autolocking correctly handles packages with extra files (patches)
 
-  $ dune build @pkg-install 2>&1 \
-  > | dune_cmd subst '_build/\.sandbox/[0-9a-f]+' '_build/.sandbox/HASH' \
-  > | dune_cmd subst 'ocamlbuild\.0\.0\.1-[0-9a-f]+' 'ocamlbuild.0.0.1-HASH1'
-  Error:
-  open(_build/.sandbox/HASH/_private/default/.pkg/ocamlbuild.0.0.1-HASH1/source/fix.patch): No such file or directory
-  -> required by
-     _build/_private/default/.pkg/ocamlbuild.0.0.1-HASH1/target
-  -> required by alias pkg-install
-  [1]
+  $ dune build @pkg-install 2>&1
+  patched