vbmithr / ocaml-websocket

Websocket library for OCaml
ISC License
162 stars 44 forks source link

switch to jbuilder #87

Closed vbmithr closed 7 years ago

vbmithr commented 7 years ago

Could you please try @copy . This is a buildsystem change to jbuilder. I really like it and it compiles blazing fast compared to ocamlbuild.

This PR is extremely messy. Apart from the switch to async, it also contain code to remove the ppx extension of lwt that is not supported by jbuilder (and that we used very rarely anyway). I just changed code to make it compile, without (hopefully) changing any functionality.

rgrinberg commented 7 years ago

Also, it would be really nice to get rid of the depopts and have explicit packages opam subpackages for websocket, websocket-lwt, websocket-async.

How does the jbuilder generated META file look like btw? That might create some breaking changes for users.

vbmithr commented 7 years ago

@copy It does not seem to be possible to use the library namespace feature of jbuilder on modules that have different set of dependencies. Correct me if I'm wrong.

@rgrinberg Gonna try this soon!

rgrinberg commented 7 years ago

@vbmithr just in case you haven't seen this before, there's a little trick to generate META files that are backwards compatible. I got this from hhugo's port of jsoo. Check out the META.cohttp.template file in https://github.com/mirage/ocaml-cohttp/pull/533 for example

copy commented 7 years ago

It does not seem to be possible to use the library namespace feature of jbuilder on modules that have different set of dependencies. Correct me if I'm wrong.

That's fine then. May I suggest at least renaming the modules to Websocket_rng*, so that they don't clash with other libraries (#69)? We could also consider getting rid of the Rng modules, since they're just one-liners and the random_string parameter is pretty much self-explanatory.

vbmithr commented 7 years ago

This should be fine now, I have fixed things according to your recommendations. @copy Could you review it again?

copy commented 7 years ago

Looks good, thanks. I can't pin it though, I'm assuming I need to pin in the build directory since the opam file is there?

opam pin add _build/install/default/lib/websocket/

Results in this error:

Error: I don't know about package websocket (passed through --only-packages/--release)
vbmithr commented 7 years ago

On 23/04/2017 22:13, Fabian wrote:

Looks good, thanks. I can't pin it though, I'm assuming I need to pin in the build directory since the opam file is there?

No, you can pin it as usual, at the root of the project. I did not have any problems pinning this package.

copy commented 7 years ago

No, you can pin it as usual, at the root of the project. I did not have any problems pinning this package.

This works, but it doesn't install websocket-lwt, which is required for websocket.lwt:

ocamlfind: Package `websocket-lwt' not found - required by `websocket.lwt'
vbmithr commented 7 years ago

Yeah, you have to opam pin add websocket-lwt <ocaml-websocket-directory>

copy commented 7 years ago

Yeah, you have to opam pin add websocket-lwt

Got it, thanks. Everything's good.