Fix native code incremental compilation #238
Conversation
Signed-off-by: David Allsopp <[email protected]>
Previously, in Mode.Native, executables depended on .cmx files only. This was fine when changes were detected by timestamp, but it is possible to semantically alter an .ml such that only the .o/.obj file alters. In this situation, Jbuilder would rebuild the .cmx file, but not relink the executable. Fixes ocaml#237. Signed-off-by: David Allsopp <[email protected]>
Signed-off-by: David Allsopp <[email protected]>
Signed-off-by: David Allsopp <[email protected]>
Signed-off-by: David Allsopp <[email protected]>
|
Title updated to reflect the slightly broader scope! I think this now deals with all the required cases:
|
|
Tested this as well. Looks good. Let's wait for @diml to come back to merge this. Do you mind ocp-indenting your commits? Sorry for the nit. |
| @@ -376,6 +384,8 @@ module Gen(P : Params) = struct | |||
| let src = lib_archive lib ~dir ~ext:(Mode.compiled_lib_ext Native) in | |||
| let dst = lib_archive lib ~dir ~ext:".cmxs" in | |||
| let build = | |||
| Build.dyn_paths (Build.arr (fun () -> [lib_archive lib ~dir ~ext:ctx.ext_lib])) | |||
There was a problem hiding this comment.
My understanding of jbuilder's internals here is a bit shaky, so I'm curious:
why do you need Build.dyn_paths over Build.paths?
There was a problem hiding this comment.
I think (my understanding's often shaky too - there's a virtuous project in the making to write a big document on Jbuilder's internals, just for the exercise of thoroughly understanding it all oneself!) that it's because it's a generated target - so paths is things which should exist already (sources, etc.) and dyn_paths is things which are generated.
However, my change is based more on the fact that the .cmx and .cmxa files elsewhere are incorporated using dyn_paths (via Arg_spec, which generates that part of the build arrow as a side effect).
There was a problem hiding this comment.
Rules are build arrow of types (unit, Action.t) Build.t, but really they could be (unit, { runtime_deps : Path.Set.t; action : Action.t }) Build.t. It is however simpler to build the set of runtime dependencies implicitly since all we do with it is always pass it through or extend it.
In the expanded version, Build.dyn_paths l basically means: Build.arr (fun x -> { x with runtime_deps = Path.Set.union x.runtime_deps (Path.Set.of_list l) }). In any case, Build.paths l is always the same as Build.dyn_paths (Build.arr (fun _ -> l)).
The only difference between the two is that with Build.paths, the dependencies are known statically. This means that we can get most of the DAG without running any command and this is enough to generate the list of package dependencies that goes into the opam file. We use that feature to generate the opam files of all Jane Street packages without hassle.
Additionally, in some cases using Build.paths can improve compilation times as static dependencies are known earlier.
In the end, we should use Build.paths wherever possible.
|
Re ocp-indent, I'll get to it - things like that will become easier when I've finished focusing on native Windows opam and can install things like that more easily! It's a weak excuse, though, as I fixed this PR on Linux... 😉 |
|
I've cherry picked this into master to make a release as this bug is causing grief for quite a few users. @diml feel free to amend/review at your leisure. |
Alter
build_exeso that the executable depends on the object files in native mode, as well as the .cmx files.It may be necessary to extend this to .cmxa?