Rewrite to use jbuilder, ocaml-migrate-parsetree and topkg

[andreas]

Hi @johnwhitington :)

I wanted to use ppx_blob with ocaml-graphql-server, but I was blocked by the fact that ppx_blob is not compatible with ocaml-migrate-parsetree. As such I began to rewrite the library to use ocaml-migrate-parsetree, and along the way switched to jbuilder and topkg. Functionally the library is almost the same, except it now looks for the included file relative to the file being compiled (jbuilder copies source files into a _build directory, so this is required). I also added a single test case with alcotest.

I would be happy to see this merged, but I know it's also some significant changes. Let me know what you think.

Thanks!

[johnwhitington]

Thanks for this.

I've had a quick look at the patch. The change to relative filenames is backwards-incompatible, isn't it, surely? Should we provide [%blob] and [%blob-relative]? Do we need to check everyone who uses ppx_blob (at least in OPAM) won't be affected?

As for the whole patch, I'd like to separate out ocaml-migrate-parsetree (which is obviously a good thing) from the other changes.

What are the new dependencies to build? Is topkg required to build, or just for making a new OPAM package?

Something as simple as ppx_blob can't really have any dependencies, other than ppx_tools and similar, so I don't think requiring jbuilder to build it is a good idea. jbuilder is very new, too. Something else may be fashionable next year.

Summary:

1. ocaml-migrate-parsetree: yes
2. topkg: probably-yes
3. alcotest: may-as-well-since-only-a-test-dependency
4. jbuilder: nope
5. relative-filenames: convince-me

[andreas]

Thanks for the quick reply!

The change to relative filenames is backwards-incompatible, isn't it, surely?

Yes, the change is backwards compatible to my knowledge.

What are the new dependencies to build? Is topkg required to build, or just for making a new OPAM package?

• ocaml-migrate-parsetree replaces ppx_tools.
• topkg is only required for making a new OPAM package.
• alcotest is only for tests.
• Personally, I would like to support the adoption of jbuilder. What build tool would you prefer?

[rgrinberg]

Note that omp depends on jbuilder already and jbuilder also drops the dependency on ocamlfind.

[johnwhitington]

Thanks @rginberg. Looks like I need to catch up on recent developments.

@andreas how would you feel about taking over ppx_blob? I wrote in an afternoon, just as an example of how ppx works, and after your patch is applied I won't understand most of it... I would be happy to assign it to you...

[emillon]

The extension node is [%blob], so I think that this errror message should be updated accordingly.

[andreas]

True, thanks!

[andreas]

Sorry I never followed up on this -- I ended up using ocaml-crunch instead. It's a tiny library though (and fairly quiet by the looks of the commit history), so I don't mind maintaining it.

[emillon]

Hi!

Thanks for taking care of this.

I noticed that depending on the list of preprocessors, this can fail.

Example:

let a = [%blob "a.txt"]
let b = [%blob "b.txt"]

let () =  Printf.printf "%S %S\n%!" a b

(jbuild_version 1)

(executable
 ((name test)
  (libraries ())
  (preprocess (pps (ppx_blob)))
  (preprocessor_deps (a.txt b.txt))
 )
)

The above works, but replacing (pps (ppx_blob)) by (pps (ppx_here ppx_blob)) fails with:

File "bin/test.ml", line 1, characters 10-14:
Error: Extension `blob' was not translated

On the other hand, this works with ppx_deriving.std (using the patched version that works with jbuilder). Maybe there's something to register on the ppx_driver side?

[johnwhitington]

@andreas Thanks for the offer. I can't transfer the repository to you yet, since you have an identically-named one (the fork). Can you rename it, so I can initiate the transfer?

[andreas]

@johnwhitington done

[johnwhitington]

Now it says "andreas already has a repository in the johnwhitington/ppx_blob network".

Shall we wait until this pull request is finished and merged, and then transfer it?

[rgrinberg]

Another options is to to add Andreas as a collaborator or administrator to your own repo. It would keep the url situation simpler for users and theres no reason why a package can't have more than 1 owner. Anyway its just a suggestion. Do as you wish with your own software of course :)

…

On Wed, Aug 16, 2017, 7:57 PM John Whitington ***@***.***> wrote: Now it says "andreas already has a repository in the johnwhitington/ppx_blob network". Shall we wait until this pull request is finished and merged, and then transfer it? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-wHyefKSXHSIoOUMHSPCNguFKbSDks5sYssigaJpZM4Olx2w> .

[johnwhitington]

Thanks @rgrinberg. Done.

[andreas]

@rgrinberg, good suggestion -- @johnwhitington thanks for adding me.

@emillon As far as I can tell, this is due to ppx_blob being OMP-based, while ppx_here is using ppx-driver. If I try your example with ppx_blob and ppx_cstruct (both OMP), then both ppx rewriters run properly.

[emillon]

OK, thanks. I guess I'll have to report that to ppx_driver. (similar problems happen when using the new OMP-based ppx_deriving).

[rgrinberg]

@andreas @johnwhitington what's the status of this PR? I see some users that would benefit from this transition ocaml/dune#285 (comment)

[andreas]

I've rebased on master now and included the -safe-string flag. I'll merge and release a new version on OPAM in the next couple of days unless someone objects.

[johnwhitington]

No objection here; thanks for the work.