review

Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>

src/dune/foreign.ml

@@ -230,8 +230,8 @@ module Sources = struct
                Raising in [Not_allowed_until] can break backwards compatibility
                when we change a file from [Unrecognized] to [Not_allowed_until].
 
-               One way could be to instead pass the dune language version here
-               and interpret those as [Unrecognized]. *)
+               An easy fix would be to treat [Not_allowed_until] as
+               [Unrecognized], but the error messages are not good then. *)
             let loc = Loc.in_dir (Path.build dir) in
             User_error.raise ~loc
               [ Pp.textf

src/dune/foreign_sources.ml

@@ -40,6 +40,9 @@ let eval_foreign_sources (d : _ Dir_with_dune.t) foreign_stubs
     let osl = stubs.names in
     Ordered_set_lang.Unordered_string.eval_loc osl
       ~key:(fun x -> x)
+      (* CR-someday aalekseyev:
+         Might be a good idea to change [standard] to mean
+         "all files with the relevant extension". *)
       ~standard:String.Map.empty
       ~parse:(fun ~loc s ->
         let name = valid_name language ~loc s in
@@ -53,8 +56,13 @@ let eval_foreign_sources (d : _ Dir_with_dune.t) foreign_stubs
             ];
         name)
     |> String.Map.map ~f:(fun (loc, name) ->
-           match String.Map.find sources name with
-           | Some paths when List.length paths > 1 ->
+      match String.Map.find sources name with
+      | Some (_ :: _ :: _ as paths) ->
+          (* CR aalekseyev:
+             This looks suspicious to me.
+             If the user writes foo.c and foo.cpp and only declares a foreign
+             library that uses foo.cpp, will that be an error?
+             I think it shouldn't be. *)
              User_error.raise ~loc
                [ Pp.textf "Multiple sources map to the same object name %S:"
                    name
@@ -77,13 +85,13 @@ let eval_foreign_sources (d : _ Dir_with_dune.t) foreign_stubs
                  ]
            | Some [ (l, path) ] when l = language ->
              (loc, Foreign.Source.make ~stubs ~path)
-           | _ ->
+           | Some [] | None | [ (_wrong_lang, _) ] ->
              User_error.raise ~loc
                [ Pp.textf "Object %S has no source; %s must be present." name
                    (String.enumerate_one_of
                       ( Foreign.Language.possible_fns language name
                           ~dune_version:d.dune_version
-                      |> List.map ~f:(fun s -> "\"" ^ s ^ "\"") ))
+                      |> List.map ~f:(fun s -> sprintf "%S" s) ))
                ])
   in
   let stub_maps = List.map foreign_stubs ~f:eval in

src/dune/lib_rules.ml

@@ -136,12 +136,17 @@ let ocamlmklib ~path ~loc ~c_library_flags ~sctx ~dir ~expander ~o_files
        ; Hidden_targets targets
        ])
 
-(* Add a rule calling [ocamlmklib] to build an OCaml library. *)
+(* Add a rule calling [ocamlmklib] to build a stubs archive for an OCaml
+   library. *)
 let ocamlmklib_ocaml (lib : Library.t) ~sctx ~dir ~expander ~o_files ~sandbox
     ~custom ~targets =
   let path =
     Path.build (Path.Build.relative dir (Library.stubs_archive_name lib))
   in
+  (* CR-someday aalekseyev:
+     I'm not sure why [c_library_flags] is needed here. I think it's unused
+     at least when building a static archive. But maybe it's used for
+     dynamic libraries. It would be nice to clarify that somewhere. *)
   ocamlmklib ~path ~loc:lib.buildable.loc ~c_library_flags:lib.c_library_flags
     ~sctx ~dir ~expander ~o_files ~sandbox ~custom ~targets
 
@@ -194,6 +199,10 @@ let include_dir_flags ~expander ~dir (stubs : Foreign.Stubs.t) =
                         Command.Args.Hidden_deps deps :: args))
                ] )))
 
+(* CR aalekseyev:
+   Maybe we'll need to support the case when dynamic library can't be
+   built for some reason. It seems that the OCaml library code path has to deal
+   with that case. *)
 (* Build a static and a dynamic archive for a foreign library. *)
 let foreign_rules (library : Foreign.Library.t) ~sctx ~expander ~dir
     ~dir_contents =