Adds support for `findlib.dynload` by bobot · Pull Request #1172 · ocaml/dune

bobot · 2018-08-23T11:45:54Z

It allows to easily dynlink packages and their dependencies. Dune is needed for specifying in the binary the list of packages statically linked.

bobot · 2018-08-23T11:50:10Z

For simplicity I didn't try to add these features:

use the same module for all the executable with the same set of packages
be able to dynamically link private packages when executed locally

ghost · 2018-08-23T13:01:31Z

src/exe.ml

+        in
+        let basename = Format.asprintf "%s_findlib_initl_%a" name Mode.pp mode in
+        let ml  = Path.relative dir (basename ^ ".ml") in
+        let mli = Path.relative dir (basename ^ ".mli") in


In general we try to put such generated modules outside of the source directory. Could you put it in the object directory instead? This is to avoid it being included in (glob_files *.ml).

Additionally, we don't generate a mli for other such generated modules (utop, inline tests, ...) so for consistency we shouldn't do it here either.

ghost · 2018-08-23T13:02:42Z

src/exe.ml

+        let mli = Path.relative dir (basename ^ ".mli") in
+        SC.add_rule sctx (Build.write_file ml s);
+        SC.add_rule sctx (Build.write_file mli "");
+        let impl = Module.File.make Module.Syntax.OCaml ml in


We rely a lot on type-based disambiguation, you can drop the Module.Syntax. here

ghost · 2018-08-23T13:08:41Z

src/exe.ml

        artifacts modules ~ext:ctx.ext_obj))
  in
