ocaml-ppx / ppx_deriving_protobuf

A Protocol Buffers codec generator for OCaml
MIT License
82 stars 12 forks source link

Conflicts with other ppx_deriving plugins #22

Open Leonidas-from-XIV opened 6 years ago

Leonidas-from-XIV commented 6 years ago

When adding a [@@deriving protobuf] stanza to a type it works, but when any other deriver is added to the build (using dune at least), e.g. ppx_deriving.show or ppx_deriving_yojson compilation breaks with

Error: Cannot locate deriver protobuf

All the other derivers work fine with each other.

ahem commented 6 years ago

I can reproduce this like this:

dune:

(library
  (name foo)
  (preprocess (pps ppx_deriving_protobuf ppx_deriving.show)))

foo.ml:

type t = int [@@deriving protobuf, show]
 $ dune build foo/foo.a
         ppx foo/foo.pp.ml (exit 1)
(cd _build/default && .ppx/ppx_deriving.show+ppx_deriving_protobuf/ppx.exe --cookie 'library-name="foo"' -o foo/foo.pp.ml --impl foo/foo.ml --dump-ast)
File "foo/foo.ml", line 1, characters 25-33:
Error: Cannot locate deriver protobuf

If I remove ppx_deriving.show and the show annotation the file builds without problems.

This is with ocaml-base-compiler.4.06.1, ppx_deriving_protobuf version 2.6 and ppx_deriving version 4.2.1

rgrinberg commented 6 years ago

@whitequark now that we've transitioned ppx_deriving_protobuf and ppx_deriving_yojson to dune, how about we proceed with moving them into ppx_deriving? We've had this discussion on github before (I cant find the link), and we agreed on doing this will keeping the opam structure the same. So things would still be published as 3 opam packages, but things will be easier to maintain as the 3 projects will be maintained together. We'd be able to add tests for bugs such as this.

whitequark commented 6 years ago

Sure, go ahead. What do we do with the repos? I would prefer to update them to say that the main repo should now be used instead.

XVilka commented 5 years ago

It is better to skip this step completely and do this only after ppx_deriving is ported to ppxlib (or future ppx).

rgrinberg commented 5 years ago

It would be worse. It is much easier to port everything to ppxlib all at once.