cmxs rule with packed modules

[ghost]

It seems that the default rule for building .cmxs files doesn't work well with packed modules. It tries to link both the .cmx and the .cmxa, which result in an error.

Step to reproduce

git clone https://github.com/janestreet/ppx_driver
cd ppx_driver
git checkout 113.33.00

then edit the myocamlbuild.ml and delete the rule "Generate a cmxs from a cmxa" which is a workaround.

Now make produces this error:

[...]
+ /home/jdimino/code/opam-root/4.02.3/bin/ocamlfind ocamlopt -shared -linkall -thread runner/ppx_driver_runner.cmxa runner/ppx_driver_runner.cmx -o runner/ppx_driver_runner.cmxs
File "_none_", line 1:
Error: Files runner/ppx_driver_runner.cmx and runner/ppx_driver_runner.cmxa
       both define a module named Ppx_driver_runner
Command exited with code 2.
[...]

Workaround

We have this problem in all jane street packages. Our current workaround is to add this rule:

    rule "Generate a cmxs from a cmxa"
      ~dep:"%.cmxa"
      ~prod:"%.cmxs"
      ~insert:`top
      (fun env _ ->
         Cmd (S [ !Options.ocamlopt
                ; A "-shared"
                ; A "-linkall"
                ; A "-I"; A (Pathname.dirname (env "%"))
                ; A (env "%.cmxa")
                ; A "-o"
                ; A (env "%.cmxs")
                ]))

[gasche]

The proposed fix does not work in general: essentially the main thing it does is to override the rule that produces the .cmxs from the .mldylib file (where the problem lies) and replace it by a direct build from the .cmxa (which works in this case). In general it amounts to disabling .mldylib, which we don't want to do, and does not fix the deeper problem.

On the other hand, the deeper problem is... a bit out of my depth. Something goes wrong in some dependency computation code used by the upstream rule, but it's not completely clear what, and it's not clear why a rule to link a library archive from a list of module would need to do this dependency computation anyway. So I tried the following patch, which disable the touchy bit, and it seems to solve your particular issue (and also not break ocamlbuild's bootstrap):

From 79931d692fd36adbee79cb22ed31d0449ce69e7f Mon Sep 17 00:00:00 2001
From: Gabriel Scherer <gabriel.scherer@gmail.com>
Date: Sun, 25 Dec 2016 22:46:16 -0500
Subject: [PATCH] drop library dependencies from link_units

---
 src/ocaml_compiler.ml | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/src/ocaml_compiler.ml b/src/ocaml_compiler.ml
index e6d5bec..4830d8c 100644
--- a/src/ocaml_compiler.ml
+++ b/src/ocaml_compiler.ml
@@ -306,6 +306,7 @@ let native_profile_library_link x = native_profile_link_gen native_lib_linker
   (fun tags -> native_lib_linker_tags tags++"profile") x

 let link_units table extensions cmX_ext cma_ext a_ext linker tagger contents_list cmX env build =
+  ignore a_ext;
   let cmX = env cmX in
   let tags = tagger (tags_of_pathname cmX) in
   let _ = Rule.build_deps_of_tags build tags in
@@ -315,7 +316,6 @@ let link_units table extensions cmX_ext cma_ext a_ext linker tagger contents_lis
     else Pathname.dirname cmX in
   let include_dirs = Pathname.include_dirs_of dir in
   let extension_keys = List.map fst extensions in
-  let libs = prepare_libs cma_ext a_ext cmX build in
   let results =
     build begin
       List.map begin fun module_name ->
@@ -337,16 +337,8 @@ let link_units table extensions cmX_ext cma_ext a_ext linker tagger contents_lis
     caml_transitive_closure
       ~caml_obj_ext:cmX_ext ~caml_lib_ext:cma_ext
       ~hidden_packages ~pack_mode:true module_paths in
-  let full_contents = libs @ module_paths in
-  let deps = List.filter (fun x -> List.mem x full_contents) deps in
-  let deps = (List.filter (fun l -> not (List.mem l deps)) libs) @ deps in
-
-  (* Hack to avoid linking twice with the standard library. *)
-  let stdlib = "stdlib/stdlib"-.-cma_ext in
-  let is_not_stdlib x = x <> stdlib in
-  let deps = List.filter is_not_stdlib deps in
-
-  linker tags deps cmX
+  let module_paths_sorted = List.filter (fun x -> List.mem x module_paths) deps in
+  linker tags module_paths_sorted cmX

 let link_modules = link_units library_index
 let pack_modules = link_units package_index
-- 
2.9.3

If it was easy for me to test the impact on the change on the OPAM universe, I would probably do it and consider applying the change upstream. For now it's not, and I am rather hesitant to make such a change in a part of the code I don't understand so well...