little-arhat / ppx_jsobject_conv

ppx rewriter for [@@deriving jsobject]
MIT License
31 stars 6 forks source link

Doesn't work with jbuilder #4

Closed copy closed 6 years ago

copy commented 6 years ago

This project currently doesn't build with jbuilder:

(* test.ml *)
module Stuff = struct
  type t = int * string * float [@@deriving jsobject]
end

let x = Stuff.of_jsobject
;; jbuild
(jbuild_version 1)

(executable
  ((name test)
   (modules (test))
   (preprocess (pps (ppx_jsobject_conv)))
   (libraries (ppx_jsobject_conv.runtime))))
% jbuilder build test.bc.js
      ocamlc test.{cmi,cmo,cmt} (exit 2)
(cd _build/default && /home/fabian/.opam/4.06.0+flambda/bin/ocamlc.opt -w -40 -g -bin-annot -I /home/fabian/.opam/4.06.0+flambda/lib/bytes -I /home/fabian/.opam/4.06.0+flambda/lib/js_of_ocaml -I /home/fabian/.opam/4.06.0+flambda/lib/ppx_jsobject_conv -I /home/fabian/.opam/4.06.0+flambda/lib/result -I /home/fabian/.opam/4.06.0+flambda/lib/uchar -no-alias-deps -I . -o test.cmo -c -impl test.pp.ml)
File "test.ml", line 5, characters 8-25:
Error: Unbound value Stuff.of_jsobject

I believe jbuilder links all ppxs together with either ppx_driver or ocaml-migrate-parsetree, so it's necessary to register with one of the drivers (Migrate_parsetree_driver.register or Ppx_driver.register_*).

rgrinberg commented 6 years ago

I think it also makes sense to port the build system of this project to jbuilder. It will simplify things quite a bit.

little-arhat commented 6 years ago

so it's necessary to register with one of the drivers

sounds like quite strict requirement from a build tool

rgrinberg commented 6 years ago

This is no longer a strict requirement since we now have a workaround https://github.com/diml/ppxfind. However, it's still highly recommended to make your ppx available to driver users.

little-arhat commented 6 years ago

@rgrinberg is there a decent guide on how to do that? ppx_jsobject_conv use ppx_driver already (in standalone mode, though), and it's possible to use rewriter by simply adding package(ppx_jsobject_conv) to your ocamlbuild _tags. And it also works with other ppx rewriters (like ppx_deriving.show) w/o any problem.

copy commented 6 years ago

@little-arhat It should be sufficient to call Ppx_driver.register_transformation, like for example here: https://github.com/janestreet/ppx_here/blob/master/src/ppx_here.ml

rgrinberg commented 6 years ago

@little-arhat if you don't mind porting your package to jbuilder, I wrote a little tutorial here:

http://rgrinberg.com/posts/extension-points-3-years-later/

Most of it is irrelevant, but the packaging bit should be handy.

little-arhat commented 6 years ago

@copy thanks! I'm using ppx_type_conv, though, so in a way ppx_jsobject_conv is just a plugin to type_conv. Would be nice to use it, instead of defining rules myself.

@rgrinberg thanks, will have a look!

little-arhat commented 6 years ago

(also wonder, whether replacing Ppx_driver.standalone with Ppx_driver.register_transformation will keep this rewriter compatible with non-jbuilder users %) )

rgrinberg commented 6 years ago

(also wonder, whether replacing Ppx_driver.standalone with Ppx_driver.register_transformation will keep this rewriter compatible with non-jbuilder users %) )

If you port your build system to jbuilder, then jbuilder will generate a META that will be compatible for all users (driver users and classical ppx users).

little-arhat commented 6 years ago

@rgrinberg cool, migrating to jbuilder turned out to be simple after all %) I've pushed new version and will update opam-repo.

@copy your test works for me with ppx_jsobject_conv 0.5.0 (uses jbuilder).

little-arhat commented 6 years ago

Should be fine now