Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to jbuilder #24

Merged
merged 3 commits into from
Jun 12, 2017
Merged

Port to jbuilder #24

merged 3 commits into from
Jun 12, 2017

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented May 23, 2017

Having removed camlp4 (PR #23), we can now switch to jbuilder. Some useful effects of this:

  • .merlin files are auto-generated, allowing editor auto-complete, etc.
  • You can develop capnp-ocaml together with a program or library using it by simply putting them both in the same jbuilder workspace, without having to opam reinstall (full rebuild) on each change.
  • 320 lines of OMakefile are replaced by 102 lines of jbuild files.
  • Built files go under _build, without polluting the source directories.

Compilation time remains about the same (17s with omake vs 16s with jbuilder).

I also copied in the schema files, rather than cloning the entire capnproto repository. This is faster, and prevents the build from breaking when the temporary directory is cleaned.

The new tests/.capnp schema files were imported from https://github.com/kentonv/capnproto.git
commit 25509cf4f1fe6b3f5a6faa2f42b865f7571db8c4.

The new benchmark/.capnp schema files were imported from https://github.com/kentonv/capnproto.git
commit f536c8cfbc749f98e43b679242b24afbab8e74b2.

These were the revisions that the old omake build used.

I also added a Makefile that provides a quick way to build, run the tests, etc, for people not familiar with jbuilder.

I disabled all jbuilder warnings that affected the existing code. I'll fix the underlying problems in a future PR.

@talex5 talex5 force-pushed the jbuilder branch 2 times, most recently from 7832dbf to f1cb585 Compare May 23, 2017 10:27
@talex5
Copy link
Collaborator Author

talex5 commented May 23, 2017

(needs a solution to ocaml/dune#90 if we want to build on OCaml 4.02 as well, since that doesn't support -O3)

capnp.opam Outdated
depends: [
"omake"
"jbuilder" {build}
"ocamlfind" {>= "1.5.1"}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's no longer necessary to have an ocamlfind dependency once you switch to jbuilder, as it pulls it in if it needs it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@avsm avsm mentioned this pull request Jun 6, 2017
@talex5 talex5 force-pushed the jbuilder branch 2 times, most recently from 071f625 to 74b1603 Compare June 8, 2017 13:33
@avsm
Copy link
Collaborator

avsm commented Jun 9, 2017

It is reasonable to have an OCaml>=4.03.0 restriction -- many of the MirageOS libraries have this restriction now for example. I don't think it's worth supporting more than two OCaml revisions for a new library (in this case, OCaml 4.04.x and 4.03.0)

@talex5
Copy link
Collaborator Author

talex5 commented Jun 9, 2017

Using jbuilder also currently breaks inlining on all compiler versions, due to https://caml.inria.fr/mantis/view.php?id=7538.

@avsm
Copy link
Collaborator

avsm commented Jun 9, 2017

can we turn off wrapped mode to avoid module aliases for now?

@talex5
Copy link
Collaborator Author

talex5 commented Jun 9, 2017

Not without breaking the API, as it used to be a pack (and I don't think jbuilder supports that).
omake isn't a big problem for now, though.

@avsm
Copy link
Collaborator

avsm commented Jun 9, 2017

Not without breaking the API, as it used to be a pack

I'm suggesting that we do a major version bump to the API and align it with modern OCaml standards, which includes:

  • remove the use of camlp4 (done)
  • eliminate the use of pack
  • replace pack with module aliases
  • adopt jbuilder and deprecate omake

it sounds like there is a bug that will delay the use of inlining if we adopt module aliases, which means there are two options:

  • adopt all the modern conventions, and wait for the inlined-functor fix in ocaml 4.06 (hopefully): so a delay of 6 months or so for the performance improvement
  • remove pack/omake and pollute the global namespace a little more with Capnp_* modules, but eventually wrap them up into a module alias.

I think that moving towards the modernity goal is an important one; does inlining really give us that much benefit in the short term that we can't wait a few months for an upstream OCaml fix for the functor inlining issue?

@talex5
Copy link
Collaborator Author

talex5 commented Jun 9, 2017

Personally I don't care much about speed, but I'd feel pretty bad about taking over a high-performance RPC library and almost immediately hugely reducing its performance (it was fast before using camlp4 includes; removing those without enabling flambda means it would be slower than before).

The old pack API is exactly the same as the new jbuilder alias one, so using jbuilder now would mean breaking the API twice.

@talex5
Copy link
Collaborator Author

talex5 commented Jun 9, 2017

It may be possible to make it work with this PR: ocaml/ocaml#1183
I can run some tests and find out.

Thomas Leonard added 2 commits June 12, 2017 10:08
Effects:

- `.merlin` files are auto-generated, allowing editor auto-complete, etc.
- You can develop capnp-ocaml together with a program or library using
  it by simply putting them both in the same jbuilder workspace, without
  having to `opam reinstall` (full rebuild) on each change.
- 320 lines of `OMakefile` are replaced by 102 lines of `jbuild` files.
- Built files go under `_build`, without polluting the source
  directories.

I also copied in the schema files, rather than cloning the entire
capnproto repository. This is much faster, and prevents the build from
breaking when the temporary directory is cleaned.

The new tests/.capnp schema files were imported from https://github.com/kentonv/capnproto.git
commit 25509cf4f1fe6b3f5a6faa2f42b865f7571db8c4.

The new benchmark/.capnp schema files were imported from https://github.com/kentonv/capnproto.git
commit f536c8cfbc749f98e43b679242b24afbab8e74b2.

These were the revisions that the old omake build used.

I also added a `Makefile` that provides a quick way to build, run the
tests, etc, for people not familiar with jbuilder.
@talex5
Copy link
Collaborator Author

talex5 commented Jun 12, 2017

I've separated this out from the RPC branch. This means that the two-arg functor isn't needed for now, and so performance is still fine. I also used the OCaml-escape in jbuilder to support 4.02.

@talex5 talex5 merged commit 6127999 into capnproto:master Jun 12, 2017
@talex5 talex5 deleted the jbuilder branch June 12, 2017 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants