Dedicated public cmi dir for package private libs

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

src/dune_rules/dune_file.ml

@@ -746,9 +746,16 @@ module Library = struct
   let is_impl t = Option.is_some t.implements
 
   let obj_dir ~dir t =
+    let private_lib =
+      match t.visibility with
+      | Private (Some _) -> true
+      | Private None
+      | Public _ ->
+        false
+    in
     Obj_dir.make_lib ~dir
       ~has_private_modules:(t.private_modules <> None)
-      (snd t.name)
+      ~private_lib (snd t.name)
 
   let main_module_name t : Lib_info.Main_module_name.t =
     match (t.implements, t.wrapped) with

src/dune_rules/install_rules.ml

@@ -134,12 +134,29 @@ end = struct
     let modes = Dune_file.Mode_conf.Set.eval lib.modes ~has_native in
     let { Mode.Dict.byte; native } = modes in
     let module_files =
+      let inside_subdir f =
+        match lib_subdir with
+        | None -> f
+        | Some d -> Filename.concat d f
+      in
+      let cmi_dir =
+        (* TODO we should get this from the external obj dir *)
+        let public_cmi_dir =
+          match Lib_info.status info with
+          | Private (_, Some _) -> Some (inside_subdir ".public_cmi")
+          | _ -> lib_subdir
+        in
+        let private_cmi_dir = inside_subdir ".private" in
+        function
+        | Visibility.Public -> public_cmi_dir
+        | Private -> Some private_cmi_dir
+      in
       let virtual_library = Library.is_virtual lib in
       List.concat_map installable_modules ~f:(fun m ->
           let cm_file_unsafe kind =
             Obj_dir.Module.cm_file_unsafe obj_dir m ~kind
           in
-          let cmi_file = (Module.visibility m, cm_file_unsafe Cmi) in
+          let cmi_file = (cmi_dir (Module.visibility m), cm_file_unsafe Cmi) in
           let other_cm_files =
             let has_impl = Module.has ~ml_kind:Impl m in
             [ if_ (native && has_impl) [ cm_file_unsafe Cmx ]
@@ -151,7 +168,7 @@ end = struct
                   Obj_dir.Module.cmt_file obj_dir m ~ml_kind)
             ]
             |> List.concat
-            |> List.map ~f:(fun f -> (Visibility.Public, f))
+            |> List.map ~f:(fun f -> (lib_subdir, f))
           in
           cmi_file :: other_cm_files)
     in
@@ -169,13 +186,7 @@ end = struct
     in
     List.concat
       [ sources
-      ; List.map module_files ~f:(fun (visibility, file) ->
-            let sub_dir =
-              match ((visibility : Visibility.t), lib_subdir) with
-              | Public, _ -> lib_subdir
-              | Private, None -> Some ".private"
-              | Private, Some dir -> Some (Filename.concat dir ".private")
-            in
+      ; List.map module_files ~f:(fun (sub_dir, file) ->
             make_entry ?sub_dir Lib file)
       ; List.map lib_files ~f:(make_entry Lib)
       ; List.map execs ~f:(make_entry Libexec)

src/dune_rules/obj_dir.ml

@@ -22,50 +22,65 @@ module External = struct
   type t =
     { public_dir : Path.t
     ; private_dir : Path.t option
+    ; public_cmi_dir : Path.t option
     }
 
-  let make ~dir ~has_private_modules =
+  let make ~dir ~has_private_modules ~private_lib =
     let private_dir =
       if has_private_modules then
         Some (Path.relative dir ".private")
       else
         None
     in
-    { public_dir = dir; private_dir }
+    let public_cmi_dir =
+      if private_lib then
+        Some (Path.relative dir ".public_cmi")
+      else
+        None
+    in
+    { public_dir = dir; private_dir; public_cmi_dir }
 
   let public_cmi_dir t = t.public_dir
 
-  let to_dyn { public_dir; private_dir } =
+  let to_dyn { public_dir; private_dir; public_cmi_dir } =
     let open Dyn.Encoder in
     record
       [ ("public_dir", Path.to_dyn public_dir)
       ; ("private_dir", option Path.to_dyn private_dir)
+      ; ("public_cmi_dir", option Path.to_dyn public_cmi_dir)
       ]
 
   let cm_dir t (cm_kind : Cm_kind.t) (visibility : Visibility.t) =
-    match (cm_kind, visibility, t.private_dir) with
-    | Cmi, Private, Some p -> p
-    | Cmi, Private, None ->
+    match (cm_kind, visibility, t.private_dir, t.public_cmi_dir) with
+    | Cmi, Private, Some p, _ -> p
+    | Cmi, Private, None, _ ->
       Code_error.raise "External.cm_dir" [ ("t", to_dyn t) ]
-    | Cmi, Public, _
-    | (Cmo | Cmx), _, _ ->
+    | Cmi, Public, _, Some public_cmi_dir -> public_cmi_dir
+    | Cmi, Public, _, None
+    | (Cmo | Cmx), _, _, _ ->
       t.public_dir
 
-  let encode { public_dir; private_dir } =
+  let encode { public_dir; private_dir; public_cmi_dir } =
     let open Dune_lang.Encoder in
     let extract d =
       Path.descendant ~of_:public_dir d |> Option.value_exn |> Path.to_string
     in
     let private_dir = Option.map ~f:extract private_dir in
-    record_fields [ field_o "private_dir" string private_dir ]
+    let public_cmi_dir = Option.map ~f:extract public_cmi_dir in
+    record_fields
+      [ field_o "private_dir" string private_dir
+      ; field_o "public_cmi_dir" string public_cmi_dir
+      ]
 
   let decode ~dir =
+    let public_dir = dir in
     let open Dune_lang.Decoder in
     fields
-      (let+ private_dir = field_o "private_dir" string in
-       let public_dir = dir in
+      (let+ private_dir = field_o "private_dir" string
+       and+ public_cmi_dir = field_o "public_cmi_dir" string in
        let private_dir = Option.map ~f:(Path.relative dir) private_dir in
-       { public_dir; private_dir })
+       let public_cmi_dir = Option.map ~f:(Path.relative dir) public_cmi_dir in
+       { public_dir; private_dir; public_cmi_dir })
 
   let byte_dir t = t.public_dir
 
@@ -79,8 +94,8 @@ module External = struct
 
   let all_obj_dirs t ~mode:_ = [ t.public_dir ]
 
-  let all_cmis { public_dir; private_dir } =
-    List.filter_opt [ Some public_dir; private_dir ]
+  let all_cmis { public_dir; private_dir; public_cmi_dir } =
+    List.filter_opt [ Some public_dir; private_dir; public_cmi_dir ]
 
   let cm_public_dir t (cm_kind : Cm_kind.t) =
     match cm_kind with
@@ -96,20 +111,23 @@ module Local = struct
     ; native_dir : Path.Build.t
     ; byte_dir : Path.Build.t
     ; public_cmi_dir : Path.Build.t option
+    ; private_lib : bool
     }
 
-  let to_dyn { dir; obj_dir; native_dir; byte_dir; public_cmi_dir } =
+  let to_dyn { dir; obj_dir; native_dir; byte_dir; public_cmi_dir; private_lib }
+      =
     let open Dyn.Encoder in
     record
       [ ("dir", Path.Build.to_dyn dir)
       ; ("obj_dir", Path.Build.to_dyn obj_dir)
       ; ("native_dir", Path.Build.to_dyn native_dir)
       ; ("byte_dir", Path.Build.to_dyn byte_dir)
       ; ("public_cmi_dir", option Path.Build.to_dyn public_cmi_dir)
+      ; ("private_lib", bool private_lib)
       ]
 
-  let make ~dir ~obj_dir ~native_dir ~byte_dir ~public_cmi_dir =
-    { dir; obj_dir; native_dir; byte_dir; public_cmi_dir }
+  let make ~dir ~obj_dir ~native_dir ~byte_dir ~public_cmi_dir ~private_lib =
+    { dir; obj_dir; native_dir; byte_dir; public_cmi_dir; private_lib }
 
   let need_dedicated_public_dir t = Option.is_some t.public_cmi_dir
 
@@ -134,22 +152,22 @@ module Local = struct
     in
     Path.Build.Set.of_list dirs |> Path.Build.Set.to_list
 
-  let make_lib ~dir ~has_private_modules lib_name =
+  let make_lib ~dir ~has_private_modules ~private_lib lib_name =
     let obj_dir = Paths.library_object_directory ~dir lib_name in
     let public_cmi_dir =
       Option.some_if has_private_modules (Paths.library_public_cmi_dir ~obj_dir)
     in
     make ~dir ~obj_dir
       ~native_dir:(Paths.library_native_dir ~obj_dir)
       ~byte_dir:(Paths.library_byte_dir ~obj_dir)
-      ~public_cmi_dir
+      ~public_cmi_dir ~private_lib
 
   let make_exe ~dir ~name =
     let obj_dir = Paths.executable_object_directory ~dir name in
     make ~dir ~obj_dir
       ~native_dir:(Paths.library_native_dir ~obj_dir)
       ~byte_dir:(Paths.library_byte_dir ~obj_dir)
-      ~public_cmi_dir:None
+      ~public_cmi_dir:None ~private_lib:false
 
   let cm_dir t cm_kind _ =
     match cm_kind with
@@ -190,11 +208,11 @@ let decode ~dir =
   let+ external_ = External.decode ~dir in
   External external_
 
-let make_lib ~dir ~has_private_modules lib_name =
-  Local (Local.make_lib ~dir ~has_private_modules lib_name)
+let make_lib ~dir ~has_private_modules ~private_lib lib_name =
+  Local (Local.make_lib ~dir ~has_private_modules ~private_lib lib_name)
 
 let make_external_no_private ~dir =
-  External (External.make ~dir ~has_private_modules:false)
+  External (External.make ~dir ~has_private_modules:false ~private_lib:false)
 
 let get_path :
     type a. a t -> l:(Local.t -> Path.Build.t) -> e:(External.t -> Path.t) -> a
@@ -226,7 +244,8 @@ let convert_to_external (t : Path.Build.t t) ~dir =
   match t with
   | Local e ->
     let has_private_modules = Local.need_dedicated_public_dir e in
-    External (External.make ~dir ~has_private_modules)
+    External
+      (External.make ~dir ~has_private_modules ~private_lib:e.private_lib)
   | _ -> assert false
 
 let all_cmis (type path) (t : path t) : path list =

src/dune_rules/obj_dir.mli

@@ -57,6 +57,7 @@ val all_obj_dirs : 'path t -> mode:Mode.t -> 'path list
 val make_lib :
      dir:Path.Build.t
   -> has_private_modules:bool
+  -> private_lib:bool
   -> Lib_name.Local.t
   -> Path.Build.t t
 

test/blackbox-tests/test-cases/private-package-lib.t/run.t

@@ -40,7 +40,6 @@ The naming convention puts the artifacts of private libs under __private__:
 
   $ ls _build/install/default/lib/foo/__private__/secret | grep -i \.cm
   secret.cma
-  secret.cmi
   secret.cmt
   secret.cmx
   secret.cmxa
@@ -86,9 +85,9 @@ Cmi for secret library must not be visible for normal users. Hence they must be
 hidden.
 
   $ grep __private__ _build/default/foo.install
+    "_build/install/default/lib/foo/__private__/secret/.public_cmi/secret.cmi" {"__private__/secret/.public_cmi/secret.cmi"}
     "_build/install/default/lib/foo/__private__/secret/secret.a" {"__private__/secret/secret.a"}
     "_build/install/default/lib/foo/__private__/secret/secret.cma" {"__private__/secret/secret.cma"}
-    "_build/install/default/lib/foo/__private__/secret/secret.cmi" {"__private__/secret/secret.cmi"}
     "_build/install/default/lib/foo/__private__/secret/secret.cmt" {"__private__/secret/secret.cmt"}
     "_build/install/default/lib/foo/__private__/secret/secret.cmx" {"__private__/secret/secret.cmx"}
     "_build/install/default/lib/foo/__private__/secret/secret.cmxa" {"__private__/secret/secret.cmxa"}
@@ -124,6 +123,18 @@ Now we try to use the library in a subproject:
   $ dune exec ./subproj/subproj.exe
   from library foo secret string
 
+But we shouldn't be able to access it directly:
+
+  $ cat >subproj/subproj.ml <<EOF
+  > print_endline Secret.secret
+  > EOF
+  $ dune exec ./subproj/subproj.exe
+  File "subproj/subproj.ml", line 1, characters 14-27:
+  1 | print_endline Secret.secret
+                    ^^^^^^^^^^^^^
+  Error: Unbound module Secret
+  [1]
+
 Now we make sure such libraries are transitively usable when installed:
 
   $ mkdir use
@@ -150,4 +161,8 @@ But we cannot use such libraries directly:
   $ dune exec --root use -- ./run.exe
   Entering directory 'use'
   Entering directory 'use'
-  direct access attempt: secret string
+  File "run.ml", line 1, characters 43-56:
+  1 | print_endline ("direct access attempt: " ^ Secret.secret)
+                                                 ^^^^^^^^^^^^^
+  Error: Unbound module Secret
+  [1]