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.

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

Fix #3766 - Libraries without modules #3793

Closed
wants to merge 1 commit into from

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Sep 15, 2020

Libraries without any modules must specify (modules) so that dune knows
whether it needs a native archive for this library without evaluating
any rules.

Libraries without any modules must specify (modules) so that dune knows
whether it needs a native archive for this library without evaluating
any rules.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg requested a review from a user Sep 15, 2020
@rgrinberg
Copy link
Member Author

rgrinberg commented Sep 15, 2020

cc @kit-ty-kate

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Sep 24, 2020

I just launched a test using opam-health-check with this branch, I should get the full result tomorrow. Hopefully this should detect any issue. So far I did try it on a few packages locally and it worked fine.

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Sep 24, 2020

There are a handful of packages that fail with this PR and for which I'm not totally sure if the failures are expected. For instance for C (or javascript) stubs-only libraries:

#=== ERROR while compiling digestif.0.9.0 =====================================#
# context              2.1.0~beta2 | linux/x86_64 | ocaml-variants.4.12.0+trunk | file:///home/opam/opam-repository
# path                 ~/.opam/4.12.0+trunk/.opam-switch/build/digestif.0.9.0
# command              ~/.opam/4.12.0+trunk/bin/dune build -p digestif -j 71
# exit-code            1
# env-file             ~/.opam/log/digestif-6-543ebd.env
# output-file          ~/.opam/log/digestif-6-543ebd.out
### output ###
# [...]
# File "src-c/native/dune", line 1, characters 0-203:
# 1 | (library
# 2 |  (name rakia)
# 3 |  (public_name digestif.rakia)
# 4 |  (foreign_stubs
# 5 |   (language c)
# 6 |   (names blake2b blake2s md5 ripemd160 sha1 sha256 sha512 sha3 whirlpool misc
# 7 |     stubs)
# 8 |   (flags
# 9 |    (:standard -I.))))
# Error: This library does not contain any modules. This should be specified
# explicitly by setting an empty field: (modules).
#=== ERROR while compiling ocaml-lua.1.8 ======================================#
# context              2.1.0~beta2 | linux/x86_64 | ocaml-variants.4.12.0+trunk | file:///home/opam/opam-repository
# path                 ~/.opam/4.12.0+trunk/.opam-switch/build/ocaml-lua.1.8
# command              ~/.opam/4.12.0+trunk/bin/dune build -p ocaml-lua -j 71 @install
# exit-code            1
# env-file             ~/.opam/log/ocaml-lua-6-b76d46.env
# output-file          ~/.opam/log/ocaml-lua-6-b76d46.out
### output ###
# File "src/lua_c/dune", line 1, characters 0-114:
# 1 | (library (name lua_c) (public_name ocaml-lua.c) (preprocess no_preprocessing)
# 2 |  (self_build_stubs_archive (lua_c)))
# Error: This library does not contain any modules. This should be specified
# explicitly by setting an empty field: (modules).
#=== ERROR while compiling zarith_stubs_js.v0.14.0 ============================#
# context              2.1.0~beta2 | linux/x86_64 | ocaml-variants.4.12.0+trunk | file:///home/opam/opam-repository
# path                 ~/.opam/4.12.0+trunk/.opam-switch/build/zarith_stubs_js.v0.14.0
# command              ~/.opam/4.12.0+trunk/bin/dune build -p zarith_stubs_js -j 71
# exit-code            1
# env-file             ~/.opam/log/zarith_stubs_js-6-335865.env
# output-file          ~/.opam/log/zarith_stubs_js-6-335865.out
### output ###
# File "dune", line 1, characters 0-193:
# 1 | (library (name zarith_stubs_js) (public_name zarith_stubs_js)
# 2 |  (js_of_ocaml (javascript_files ./biginteger.js ./runtime.js)
# 3 |   (flags --no-sourcemap))
# 4 |  (libraries) (preprocess no_preprocessing))
# Error: This library does not contain any modules. This should be specified
# explicitly by setting an empty field: (modules).

In those cases I would expect a non-empty <lib>.a to be present at the end. Could you confirm this is the expected behaviour?
List of packages that are failing due to this on OCaml 4.12:

  • jane-street-headers
  • digestif
  • checkseum
  • lbfgs
  • zarith_stubs_js
  • glfw-ocaml
  • ocaml-lua
  • ubpf

gettext and stdlib-shims.0.1.0 (fixed in 0.2.0) also fail but for good reasons: empty OCaml library without setting (module )

@dra27
Copy link
Collaborator

