search

pdonadeo / ocaml-lens

Private extraction of astrada's lens library
56 stars 9 forks source link

Make ocaml-lens work with jbuilder #9

Closed fare closed 6 years ago

fare commented 6 years ago

Build it will jbuilder.

Use ppx_tools_versioned so it works with jbuilder preprocessing.

fare commented 6 years ago

NB: Please review carefully, since I have no idea what I'm doing.

fare commented 6 years ago

I admit I am incompetent as to the distinction between deriver and rewriter, and cannot comment. The code looks like it mostly works for me. However the existing test file fails, seemingly because of conflict between the [@@deriving lens] from a module and from its interface.

fare commented 6 years ago

As for testing... so far I have only tested with 4.06.1. I'm not too sure how to test with other versions.

rgrinberg commented 6 years ago

François-René Rideau notifications@github.com writes:

As for testing... so far I have only tested with 4.06.1. I'm not too sure how to test with other versions.

Have you considered using opam:

$ opam switch create 4.05.0

etc.

rgrinberg commented 6 years ago

François-René Rideau notifications@github.com writes:

I admit I am incompetent as to the distinction between deriver and rewriter, and cannot comment. The code looks like it mostly works for me. However the existing test file fails, seemingly because of conflict between the [@@deriving lens] from a module and from its interface.

The difference is in the META generated. A deriver needs some extra stuff to make it work with ppx_deriving's dynlink.

pdonadeo commented 6 years ago

First of all I want to apologise for the delay, I have very few spare time for side projects.

My problem in this context is that I don't use jbuilder so I don't know how it works.

The patch essentially adds the configuration files for jbuilder and proposes a different build system in the opam file.

Questions:

Otherwise we have to tackle this problem in a different way, maybe asking help to the jbuilder team.

fare commented 6 years ago

I didn't test with 4.03 or anything but 4.06.1: I just adapted the statement from some existing configuration file for jbuilder.

I'll try at least with 4.05 to see how it goes...

The lenses work for me in a private project. The given test file, on the other hand, doesn't work (see above).

pdonadeo commented 6 years ago

