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

confused by Dune foreign_libraries doc: how to define a C library used by C stubs? #4409

Open
gasche opened this issue Mar 27, 2021 · 16 comments
Labels
docs Documentation improvements

Comments

@gasche
Copy link
Member

gasche commented Mar 27, 2021

I have a simple project that consists of:

  • a simple C library foo
  • an OCaml program prog that uses C stubs that themselves use functions provided by foo

It took me several attempts, and many confused looks at the Dune documentation for dealing with foreign libraries, to get something working. And then, a few weeks later, I realized that the thing was not working (whether it builds or not depends on parallelization choices). Then I went to read the doc on dealing with foreign libraries again, and I got a new solution that was not working. And then another solution that seems to be working, but seems unnecessarily redundant. What is going on?

Project layout (see the complete repro case at https://gitlab.com/gasche-snippets/dune-c-library-repro-case ):

lib-foo/
  foo.c
  foo.h
  dune
prog/
  prog.ml  // uses a function from prog_stubs.c
  prog_stubs.c // includes "../lib-foo/foo.h" and use function from there
  dune
dune

First iteration: declare the library with foreign_library, declare the dependency with foreign_archives

My first iteration declares a foreign_library for foo in lib-foo/dune:

; lib-foo/dune
(foreign_library
 (archive_name foo)
 (language c)
 (names foo)
)

and then uses a foreign_archives stanza to declare that prog depends on foo:

; prog/dune
(executable
 (name prog)
 (foreign_stubs (language c)
   (names prog_stubs))
 (foreign_archives ../lib-foo/foo)
)

This seems to work (it prints 42 as expected):

$ dune clean
$ dune build @all
$ dune exec prog/prog.exe
42

But in fact it does not work:

    $ dune clean
    $ dune exec prog/prog.exe
             gcc prog/prog_stubs.o (exit 1)
    (cd _build/default/prog && /usr/lib64/ccache/gcc -O2 -fno-strict-aliasing -fwrapv -fPIC -D_FILE_OFFSET_BITS=64 -D_REENTRANT -O2 -fno-strict-aliasing -fwrapv -fPIC -g -I /home/gasche/.opam/4.12.0/lib/ocaml -o prog_stubs.o -c prog_stubs.c)
    prog_stubs.c:2:10: fatal error: ../lib-foo/foo.h: No such file or directory
        2 | #include "../lib-foo/foo.h"
          |          ^~~~~~~~~~~~~~~~~~
    compilation terminated.

It looks like Dune does not understand (?) that prog depends on foo.

Reproduction repository: https://gitlab.com/gasche-snippets/dune-c-library-repro-case/-/tree/first-iteration

Second iteration: declare the library with both foreign_library and library, declare the dependency with library

Hidden of the middle of some documentation on foreign build sandboxing (which is clearly not what I'm doing here), there is this suggestion:

The last step is to attach these archives to an OCaml library as follows:

(library
(name bar)
(foreign_archives foo))

Then, whenever you use the bar library, you will also be able to use C functions from libfoo.

I thought that maybe I needed this: maybe depending on bar in this way, I "will be able to use C functions from libfoo". So I did just that: in lib-foo/dune, define both a foreign_library and a library, and in prog/dune, use (libraries foo) instead of (foreign_archives ../foo/foo).

But again this does not work:

$ dune clean
$ dune exec prog/prog.exe
    ocamlopt prog/prog.exe (exit 2)
(cd _build/default && /home/gasche/.opam/4.12.0/bin/ocamlopt.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -o prog/prog.exe lib-foo/foo.cmxa -I lib-foo prog/prog_stubs.o prog/.prog.eobjs/native/dune__exe__Prog.cmx)
/usr/bin/ld: prog/prog_stubs.o: in function `call_foo':
/tmp/repro/_build/default/prog/prog_stubs.c:5: undefined reference to `foo'
collect2: error: ld returned 1 exit status
File "caml_startup", line 1:
Error: Error during linking (exit code 1)

It looks like the issue is that dune detects foo as a dependency, but does not link it into the resulting library. (This may come from the fact that foo is used in C files, but not explicitly in the OCaml code.)

Several Dune issues have discussed the difficulty to basically do what -linkpkg does in ocamlfind (Discuss: dune problems using dynlink plugins, #3141, #2417). But as far as I can tell, none of the solutions proposed in the dynlink case give an easy way to tell dune that prog/prog.exe should link with the foo library.

Reproduction repository: https://gitlab.com/gasche-snippets/dune-c-library-repro-case/-/tree/second-iteration

Third iteration: declare the library with foreign_library and library, declare the dependency with foreign_archives and libraries

Declaring the dependency by using both (libraries foo) and (foreign_archives ../lib-foo/foo) in prog/dune seems to work.

; lib-foo/dune
(library
  (name foo)
  (foreign_archives foo)
)

(foreign_library
 (archive_name foo)
 (language c)
 (names foo)
)
; prog/dune
(executable
 (name prog)
 (foreign_stubs (language c)
   (names prog_stubs))
 (libraries foo)
 (foreign_archives ../lib-foo/foo)
)

Questions:

  • does it, in fact, work?
  • is it the recommended approach?
  • if so, could it be documented more clearly?
  • why is the redundancy (on both ends) necessary?

Reproduction repository: https://gitlab.com/gasche-snippets/dune-c-library-repro-case/-/tree/third-iteration

@nojb
Copy link
Collaborator

nojb commented Mar 27, 2021

I haven't looked carefully at all the attemps, but the first attempt should work if you add (extra_deps ../lib-foo/foo.h) to the (foreign_stubs ...) field:

; prog/dune
(executable
 (name prog)
 (foreign_stubs (language c)
   (names prog_stubs)
   (extra_deps ../lib-foo/foo.h))
 (foreign_archives ../lib-foo/foo)
)

This missing dependency explains why it seems to work depending on the parallelization choice.

I agree that many people struggle with how to use the foreign stubs stanzas, and that the doc should be improved!

@gasche
Copy link
Member Author

gasche commented Mar 27, 2021

My understanding was that include_dirs and extra_deps are used to track dependencies on files that (morally) are part of the (foreign stubs of the) current library/executable object. Here you are suggesting to use it to track a dependency on a file that is part of a separate archive. I thought that this was precisely the job (one of the jobs) of the foreign_archives stanza?

@nojb
Copy link
Collaborator

nojb commented Mar 27, 2021

I thought that this was precisely the job (one of the jobs) of the foreign_archives stanza?

Maybe there is room for improvement here, but the foreign_archives stanza just means "link with this library". The library might be built by dune (as in your example) or not, perhaps the binary archive is put directly in the file system. So the (foreign_archives ..) only declares the library archive as a dependency, and does not try to do anything more clever than that (as in trying to automatically declare a dependency on header files of the library, etc).

@nojb
Copy link
Collaborator

nojb commented Mar 27, 2021

My understanding was that include_dirs and extra_deps are used to track dependencies on files that (morally) are part of the (foreign stubs of the) current library/executable object.

As far as I understand, I don't think any such (moral) restriction is intended :)

@gasche
Copy link
Member Author

gasche commented Mar 27, 2021

Here is why I had the impression that foreign_library would handle .h automagically:

  1. It works: when I build my library, obviously foo.h is copied where prog_stubs.c can find it.
  2. Various mentions in the documentation, for example:

Header files

C/C++ source files may include header files in the same directory as the C/C++ source files or in the same directory group when using include_subdirs.

The header files must have the .h extension.

I understand this as saying:

  • we recognize .h files and do the right thing
  • by default we only recognize .h files in the current directory, otherwise use include_subdirs

@gasche
Copy link
Member Author

gasche commented Mar 27, 2021

Note: reading the documentation again, maybe (include_dirs (library foo)) would make sense in my Dune build. I may try a fourth iteration using this instead of double-library wrapping.

@gasche
Copy link
Member Author

gasche commented Mar 27, 2021

Fourth iteration: foreign_library+(simple)library library, include_dirs+foreign_archive dependency

--- a/lib-foo/dune
+++ b/lib-foo/dune
@@ -1,6 +1,5 @@
 (library
   (name foo)
-  (foreign_archives foo)
 )
--- a/prog/dune
+++ b/prog/dune
@@ -2,7 +2,7 @@
  (name prog)
  (foreign_stubs (language c)
    (names prog_stubs)
+   (include_dirs (lib foo))
  )
- (libraries foo)
  (foreign_archives ../lib-foo/foo)
 )

@gasche
Copy link
Member Author

gasche commented Mar 28, 2021

To move forward, I think that it would be helpful to know what are the recommend ways to implement this (simple?) use-case of hybrid C/OCaml project. Is it one of the current attempts, or something else? We could document that.

I think in an ideal world my preference would be for having a solution looking like this:

; lib-foo/dune
(foreign_library
 (name foo)
 (language c)
 (names foo)
)

; prog/dune
(executable
 (name prog)
 (foreign_stubs (language c)
   (names prog_stubs)
   (foreign_libraries foo)
 )
)

Note:

  • The foreign_stubs section has a (foreign_libraries ...) dependency stanza, which implies (include_dirs ...) at (foreign) compile time and (foreign_archives ...) at link-time.
  • foreign_library has (name foo), so that the dependency can be specified by naming foo rather than relative filesystem paths ../lib-foo/foo.

@nojb
Copy link
Collaborator

nojb commented Mar 28, 2021

I believe the simplest is attempt 1 + (extra_deps ../lib-foo/foo.h) to specify the dependency of prog_stubs.c on foo.h.

Anyway cc-ing @snowleopard who implemented foreign library/stubs support (see #2650 for some of the rationale).

@snowleopard
Copy link
Collaborator

snowleopard commented Mar 29, 2021

Right now the simplest way forward is the first attempt with the extra_deps declaration as @nojb suggests.

@gasche I agree that there is a room for improvement here, especially in terms of documentation. Would you like to help with that, on the basis of the difficulties you just experienced? I'll be happy to review the changes.

The current implementation of foreign code support grew out of an RFC that I wrote some time ago: #2650. I think that we should avoid "automagic" solutions in foreign code support as much as possible, because things here can be (and admittedly already are) very confusing even without any magic. Still, feel free to write up a new RFC for a higher-level support for foreign code if you think it will be a net positive change for Dune users.

@gasche
Copy link
Member Author

gasche commented Mar 29, 2021

I'm happy to help with the documentation if I can, but:

  1. I have already offered to do some other improvement of the Dune documentation (sort stanzas and variables alphabetically I think?) and not gotten to this point of my TODO list yet, so please don't hold your breath.

  2. I am not sure I understand the features yet, so I may not be the best person to write documentation about them.

  3. Of the 7 issues I have opened against Dune, all in the last few months, 5 are documentation or usability issues, and only one of them resulted in an action from an active Dune contributor (it was Jérémie getting bitten by the exact same usability issue, independently of my report I believe, and then fixing the issue).

I understand the benefits of encouraging Dune users to contribute to its development. But (in the small part of the development I see, which is only my own issues) I also notice that active contributors are not actively improving documentation or fixing usability issues, and I think that this is correlated with the fact that the documentation is of unequal quality and usability is disappointing in various places.

You have a bug label which currently has 20 open issues and 42 closed issues; this suggests that around 2/3 of the reported bugs are fixed. There are no usability or documentation labels, and I suspect that if there was, the fixing rate would be much lower. (For my own issues it is 1/5, and the one fix seems mostly independent from the report.)

Crowd-sourcing documentation contributions is great, but if the people implementing new features are not considering documentation a priority, why would we expect future documentation to be better than the current documentation? (Usability issues are even less likely to be fixed this way, given that the barrier to entry for a fix is much higher than for documentation.)

@gasche
Copy link
Member Author

gasche commented Mar 29, 2021

I will change my build system to use extra_deps as suggested. I am not fond of this solution because it requires a relative path from the dependency to the library. I am using the same C library in different dune files in my project, and the relative path needs to be slightly different in various places, which is a pain; but then I already need to do this with (foreign_archives ...) anyway.

@snowleopard snowleopard added the docs Documentation improvements label Mar 29, 2021
@snowleopard
Copy link
Collaborator

@gasche You can use the docs label to mark issues about documentation improvements (I just did that for this issue).

I personally do care about good documentation, and as the implementer of this feature I feel responsible for documenting it well. If you don't have any specific improvements in mind at present, I'll go over the corresponding sections and make them better when I find some time. I wouldn't be surprised if some parts just got stale and need refreshing. Maintaining good documentation is hard.

I will change my build system to use extra_deps as suggested. I am not fond of this solution because it requires a relative path from the dependency to the library.

I agree that it would be nice to avoid relative paths here but the devil is in the details, which is why I think a change like this deserves a new RFC.

Anyway, I self-assigned for now. If anyone has bandwidth to move this issue forward, please feel free to take over.

@snowleopard snowleopard self-assigned this Mar 29, 2021
@bobot bobot added this to the 3.0 milestone Oct 12, 2021
@bobot
Copy link
Collaborator

bobot commented Oct 12, 2021

I'm upgrading this issue to 3.0 milestone so that we don't forget it. The reference section could be improved, and particularly the limitations as also found in #4883 .

@ghost ghost removed this from the 3.0 milestone Oct 20, 2021
@gasche
Copy link
Member Author

gasche commented Aug 4, 2022

Is this issue still relevant, or were people able to improve the documentation of C libraries?

@snowleopard snowleopard removed their assignment Aug 4, 2022
@snowleopard
Copy link
Collaborator

I'm not aware of any improvements specifically in the documentation of C libraries. However, there was some general work on improving Dune's documentation.

I know that @voodoos is currently looking at the foreign stubs code as part of #5649, so perhaps he has a better idea of the state of docs here. What's your take, @voodoos?

Personally, I won't be able to look into this issue any time soon, so I unassigned myself, to make it clear that no one is actively working on it right now.

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

No branches or pull requests

4 participants