dra27 commented Sep 24, 2020

Oo - is ocaml-gettext an actual in the wild case of a library with no modules?!

@rgrinberg
Copy link
Member Author

rgrinberg commented Sep 28, 2020

There are a handful of packages that fail with this PR and for which I'm not totally sure if the failures are expected. For instance for C (or javascript) stubs-only libraries:

Yeah, that makes sense. Will amend this PR.

The only exception seems to be the zarith js stubs, for that package a (modules) field will still be required.

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Sep 28, 2020

Oo - is ocaml-gettext an actual in the wild case of a library with no modules?!

I think the author thought an intermediate empty library was necessary in dune to "initialize" the base library as previously gettext was just a base namespace library just for gettext.base, gettext.extension and gettext.extract to exist.

gildor478/ocaml-gettext@48d320e#diff-5929aa3a82d5e73ebf049c97f9ce3462

I think this could easily be just removed.

@rgrinberg rgrinberg changed the title Fix #3766 Fix #3766 - Libraries without modules Oct 1, 2020
@rgrinberg
Copy link
Member Author

rgrinberg commented Oct 5, 2020

Me and @jeremiedimino discussed this again, and we propose a simpler solution that will not break any existing packages:

We'll just generate an empty module for libraries that do not have a module. We will not install the cmi for this module, so it will be invisible. @kit-ty-kate wdyt

@rgrinberg rgrinberg added this to the 2.8 milestone Oct 5, 2020
@kit-ty-kate
Copy link
Member

kit-ty-kate commented Oct 6, 2020

Me and @jeremiedimino discussed this again, and we propose a simpler solution that will not break any existing packages:

We'll just generate an empty module for libraries that do not have a module. We will not install the cmi for this module, so it will be invisible. @kit-ty-kate wdyt

I don't know, I guess I personally liked the current proposal (this PR) a bit more as it would not create any unnecessary empty modules. It feels a bit cleaner I guess.
But ultimately if it's better for you, as long as it fixes the issue I'll be personally happy with anything. Note that I could fix the broken packages when needed, those broken packages weren't a big deal.
One thing however, if I understand correctly, that it completely removes the ability to create real empty libraries with dune. @dra27 was the change in the compiler meant to be also used by users in any way or was it to just to unlock something else?

@dra27
Copy link
Collaborator

dra27 commented Oct 6, 2020

That idea was discussed when we hit the original problem in stdlib-shims of genuinely needing an empty library - the conclusion there was that any name picked was still the possibility for a link error at some point in the future, since hiding the .cmi still reserves the module name.

However, those other failures are for wrapped libraries, aren't they? Truly empty libraries can only happen for (wrapped false), right, so am I missing something?

@dra27
Copy link
Collaborator

dra27 commented Oct 8, 2020

Further follow-up - I just hit this in a vendored build - things like ppx_base and ppx_jane were failing too (i.e. libraries which are just collections of other libraries). In this instance, there's definitely no problem because Ppx_base and Ppx_jane are empty modules created by Dune and so OCaml will always generate a .lib file. This patch on top of this PR is working well for me:

diff --git a/src/dune_rules/lib_archives.ml b/src/dune_rules/lib_archives.ml
index 02e1badfa..6adef9702 100644
--- a/src/dune_rules/lib_archives.ml
+++ b/src/dune_rules/lib_archives.ml
@@ -17,7 +17,7 @@ let has_native_archive lib config contents =
   let name = Dune_file.Library.best_name lib in
   let ml_sources = Dir_contents.ocaml contents in
   let modules = Ml_sources.modules_of_library ml_sources ~name in
-  not (Modules.is_empty modules)
+  not (Modules.is_empty modules) || lib.wrapped <> (This (Simple false))

 module Library = Dune_file.Library

diff --git a/src/dune_rules/lib_rules.ml b/src/dune_rules/lib_rules.ml
index 6bc606b1c..c611258a7 100644
--- a/src/dune_rules/lib_rules.ml
+++ b/src/dune_rules/lib_rules.ml
@@ -62,6 +62,7 @@ let build_lib (lib : Library.t) ~sctx ~dir_contents ~expander ~flags ~dir ~mode
          if
            (not (Dune_file.Buildable.no_modules_specified lib.buildable))
            && Modules.is_empty modules
+           && lib.wrapped = (This (Simple false))
          then
            let is_error =
              not

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Oct 28, 2020

@rgrinberg any news on this? As already said in DM, this branch works perfectly with @dra27's patch. I've added it to opam-alpha-repository a while ago, in case you want to test it further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants