mgree commented 9 months ago

I'm working on libdash, which is offers OCaml and Python bindings to the dash POSIX shell parser. dune is being uncooperative around packaging the library up properly (i.e., with both bindings and code). My work is in the ~~ocaml-static-ctypes branch~~ main branch (since OCaml bindings don't work right now anyway). (I originally posted this on discuss.ocaml.org, but wasn't able to resolve things there.)

The structure is:

an autotools build constructs libdash.a and dlldash.so from the (lightly modified dash source)
some static ctypes bindings connects the C interface to OCaml
a bunch of OCaml converts the C ASTs into more sensible structures
some executables use the OCaml ASTs to do some work

A critical property here is that the C library is not a common one, and must be distributed with the OCaml source. The produced OCaml native library should include all of the C code.

dune woes

I---after several years of trying off and on---have gotten dune to build the project locally and have tests pass. (I posted here three years ago.) But there's a problem: the build fails with multiple uses of libdash.a. Here's the output:

$ dune build
Error: Multiple rules generated for
_build/install/default/lib/libdash/libdash.a:
- ocaml/dune:8
- ocaml/dune:8
-> required by _build/default/libdash.install
-> required by alias all
-> required by alias default

The offending clause:

(library
  (name libdash)
  (public_name libdash)
  (modes native)
  (modules (:standard \ json_to_shell shell_to_json ast_json))
  (libraries ctypes ctypes.foreign)
  (foreign_archives ../dash)
  (ctypes
    (external_library_name dash)
    (build_flags_resolver (vendored (c_flags :standard) (c_library_flags :standard)))
    (deps (glob_files ../src/*.h) ../src/builtins.h ../src/nodes.h ../src/syntax.h ../src/token.h ../src/token_vars.h)
    (headers (preamble
              "\
             \n#include \"../src/shell.h\"\
             \n#include \"../src/memalloc.h\"\
             \n#include \"../src/mystring.h\"\
             \n#include \"../src/init.h\"\
             \n#include \"../src/main.h\"\
             \n#include \"../src/input.h\"\
             \n#include \"../src/var.h\"\
             \n#include \"../src/alias.h\"\
             \n#include \"../src/redir.h\"\
             \n#include \"../src/parser.h\"\
             \n#include \"../src/nodes.h\"\
             \n"))
    (type_description
      (instance Types)
      (functor Type_description))
    (function_description
      (instance Functions)
      (functor Function_description))
    (generated_types Types_generated)
    (generated_entry_point Cdash)))

In particular, the problem is my use of (foreign_archives ../dash). This, however, is the key line that makes the executables work. Without it, the linker fails when building the executables and it can't find libdash's symbols.

linking woes

The build works fine if I move (foreign_archives ../dash) to the executables, but then there's a different problem: others trying to link with libdash won't get the actual libdash.a and those clients will fail at link time with missing symbols. I've tried using an install stanza to make sure libdash.a is in (section lib), but it has no effect on libraries that then try to link with dash.

what should i do?

The library is currently built with custom ocamlfind commands that once worked but no longer work on Linux. They're fine on macOS; it's a similar linking issue.

My feeling is that this is a bad interaction between ctypes and dune, and I should simply be able to do this. (I don't see the issue of the multiple dependency on libdash.a, which should indeed be used more than once. That's what libraries are for!) But I would not be the first person to have wrongly felt "I should simply be able to do this"! So... what do I do?

I see a few ways forward, in order of preference:.

Figure out the magic incantation that lets dune actually package the C code with the library.
Give up on Ctypes entirely and interface with C more directly, allowing me to use foreign_archives with the library. (This is the approach taken by re2.)
Give up on OCaml bindings (I am the only client; everyone else uses Python---and I could too, at the cost of some communication overhead).
Create a conf-libdash that does the C library installation and have the OCaml bindings depend on that.

I put (4) last because it is a remarkable amount of faffing about for something that should be really very simple... making me more inclined to take a performance hit in order to no longer need to interact with OCaml (which has been incredibly frustrating in its interaction with C... my last attempt on this was in 2020).

Specifications

Version of dune: 3.12.1
Version of ocaml: 5.2.0
Operating system: Debian trixie

emillon commented 9 months ago

Thanks for the thorough bug report. I want to help you with this. There are a couple alternatives to add to your list:

it's actually a bug in dune's ctypes rules (very likely)
you can also use handwritten ctypes rules instead of (ctypes)

I'll have a detailed look at where the duplicate rule comes from.

emillon commented 9 months ago

So, I have something that seems to work.

I had a look at the output of dune rules and found that the 2 targets are:

the one producing the ocaml library
the one producing the C library

The issue is that when you specify (library (name x)), the native version of the ocaml code is linked into libx.a. In this case, this means that we have two different files names libdash.a: the OCaml one and the C one.

As for the fix in libdash, here's what I came up with:

Full patch

```diff --git a/dune b/dune --- a/dune +++ b/dune @@ -6,6 +6,7 @@ builtins.h nodes.h syntax.h token.h token_vars.h ) (action + (setenv CC "%{cc}" (bash "\ \n set -e\ @@ -17,7 +18,7 @@ \n cp lib/libdash.a libdash.a\ \n cp lib/dlldash.so dlldash.so\ \n cp src/{builtins,nodes,syntax,token,token_vars}.h .\ - \n"))) + \n")))) (subdir src (rule diff --git a/ocaml/dune b/ocaml/dune index 0df89cd..0f897b9 100644 --- a/ocaml/dune +++ b/ocaml/dune @@ -3,18 +3,21 @@ (public_names shell_to_json json_to_shell) (modules shell_to_json json_to_shell ast_json) (modes (native exe)) - (foreign_archives ../dash) (libraries libdash yojson atdgen)) +(rule (copy ../dlldash.so dlldash_native.so)) +(rule (copy ../libdash.a libdash_native.a)) + (library (name libdash) (public_name libdash) (modes native) (modules (:standard \ json_to_shell shell_to_json ast_json)) (libraries ctypes ctypes.foreign) + (foreign_archives dash_native) (ctypes (external_library_name dash) - (build_flags_resolver (vendored (c_flags :standard) (c_library_flags :standard))) + (build_flags_resolver vendored) (deps (glob_files ../src/*.h) ../src/builtins.h ../src/nodes.h ../src/syntax.h ../src/token.h ../src/token_vars.h) (headers (preamble "\ ```

Let's unpack this:

This is the actual fix. Instead of attaching the foreign archive, we put it on the library. But instead of naming it dash, we call it dash_native to change the name of the .a and .so files. But since these are build as libdash.a and dlldash.so, we add copy rules to give them the right name when copying them over.

diff --git a/ocaml/dune b/ocaml/dune
index 0df89cd..0f897b9 100644
--- a/ocaml/dune
+++ b/ocaml/dune
@@ -3,18 +3,21 @@
  (public_names shell_to_json json_to_shell)
  (modules shell_to_json json_to_shell ast_json)
  (modes (native exe))
- (foreign_archives ../dash)
  (libraries libdash yojson atdgen))

+(rule (copy ../dlldash.so dlldash_native.so))
+(rule (copy ../libdash.a libdash_native.a))
+
 (library
   (name libdash)
   (public_name libdash)
   (modes native)
   (modules (:standard \ json_to_shell shell_to_json ast_json))
   (libraries ctypes ctypes.foreign)
+  (foreign_archives dash_native)
   (ctypes
     (external_library_name dash)

The C flags need to be altered, in particular -fPIC was missing in my setup. Using %{cc} (the compiler configured for C compilation by ocaml) works, but there might be other ways to achieve this.

--- a/dune
+++ b/dune
@@ -6,6 +6,7 @@
            builtins.h nodes.h syntax.h token.h token_vars.h
            )
   (action
+    (setenv CC "%{cc}"
     (bash
     "\
      \n set -e\
@@ -17,7 +18,7 @@
      \n cp lib/libdash.a libdash.a\
      \n cp lib/dlldash.so dlldash.so\
      \n cp src/{builtins,nodes,syntax,token,token_vars}.h .\
-     \n")))
+     \n"))))

 (subdir src
   (rule

This bit is actually just a cleanup, it removed the default values of flags (it will execute identically):

-    (build_flags_resolver (vendored (c_flags :standard) (c_library_flags :standard)))
+    (build_flags_resolver vendored)

The problem with that fix is that it might not work when installed. I think you'd have to install the dash_native versions for this to work, so that might not be 100% satisfactory (tell me what you think about this).

In terms of dune, there are several ways to improve the situation:

I don't think we could directly refer to ../libdash.a instead of copying it in the current directory, because in the dynamic case, executables will need to load both the .so of the C and OCaml code. NB: the OCaml code is statically linked in most cases, but if it is built as a plugin, the native code will get loaded dynamically too. These .so files will be installed in the same directory, so they need different names.
the error message is not great. I wonder if it could make sense to add labels to rules (in addition to locations) so that we could see which rules are conflicting.

mgree commented 9 months ago

Thank you! I tried something like this, but less successfully. The patch works in CI; I'll test it out on my Linux machine as soon as I can.

emillon commented 9 months ago

Nice. Actually, the issue can be reproduced without (ctypes) at all: if a library named libx, it will create a libx.a and libx.so, and a foreign archive named x will create a libx.a and dllx.so so there will be a conflict. I think that the best thing we can do is try to detect that and error out before generating rules.

mgree commented 9 months ago

Nice! I don't know if that's the best you could do: the renaming dance here (which I still need to confirm works for my case... sorry to be slow!) could in principle be automated.

mgree commented 9 months ago

Just tested and things seem to work. Thank you very much for this fix!

mgree commented 9 months ago

(Last comment for now, sorry for the spam.) Should I mark this as closed, or will there be something done to smooth the way for this? I would be willing to write a bit of documentation about this approach to things, so that others might have an easier time getting here.

rgrinberg commented 9 months ago

Just an FYI, the ctypes extension is experimental. So if you prefer more stability at the cost of a little more boilerplate, I would suggest not using it and just writing the build rules by hand.

F-Loyer commented 9 months ago

Ctypes is the way proposed by the Real World Ocaml book to bind C to OCaml... then a good support would be welcome.

I have filed a proposal here: https://github.com/ocaml/dune/discussions/9906. Then there is a need for a dune support for libraries which should be accessed dynamicaly (Ctypes would be the main use case), but the need is mitigated since we can statically link the library with a single dummy external __dummy_foo__ : unit -> int = "foo". However, I have just discovered the ctypes stanza and will try it and see if it fits the need. However, I guess from https://dune.readthedocs.io/en/stable/foreign-code.html#ctypes-stubgen that the main program should be tagged as using a ctypes library. It would be more convenient to be able to tag an Ocaml library as using an external C library, put it in something like dune-package and make it transparent for the user of the OCaml library. If not possible, the best way would be to include a dummy external statement in the OCaml library making things transparent from the user of the library.

ocaml / dune

Ctypes and foreign bindings won't bundle library properly #9773

dune woes

linking woes

what should i do?

Specifications