Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not build and install shared libs when not supported #1165

Merged
5 commits merged into from Aug 22, 2018
Merged

Do not build and install shared libs when not supported #1165

5 commits merged into from Aug 22, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 21, 2018

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.config file installed by OCaml and looks for SUPPORT_SHARED_LIBRARIES.

This should fix #1051. /cc @aantron, @gruenewa and @lvicentesanchez

@jeremiedimino
Do not build and install shared libs when not supported
826cbed 
Read `ocamlc -where`/Makefile.config to determine whether this is
supported.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost ghost requested review from rgrinberg and emillon August 21, 2018 16:42
emillon
emillon reviewed Aug 21, 2018
View reviewed changes
src/context.ml Outdated
; "opam_vars", string_hashtbl string t.opam_var_cache
; "ocaml_config", Ocaml_config.sexp_of_t t.ocaml_config
; "which", string_hashtbl (option path) t.which_cache
]

let compare a b = compare a.name b.name

(* Parse the [`ocamlc -where`/makefile_config] file *)
module Makefile_config = struct
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, this code should clearly go in the ocaml_config library. I moved it there

src/lib_rules.ml Outdated
@@ -340,7 +341,7 @@ module Gen (P : Install_rules.Params) = struct

let dep_graphs = Ocamldep.rules cctx in

let dynlink = lib.dynlink in
let dynlink = lib.dynlink && ctx.supports_shared_libraries in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be much clearer than this though

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
end

and 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that

@jeremiedimino
Move Makefile.config parsing to Ocaml_config
ba516c1 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Fix ocaml-config tests
19c4784 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@jeremiedimino
Make sure we can't use lib.dynlink directly
382189c 
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
rgrinberg
rgrinberg reviewed Aug 22, 2018
View reviewed changes
src/ocaml-config/ocaml_config.ml
if len < 0 then
""
else
String.sub line ~pos:(i + 2) ~len)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, you should just introduce String.drop. Otherwise this introduces the possibility for off by 1 errors.

rgrinberg
rgrinberg approved these changes Aug 22, 2018
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ghost
Copy link
Author

ghost commented Aug 22, 2018

I guess it serves its purpose. Since we don't test the case where SUPPORTS_SHARED_LIBRARIES is false, I tend to think we should keep it

emillon
emillon approved these changes Aug 22, 2018
View reviewed changes
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's a bit verbose but it's easier to refactor that way. Let's give it a go!

@ghost ghost merged commit c87d8e9 into ocaml:master Aug 22, 2018
@aantron aantron mentioned this pull request Aug 22, 2018
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Sep 14, 2018
@rgrinberg
[new release] dune (1.2.0)
64d5a2a 
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)
@rgrinberg rgrinberg mentioned this pull request Sep 14, 2018
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dune expects .so files generated even when shared libraries are not supported
3 participants
@rgrinberg @emillon @jeremiedimino