search

issuu / ocaml-protoc-plugin

ocaml-protoc-plugin
https://issuu.github.io/ocaml-protoc-plugin/
Other
48 stars 19 forks source link

do not force google types to be in /usr/include or /usr/local/include #26

Closed vprevosto closed 3 years ago

vprevosto commented 3 years ago

I've been trying to install ocaml-protoc-plugin in nix through opam2nix, and stumbled upon the fact that in this setting google types are neither in /usr/include nor in /usr/local/include, as assumed by the dune rule in src/google_types, but somewhere in /nix/store/xxxxxxxx-protobuf-vn.n.n/include. The proposed patch assumes that include is a sibling of the bin directory where protoc lies (if protoc is not in the PATH at this point, the build will fail anyway).

andersfugmann commented 3 years ago

Thanks. I agree that the current solution is far from optimal. A better solution would be to use pkg-config, but I don't have access to all the different platforms out there. Does Nix support pkg-config? If it does, could you provide the output of pkg-config --variable=includedir protobuf, and tell me if $(pkg-config --variable=includedir protobuf)/google/protobuf directory exists?

vprevosto commented 3 years ago

Thanks for your answer. A colleague of mine indeed pointed out to me #19, which (despite its name 😛) contains a similar proposal. I'll check whether pkg-config is pulled by opam2nix (or protobuf itself) one way or the other. Otherwise, I have to admit that I'd be a bit reluctant to make a specific dependency just for that. I'll keep you informed.

By the way: I have found that the stanza (enabled_if {%arch_sixtyfour}) in the tests in rejected by dune (on my machine, version 2.8.2) as well as on the CI here. Is that normal?

vprevosto commented 3 years ago

OK, if conf-pkg-config is added as a build-dependency in the opam package, nix seems happy. Would this additional dependency be acceptable?

andersfugmann commented 3 years ago

Awesome. This looks like a good solution! Thanks. For the arch_sixtyfour - its not intentional. It should be fixed already.