Do not build and install shared libs when not supported#1165
Do not build and install shared libs when not supported#11655 commits merged intomasterfrom unknown repository
Conversation
Read `ocamlc -where`/Makefile.config to determine whether this is supported. Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
src/context.ml
Outdated
| let compare a b = compare a.name b.name | ||
|
|
||
| (* Parse the [`ocamlc -where`/makefile_config] file *) | ||
| module Makefile_config = struct |
There was a problem hiding this comment.
What do you think about putting this in its own module and making t abstract? This is a small addition, but it's part of a large module so it seems worth doing it to keep things manageable.
There was a problem hiding this comment.
Looking at this again, this code should clearly go in the ocaml_config library. I moved it there
src/lib_rules.ml
Outdated
| let dep_graphs = Ocamldep.rules cctx in | ||
|
|
||
| let dynlink = lib.dynlink in | ||
| let dynlink = lib.dynlink && ctx.supports_shared_libraries in |
There was a problem hiding this comment.
This pattern (lib.dynlink && ctx.supports_shared_libraries) appears 3 times. Is it worth extracting a combinator for it? (I guess Context would be the natural place for it)
There was a problem hiding this comment.
I don't think it would be much clearer than this though
There was a problem hiding this comment.
Yes, but I'm wondering whether we'll think of adding && ctx.supports_shared_libraries next time we'll use lib.dynlink. The current solution works for me, though.
There was a problem hiding this comment.
I guess we could add an API like this:
module Dynlink_supported : sig
type t
val of_bool : bool -> t
module Global : sig
type t
val of_bool : bool -> t
end
val get : t -> Global.t -> bool
endand change the type of lib.dynlink to Dynlink_supported.t and the type of ctx.supports_shared_libraries to Dynlink_supported.Global.t. Then we are sure to catch such errors.
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
| if len < 0 then | ||
| "" | ||
| else | ||
| String.sub line ~pos:(i + 2) ~len) |
There was a problem hiding this comment.
Imo, you should just introduce String.drop. Otherwise this introduces the possibility for off by 1 errors.
rgrinberg
left a comment
There was a problem hiding this comment.
This Dynlink_supported modules seems like its more trouble than its worth. But I don't have a strong opinion on it and it's unlikely to introduce additional maintenance pain
|
I guess it serves its purpose. Since we don't test the case where |
emillon
left a comment
There was a problem hiding this comment.
I agree that it's a bit verbose but it's easier to refactor that way. Let's give it a go!
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)
After this PR, dune no longer tries to build and install shared libraries when this is not supported by the OS. To determine whether this is supported, dune reads the contents of the
Makefile.configfile installed by OCaml and looks forSUPPORT_SHARED_LIBRARIES.This should fix #1051. /cc @aantron, @gruenewa and @lvicentesanchez