@fare I want to solve this issue of course but I need some help from you.

  1. in a 4.06.1 opam switch (I'll take care of other compiler versions) compile lens from sources, with your patch, and install it
  2. create a very minimal hello_world.ml using lens and a jbuild file to compile the hello_world with the library
  3. create a gist or whatever tool you like and send me the hello_world.ml and the jbuild, so I can reproduce the correct compilation, with your patch.

Once I can compile a simple program with your patch and jbuilder I'll try to compile the same source and lib with ocamlbuild, let's see what happen.

The point here is to avoid to compromise the build chain of all the existing projects using lens with a "jbuilder only" release.

fare commented 6 years ago

I added a hello example using lenses.

I failed to make [@@deriving lens] work with { submodule = true }. submodule = true also fails in the previously provided ppx_test. Is it because I use OCaml 4.06.1 versus earlier? Because I use Migrate_parsetree? Because of jbuilder itself? I don't know. I'd bet something incompatible changed about the AST for modules.

pdonadeo commented 6 years ago

I read a lot of threads here and there and I have to admit that... it's really a mess (ocaml-ppx/ppx_deriving#153).

The current state is:

  1. the "hello" example provided by @fare compiles with the OPAM package (1.2) using oasis/ocamlbuild/findlib.
  2. also it can be compiled in a very essential bash script, not using any build system.
  3. I wasn't able to compile and install with duna :disappointed:

There are two issue here: one "political" (against duna, Jane Street, etc.) and one technical: I don't know how duna works and why it doesn't find the installed library.

I want to be pragmatic and ideally provide an OPAM package for lens which:

  1. can be used in a duna environment, since it seems this build system is rapidly becoming the most adopded
  2. I don't want to break projects using lens

Here the only person that seems to understand the issue in deep is @rgrinberg: I read your comments here: ocaml-ppx/ppx_deriving#153

Do you know what is the present state of duna/ppx_deriving discussion? Do you think I (we) can fix this tiny library and make everybody happy?

There is at least one solution, but it's really ugly: to keep lens as it is and publish another OPAM package, lens-duna, compiling with this build system. I'd like to avoid such an abomination but as I said I'm a pragmatic person and I don't want to fight against Jane Street or any other in the OCaml community, for a library of just two files, defending a "principle".

@rgrinberg if you can help, please... If you can't or you don't care but you know someone who can give help, I'll appreciate very much.

fare commented 6 years ago

Wait. Was dune renamed to duna or was that a typo? In any case, I had no problem building with jbuilder as itself installed by opam install jbuilder in a 4.06.1 installation.

rgrinberg commented 6 years ago

Paolo Donadeo notifications@github.com writes:

Do you know what is the present state of duna/ppx_deriving discussion? Do you think I (we) can fix this tiny library and make everybody happy?

All of the problems have been fixed and ppx_deriving is compatible with drivers, omp, and type_conv. Though it might be required to drop 4.02 support (and possibly 4.03 as well).

There is at least one solution, but it's really ugly: to keep lens as it is and publish another OPAM package, lens-duna, compiling with this build system. I'd like to avoid such an abomination but as I said I'm a pragmatic person and I don't want to fight against Jane Street or any other in the OCaml community, for a library of just two files, defending a "principle".

@rgrinberg if you can help, please... If you can't or you don't care but you know someone who can give help, I'll appreciate very much.

I'll have a look at the port today. The time that I have to spend on this port is limited, because I don't really have time for this.

What I'd like to see is testing of this port with different versions of OCaml.

pdonadeo commented 6 years ago

@fare sorry, "dune" is a typo, maybe too much Herbert reading... Yes i confirm I can compile the lib with jbuilder but I don't know how to install it and then to compile the hello example linking using that installed library.

@rgrinberg thanks, good news. I'll investigate more on the jbuilder side. I believe it should be possible for an ocamlbuild-generated library (which is a f***ing regular object from the OCaml compiler) to be linked with the example, using jbuilder.

The problem is: why jbuilder don't even find the library?

rgrinberg commented 6 years ago

@rgrinberg thanks, good news. I'll investigate more on the jbuilder side. I believe it should be possible for an ocamlbuild-generated library (which is a f***ing regular object from the OCaml compiler) to be linked with the example, using jbuilder.

The issue is that dune expects a different packaging standard for ppx's. Jbuilder expects a ppx to register with a driver so that it can link all them all into a single preprocessor. So all you'd really need is to write a correct META file and use omp.

pdonadeo commented 6 years ago

@rgrinberg thanks, that was a good hint!

I'm on the right track and I have an almost-working local branch.

pdonadeo commented 6 years ago

@fare @rgrinberg I can confirm I can build lens and install it with OCaml >= 4.041. The installed library can be linked with the example provided by François-René (using jbuilder) and with an old project of mine still using ocamlbuild.

I'll release as soon as possible.

fare commented 6 years ago

On the other hand, the jbuilder recipe explicitly uses versioned parse trees (as required by jbuilder), with 4_06 being the version I used (because it's the version I tested), so it won't build with older versions. If you believe I ought to use an older version than 4.06, that's trivial to change and test.

pdonadeo commented 6 years ago

@fare my proposal is to test the new package in your environment.

  1. in your switch uninstall lens: opam uninstall lens
  2. clone the repo: git clone https://github.com/pdonadeo/ocaml-lens.git; cd ocaml-lens
  3. switch to the jbuilder branch: git checkout -b jbuilder origin/jbuilder
  4. install the local package: opam pin add lens .

In this environment I can compile your hello_lens with jbuilder runtest using this jbuild file:

(jbuild_version 1)

(executable
 ((name hello_lens)
  (modules (hello_lens))
  (libraries (lens))
  (preprocess (pps (lens.ppx_deriving)))))

(alias
 ((name runtest)
  (deps (hello_lens.exe))
  (action (run ${<}))))

I hope this also works for you, let me know.

pdonadeo commented 6 years ago

@fare meanwhile I can confirm that the new version of lens (compiled and installed with jbuilder) works with an old project of mine, still in production. It also works (at least here) with your example, can you confirm it also works with a more realistic project of yours?

pdonadeo commented 6 years ago

@rgrinberg since nobody seems interested anymore... Do you think you can make a quick check if the jbuilder branch works in a typical environment using dune?

If you can't I'll merge the branch and publish a new version.

fare commented 6 years ago

Yes, it works perfectly for me with jbuild on my open-source-but-not-released-yet project.

NB: the jbuild version explicitly requires the versioned ppx for OCaml 4.06, and so won't work with older versions of OCaml.

pdonadeo commented 6 years ago

Ok, very nice! This week end (sorry, its only monday...) I'll release.

Meanwhile you can opam pin the jbuilder branch.