-I option not passed during development for headers installed by a sibling package from a monorepo
Closed this issue · 14 comments
Expected Behavior
Luv vendors libuv, and installs libuv's header uv.h at lib/luv/uv.h using an install stanza (see also #3907 about this stanza).
When a project that depends on package luv is compiled, Dune correctly passes -I path/to/switch/lib/luv when compiling the project's C files, which allows them to pull in uv.h with #include <uv.h>.
I recently added package luv_unix to the Luv repo (in branch unix), which depends on luv and needs uv.h. After pinning everything, doing
opam install luv
opam install luv_unix
works, because the package installations are separate, and uv.h is in the switch by the time luv_unix is installed.
The expected behavior is that during development,
dune build
or some variation on it, should also work. luv_unix should be able to find uv.h.
Actual Behavior
$ dune build
gcc src/unix/luv_unix.o (exit 1)
luv_unix.c:9:10: fatal error: uv.h: No such file or directory
9 | #include <uv.h>
| ^~~~~~
compilation terminated.
This can be seen immediately in Travis logs. The command failing there is trying to build Luv's tester, which in branch unix depends on luv_unix.
Reproduction
opam pin add luv https://github.com/aantron/luv.git#unix
opam pin add luv_unix https://github.com/aantron/luv.git#unix
opam install luv luv_unix
opam remove luv luv_unix
git clone https://github.com/aantron/luv.git
cd luv
make test # or dune build -p luv,luv_unix, etc.Specifications
- Version of
dune(output ofdune --version): 2.8.1 - Version of
ocaml(output ofocamlc --version): 4.11.1 - Operating system (distribution and version): Ubuntu 20.04 on WSL1
I didn't get an error with dune 2.8.2. Could you test this version?
git clone https://github.com/aantron/luv.git --recursive
git checkout unix
dune build
(make test is very long, is stuck?)
It is possible that it is non-deterministic, depends if the files have been copied in the local install directory.
I didn't get an error with dune
2.8.2. Could you test this version?
It still occurs with 2.8.2. The most likely reason you didn't observe it is that you may have a system-wide uv.h installed somewhere in default search paths. Use find or locate to find it, and rename it to uvv.h or similar during the Luv build.
It is possible that it is non-deterministic, depends if the files have been copied in the local install directory.
It is possible, and I tried many combinations of dune commands before submitting this issue. I got uv.h installed locally in _build, yet I found no command that would cause Dune to add the arguments for finding it when building luv_unix. The comands you showed do not do that — you can inspect the output of dune --verbose to see, especially after hiding any system-wide uv.h files.
Hi,
I think I got a working repro here:
$ cat > dune-project << EOF
> (lang dune 3.4)
>
> (package (name a))
> (package (name b))
> EOF
$ mkdir a
$ cat > a/dune << EOF
> (library
> (public_name a)
> (foreign_stubs
> (language c)
> (names fifty)
> (include_dirs include))
> (install_c_headers include/fifty))
> EOF
$ cat > a/fifty.c << EOF
> #include "fifty.h"
> int fifty = 50;
> EOF
$ mkdir a/include
$ cat > a/include/fifty.h << EOF
> extern int fifty;
> EOF
$ mkdir b
$ cat > b/dune << EOF
> (library
> (public_name b)
> (foreign_stubs
> (language c)
> (names stubs))
> (libraries a))
> EOF
$ cat > b/stubs.c << EOF
> #include "caml/memory.h"
> #include <fifty.h>
> value get_value(value unit) {
> CAMLparam1(unit);
> CAMLreturn(Val_int(fifty));
> }
> EOF
$ cat > b/b.ml << EOF
> external get_value : unit -> int = "get_value"
> EOF
$ mkdir bin
$ cat > bin/dune << EOF
> (executable
> (name main)
> (libraries b))
> EOF
$ cat > bin/main.ml << EOF
> let () = Printf.printf "value = %d\n" (B.get_value ())
> EOF
$ dune exec bin/main.exe
File "b/dune", line 5, characters 9-14:
5 | (names stubs))
^^^^^
stubs.c:2:10: fatal error: fifty.h: No such file or directory
2 | #include <fifty.h>
| ^~~~~~~~~
compilation terminated.
[1]
Does that sound accurate to you?
The issue as I see it is that we're trying to refer to the installed layout as the package is being built. I see 2 possible approaches here:
- try to generate the installed layout and use that. That would force a dependency on
(package a)which can be a big overapproximation. And I believe that it would compete withsites. - add to
-Ithe paths that we're going to install. It's not really practical to do that from raw(install)stanzas but this can be expressed as(install_c_headers)in the corresponding(library)stanza. There's a bit of overapproximation in here too since there might be some header files in these source directories that are not going to be installed. I suppose that we could also create a sandbox directory with just the stuff to install and add this to the include path.
Exploring the second solution, I hacked together a fix, which is to extend the arguments supported (include_dirs) in (foreign_stubs):
DIRexpands to a single directory(lib L)expands to the directory of whereLis defined- 🆕
(public_headers L)expands to the list of directories referenced in(install_c_headers)inL's definition
Then luv's use case can be supported by changing the dune file to:
(library
(public_name luv_unix)
(libraries luv luv.c result unix)
- (foreign_stubs (language c) (names luv_unix))
+ (foreign_stubs (language c) (names luv_unix) (include_dirs (public_headers luv.c)))
(flags (:standard -w -49 -open Result)))and switching luv.c's definition to use (install_c_headers) instead of raw (install) stanzas.
Hmm, actually if we go that route we'll probably need to extend install_c_headers so that it can preserve the directory structure. that doesn't seem possible at the moment.
@emillon did you manage to get any further with this? It's a pretty big blocker to monorepo workflows where any C-using library (like libuv or ctypes) are present, so it would be very handy for this to work for RWO.
not for now. I was waiting for feedback on this feature idea, and wanted to explore alternative solutions that do not require a new dune feature. but this looks pretty important so we can try to make installing C libraries a bit more first class.
I can try to make a more reviewable branch of dune and have a fork of libuv use it if you'd like.
Besides luv and ctypes, do you have other use cases in mind?
A branch of dune to experiment in a monorepo sounds great, thanks! luv and ctypes are the first two C libraries at the base of the dependency cone. This will affect pretty much every C library in a monorepo that exports header files. (Another example off the top of my head would be yaml, which bundles libyaml)
FWIW I'd be happy to test a branch of dune with https://github.com/moby/vpnkit -- I think switching to a monorepo will simplify the development experience (especially for new devs)
I wrote a poc with 2 features that taken together help fix the issue.
(install_c_headers) accepts, in addition to plain names, a (dir) construct which looks like this:
(install_c_headers
(dir
(root vendor/mylib/include)
(files mylib.h mylib/a.h mylib/b.h)))
Internally, this builds an install stanza that's equivalent to (root/file as file) for file in files.
(This can be improved when directory targets get more powerful (#6133) - then it won't be necessary to list the contents of the directory.)
(include_dirs) gets a new constructor, (public_lib l) (the name is not great), which expands to the set of include directories dune knows about for library l. This relies on install_c_headers: it adds all the directories containing .h files referenced directly, and all roots of (dirs).
(This step is manual - maybe we can be a be more automatic and look at all libraries that the library compiles against, and use the corresponding include dirs when building stubs for the library)
Here's a branch of dune with these features: https://github.com/emillon/dune/tree/workspace-headers and a branch of luv that uses them https://github.com/emillon/luv/tree/workspace-headers.
Together, this makes a cold dune build succeed in luv's source tree!
For ctypes, I think that this can simplify a bit how the headers are accessed (without %{lib:...:*.h}) but I'm not sure what's not working today. I'll have a look at yaml too.
That @emillon, promising progress! Would you be able to try building luv as a monorepo though; since I think that will still fail as it depends on ctypes. (this feature is so that we can build them all as a monorepo).
On vpnkit, with these trees I confirm that:
File "_build/.dune/default/duniverse/luv/src/c/dune", line 93, characters 0-418:
93 | (rule
94 | (targets generate_types_step_2.exe)
95 | (deps (:c generate_types_step_2.c) helpers.h shims.h)
.....
105 | -I %{ocaml_where} \
106 | -I vendor/libuv/include -o %{targets}; \
107 | fi")))
In file included from generate_types_step_2.c:4:
vendor/libuv/include/uv.h:1759:49: warning: "callback" is deprecated: use "caml_callback" instead [-W#pragma-messages]
UV_EXTERN void uv_once(uv_once_t* guard, void (*callback)(void));
^
/nix/store/lzs9pc8k5chyp170mykdsmbrv2shb4q0-ocaml-4.14.0/lib/ocaml/caml/compatibility.h:71:18: note: expanded from macro 'callback'
#define callback CAML_DEPRECATED("callback", "caml_callback") caml_callback
^
/nix/store/lzs9pc8k5chyp170mykdsmbrv2shb4q0-ocaml-4.14.0/lib/ocaml/caml/misc.h:57:3: note: expanded from macro 'CAML_DEPRECATED'
CAML_PREPROWARNING(name1 is deprecated: use name2 instead)
^
/nix/store/lzs9pc8k5chyp170mykdsmbrv2shb4q0-ocaml-4.14.0/lib/ocaml/caml/misc.h:55:31: note: expanded from macro 'CAML_PREPROWARNING'
#define CAML_PREPROWARNING(x) _Pragma(CAML_MAKEWARNING2(x))
^
<scratch space>:142:6: note: expanded from here
GCC warning "\"callback\" is deprecated: use \"caml_callback\" instead"
^
In file included from generate_types_step_2.c:5:
./helpers.h:151:44: warning: "callback" is deprecated: use "caml_callback" instead [-W#pragma-messages]
CAMLprim value luv_set_once_callback(value callback);
^
/nix/store/lzs9pc8k5chyp170mykdsmbrv2shb4q0-ocaml-4.14.0/lib/ocaml/caml/compatibility.h:71:18: note: expanded from macro 'callback'
#define callback CAML_DEPRECATED("callback", "caml_callback") caml_callback
^
/nix/store/lzs9pc8k5chyp170mykdsmbrv2shb4q0-ocaml-4.14.0/lib/ocaml/caml/misc.h:57:3: note: expanded from macro 'CAML_DEPRECATED'
CAML_PREPROWARNING(name1 is deprecated: use name2 instead)
^
/nix/store/lzs9pc8k5chyp170mykdsmbrv2shb4q0-ocaml-4.14.0/lib/ocaml/caml/misc.h:55:31: note: expanded from macro 'CAML_PREPROWARNING'
#define CAML_PREPROWARNING(x) _Pragma(CAML_MAKEWARNING2(x))
^
<scratch space>:248:6: note: expanded from here
GCC warning "\"callback\" is deprecated: use \"caml_callback\" instead"
^
generate_types_step_2.c:8:10: fatal error: 'ctypes_cstubs_internals.h' file not found
#include "ctypes_cstubs_internals.h"
^~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings and 1 error generated.
Thanks for the pointer, I wasn't sure what was wrong with ctypes. The issue was here is code that refers to %{lib:ctypes:.} which fails when ctypes is not installed (not sure why this wasn't an issue when I tried to build luv).
To fix that, I added a %{headers:LIB} construct that expands to the list of public header directories installed by LIB. This is more powerful than (public_lib LIB) since it one can be expressed in terms of the other, and pforms are accepted in more places. Notes: this expands to a list of directories, so things like -I %{headers:LIB} won't work. we don't have a way to automatically add -I to a list of values in dune lang at the moment so that might not be the right design. Also, for now this expands to the empty list for libraries acquired through opam, but it's easy to fix (the "compatible" way is to point to the lib directory, but actually since we're in control of both sides of that feature we can think about the best place to install these headers).
And this works in vpnkit! I created a monorepo branch here and if you pull from it (after pinning dune to the branch mentioned above), dune build @install works.
#7512 attempts partially fixes this issue by preserving the paths given in install_c_headers.
The issue was not fixed by #7512 and shouldn't be closed only on the basis of #7512 being merged.
install_c_headersdid not preserve paths.- Therefore, I used an
(install ...)stanza withas(otherwise(install)also doesn't preserve paths):(install (section lib) (package luv) (files (vendor/libuv/include/uv.h as uv.h) (vendor/libuv/include/uv/aix.h as uv/aix.h) (vendor/libuv/include/uv/bsd.h as uv/bsd.h) (vendor/libuv/include/uv/darwin.h as uv/darwin.h) (vendor/libuv/include/uv/errno.h as uv/errno.h) (vendor/libuv/include/uv/linux.h as uv/linux.h) (vendor/libuv/include/uv/os390.h as uv/os390.h) (vendor/libuv/include/uv/posix.h as uv/posix.h) (vendor/libuv/include/uv/stdint-msvc2008.h as uv/stdint-msvc2008.h) (vendor/libuv/include/uv/sunos.h as uv/sunos.h) (vendor/libuv/include/uv/threadpool.h as uv/threadpool.h) (vendor/libuv/include/uv/tree.h as uv/tree.h) (vendor/libuv/include/uv/unix.h as uv/unix.h) (vendor/libuv/include/uv/version.h as uv/version.h) (vendor/libuv/include/uv/win.h as uv/win.h))) - This works when something depends on Luv when Luv is installed in a switch, but the header files are not found by C code in a sibling package in a monorepo.
- I see no difference in the file structure or
-Iflags passed with(public_headers)during the build in the monorepo. I only see that all the headers now appear in_build/install, but during the monorepo build_build/installis not referenced by the generated-Ioptions, but only_build/defaultis.
So, #7512 appears to have done nothing to address the specific issue of accessing a vendored C library's headers while building in a monorepo.
I appear to have worked around all the issues and supported both builds against an opam install and in a monorepo by manually adding copy actions for every header file, including a dummy uv directory to contain copy rules for headers in the include/uv subdirectory, which covers monorepo builds, and also keeping the (install) stanza, which is needed for opam builds:
Is there a nicer way of doing this with Dune?