+  let link_flags_for_requires =
+    Result.map requires ~f:(fun libs ->


Could you move this code to its own module? It's looks big enough and self-contained, and in the future I hope that we'll also support a generic method for link-time code generation (#594). So for instance you can move it to Link_time_code_gen.

ghost · 2018-08-23T13:09:03Z

src/exe.ml

+        let preds = Variant.Set.add preds (Mode.variant mode) in
+        let s =
+          Format.asprintf "%a@\nFindlib.record_package_predicates %a;;@."
+            (Fmt.list ~pp_sep:Fmt.nl (fun fmt lib -> Format.fprintf fmt "Findlib.record_package Findlib.Record_core %S;;" (Lib.name lib)))


This line looks a bit long

ghost · 2018-08-23T13:11:31Z

Thanks, could you also add something in the manual about this?

bobot · 2018-08-23T14:12:13Z

I think all the points have been fixed. Remain the documentation that I think I will put in the advanced section.

For the question of thread and mt, I perhaps choose the wrong solution. The documentation says that:

mt: what this means is that using a library that can be used with or without threads with dune will force the threaded version

Currently the branch remove mt if thread is not used. But that mean that in that case if we dynlink a library we will obtain its not threaded version. But statically we will always have the threaded version.

Another solution would be to force thread (and unix) when findlib.dynload is used.

ghost · 2018-08-23T14:21:17Z

src/exe.ml

+      let l = findlib_dynload ~name ~mode cctx l in
+      let arg_spec =
+        List.map l.libs_or_module ~f:(function
+          | Libs l -> Lib.L.link_flags l ~mode ~stdlib_dir:ctx.stdlib_dir


link_flags does some sharing of flags, so it doesn't seem right to call it multiple time. It should probably take the lib_or_module list type directly.

ghost · 2018-08-23T14:23:49Z

src/link_time_code_gen.mli

+}
+
+
+val create: ?flags:Ocaml_flags.t -> Lib.L.t -> t


I think we can do everything in one step. From the outside of this module, findlib.dynload is basically just an implementation detail.

BTW, for multi-line function signatures, we use the following style in the code base:

val foo : x -> y -> z

ghost · 2018-08-23T14:24:23Z

src/link_time_code_gen.mli

+(** {1 Handle link time code generation} *)
+
+type libs_or_module =
+  | Libs of Lib.L.t


It would seem more natural to me to use Lib of Lib.t

ghost · 2018-08-23T14:28:10Z

BTW, the code looked simpler to me when we were doing everything in one step, i.e. without going through the Link_time_code_gen.t type. What about calling this module simply Linker and returning the Arg_spec.t directly as before?

bobot · 2018-08-23T15:02:50Z

The type Link_time_code_gen.t is an attempt for allowing different link time code generation one after the other. The module doesn't do all the linking so I prefer to keep the current name. So I remove Link_time_code_gen.t and move libs_and_module (renamed in libs_and_modules and with Lib.t) to Lib?

Could you take a look at the documentation? Do you think the laïus about naming convention is interesting?

bobot · 2018-08-23T15:13:59Z

@diml Why Module.t doesn't contains obj_dir, so that we completely know where are its source and where is its compiled unit?

rgrinberg · 2018-08-23T15:26:03Z

@bobot I've also considered adding obj_dir directly to the modules themselves as part of implementing private modules. I think this would be pretty useful.

bobot · 2018-08-23T15:44:30Z

@rgrinberg I will not try it in this MR because in some place obj_dir seems not directly accessible (e.g. dir_content). There is perhaps a time where modules are found on the file system but not yet associated to a library.

All the remarks of @diml except the renaming of Link_time_code_gen into Linker are done.

What is the downside of always linking with thread?

ghost · 2018-08-23T16:14:17Z

doc/advanced-topics.rst

+===========================
+
+Dune supports the ``findlib.dynload`` package that allows to dynamically
+load packages and their dependencies (using OCaml Dynlink modules).


s/modules/module/

ghost · 2018-08-23T16:18:43Z

Patch looks good. Giving an example in the documentation seems good as well. It might be worth adding a pointer to the documentation of findlib and in particular the dynload part.

ghost · 2018-08-23T16:23:41Z

What is the downside of always linking with thread?

Note that we don't always link with threads, only when there is a dependency on it. What dune does is set the findlib predicates so that when a library has a threaded and non-threaded implementation, the threaded one is always selected. This was done this way for historical reasons.

When we have library variants, we can arrange things so that it is possible to select the non-threaded version. Personally, I don't think it's worth writing two implementations, especially as we are planning to have OCaml support multicore one day.

bobot · 2018-08-24T09:54:55Z

We should always link with thread when findlib.dynload is used. Otherwise if you dynlink a library that use thread you have:

error loading shared library: ...: undefined symbol: caml_thread_join

It will also simplify the question about the predicate to use. I'm going to do the modification, but should I just make it an error to not use thread with findlib.dynload so it is explicit, or should I just add it automatically?

bobot · 2018-08-27T09:07:40Z

Now thread is required explicitly when findlib.dynload is used. I think every remark have been addressed.

ghost · 2018-08-27T16:42:47Z

We should always link with thread when findlib.dynload is used. Otherwise if you dynlink a library that use thread you have:

This is not specific to threads though, it's only because OCaml doesn't build and install threads.cmxs, right?

bobot · 2018-08-27T20:51:41Z

Oh perhaps, yes indeed 😭 I though threads was special because of the runtime, but it seems it just needs to register an hook. So if the compiler installs the cmxs, it should work while keeping all the predicates. So I can remove the requirement for the thread library in this PR, and we add to the compiler and previous version in opam the installation of the cmxs ? The META for the standard library are in ocamlfind, which would need an update. 👨‍🏭

ghost · 2018-08-28T08:00:46Z

Yep, that seems better. Findlib could also detect this case and report an error.

ghost · 2018-08-28T10:17:33Z

BTW, the rest of the PR looks good, I'm happy to merge it if you remove the check for threads.

The META for the standard library are in ocamlfind, which would need an update.

I have been thinking that we should just move the META files to the compiler distribution. That would simplify things

which allows to easily dynlink packages and their dependencies. Dune is needed for putting in the binary the list of package statically linked. Signed-off-by: François Bobot <francois.bobot@cea.fr>

bobot · 2018-08-28T16:55:29Z

if you remove the check for threads.

It is removed, rebased and squashed.

ghost · 2018-08-28T17:10:09Z

Thanks!

@diml

CHANGES: - Ignore stderr output when trying to find out the number of jobs available (ocaml/dune#1118, fix ocaml/dune#1116, @diml) - Fix error message when the source directory of `copy_files` does not exist. (ocaml/dune#1120, fix ocaml/dune#1099, @emillon) - Highlight error locations in error messages (ocaml/dune#1121, @emillon) - Display actual stanza when package is ambiguous (ocaml/dune#1126, fix ocaml/dune#1123, @emillon) - Add `dune unstable-fmt` to format `dune` files. The interface and syntax are still subject to change, so use with caution. (ocaml/dune#1130, fix ocaml/dune#940, @emillon) - Improve error message for `dune utop` without a library name (ocaml/dune#1154, fix ocaml/dune#1149, @emillon) - Fix parsing `ocamllex` stanza in jbuild files (ocaml/dune#1150, @rgrinberg) - Highlight multi-line errors (ocaml/dune#1131, @anuragsoni) - Do no try to generate shared libraries when this is not supported by the OS (ocaml/dune#1165, fix ocaml/dune#1051, @diml) - Fix `Flags.write_{sexp,lines}` in configurator by avoiding the use of `Stdune.Path` (ocaml/dune#1175, fix ocaml/dune#1161, @rgrinberg) - Add support for `findlib.dynload`: when linking an executable using `findlib.dynload`, automatically record linked in libraries and findlib predicates (ocaml/dune#1172, @bobot) - Add support for promoting a selected list of files (ocaml/dune#1192, @diml) - Add an emacs mode providing helpers to promote correction files (ocaml/dune#1192, @diml) - Improve message suggesting to remove parentheses (ocaml/dune#1196, fix ocaml/dune#1173, @emillon) - Add `(wrapped (transition "..message.."))` as an option that will generate wrapped modules but keep unwrapped modules with a deprecation message to preserve compatibility. (ocaml/dune#1188, fix ocaml/dune#985, @rgrinberg) - Fix the flags passed to the ppx rewriter when using `staged_pps` (ocaml/dune#1218, @diml) - Add `(env var)` to add a dependency to an environment variable. (ocaml/dune#1186, @emillon) - Add a simple version of a polling mode: `dune build -w` keeps running and restarts the build when something change on the filesystem (ocaml/dune#1140, @kodek16) - Cleanup the way we detect the library search path. We no longer call `opam config var lib` in the default build context (ocaml/dune#1226, @diml) - Make test stanzas honor the -p flag. (ocaml/dune#1236, fix ocaml/dune#1231, @emillon) - Test stanzas take an optional (action) field to customize how they run (ocaml/dune#1248, ocaml/dune#1195, @emillon) - Add support for private modules via the `private_modules` field (ocaml/dune#1241, fix ocaml/dune#427, @rgrinberg) - Add support for passing arguments to the OCaml compiler via a response file when the list of arguments is too long (ocaml/dune#1256, @diml) - Do not print diffs by default when running inside dune (ocaml/dune#1260, @diml) - Interpret `$ dune build dir` as building the default alias in `dir`. (ocaml/dune#1259, @rgrinberg) - Make the `dynlink` library available without findlib installed (ocaml/dune#1270, fix ocaml/dune#1264, @rgrinberg)

bobot requested a review from a user August 23, 2018 11:45

bobot requested review from emillon and rgrinberg as code owners August 23, 2018 11:45

bobot force-pushed the fl_dynload branch 3 times, most recently from e8cfc43 to 2e2fe48 Compare August 23, 2018 12:07

ghost suggested changes Aug 23, 2018

View reviewed changes

bobot force-pushed the fl_dynload branch from 2e2fe48 to 5698160 Compare August 23, 2018 14:00

ghost suggested changes Aug 23, 2018

View reviewed changes

bobot force-pushed the fl_dynload branch from 5698160 to fb6c75e Compare August 23, 2018 14:51

bobot force-pushed the fl_dynload branch from fb6c75e to e78ff44 Compare August 23, 2018 15:36

ghost suggested changes Aug 23, 2018

View reviewed changes

bobot force-pushed the fl_dynload branch 2 times, most recently from 9fd5d6e to 2cb4dea Compare August 27, 2018 09:03

bobot force-pushed the fl_dynload branch 2 times, most recently from 9a0b866 to b563399 Compare August 28, 2018 14:42

Adds support for findlib.dynload

22fc961

which allows to easily dynlink packages and their dependencies. Dune is needed for putting in the binary the list of package statically linked. Signed-off-by: François Bobot <francois.bobot@cea.fr>

bobot force-pushed the fl_dynload branch from b563399 to 22fc961 Compare August 28, 2018 14:54

ghost merged commit 7f09979 into ocaml:master Aug 28, 2018

rgrinberg mentioned this pull request Sep 14, 2018

[new release] dune (1.2.0) ocaml/opam-repository#12622

Merged

arlencox mentioned this pull request Sep 21, 2018

Generating .so file results in link error #1310

Closed

bobot mentioned this pull request Nov 8, 2018

Make (nat)dynlink sound (also fixes MPR#4208, MPR#4229, MPR#4839, MPR#6462, MPR#6957, MPR#6950) ocaml/ocaml#1063

Merged

This pull request was closed.

Conversation

bobot commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

bobot commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

bobot commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented Aug 23, 2018

Uh oh!

rgrinberg commented Aug 23, 2018

Uh oh!

bobot commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

bobot commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented Aug 27, 2018

Uh oh!

ghost commented Aug 27, 2018

Uh oh!

bobot commented Aug 27, 2018

Uh oh!

ghost commented Aug 28, 2018

Uh oh!

ghost commented Aug 28, 2018

Uh oh!

bobot commented Aug 28, 2018

Uh oh!

ghost commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bobot commented Aug 23, 2018 •

edited

Loading

bobot commented Aug 23, 2018 •

edited

Loading

bobot commented Aug 23, 2018 •

edited

Loading

bobot commented Aug 24, 2018 •

edited

Loading