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

understanding the current mess #153

Closed
gasche opened this issue Oct 14, 2017 · 49 comments
Closed

understanding the current mess #153

gasche opened this issue Oct 14, 2017 · 49 comments

Comments

@gasche
Copy link
Contributor

gasche commented Oct 14, 2017

Hi all,

(cc @let-def, @trefis, @bikalgurung, @avsm, @xclerc, @alainfrisch )

It seems that the current state of ppx-deriving combined with the rest of the ppx ecosystem is fairly unsatisfying for now, as this repository is full of users for which the tool does not work any more: #143, #147, #148, #149, #150 are all issues that arose since the last release of ppx-deriving (4.2) with ocaml-migrate-parsetree (omp for short) integration. (Some deriving plugins are also affected, see ocaml-ppx/ppx_deriving_yojson#56 , ocaml-ppx/ppx_deriving_yojson#63. )

This integration was not painless:

  • the previous ppx_deriving maintainer, @whitequark, has distanced themselves from maintenance of the library for many reasons and is not currently able to work more on improving the library (that's perfectly understandable and we should be able to get it to move forward on our own)
  • @let-def, who created omp, proposed a first PR to use it for ppx-deriving (to get 4.05 compatibility "for free") as [WIP] Port to ocaml-migrate-parsetree & OCaml 4.05 #123, which required a change in the way ppx_deriving is invoked
  • @diml (who is currently in holidays and should thus not be expected to work on this issue) later proposed to revert this change and propose a "more compatible" approach, Revert #123 + omp driver integration #141, which was eventually merged and is what remains in the 4.2 release.

I am not a heavy user of ppx-deriving myself, and far from an expert on ppx matters. My understanding of the issue is therefore almost non-existent, and I would be happy if other people took care of the issue for the 4.05 and 4.06 release cycles. However, progress has been slow to nonexistent so far, so I'm creating this issue to try to at least clarify the situation and understand what the problem is.

I have two separate questions:

  1. There are plenty of examples of the fact that ppx-deriving currently does not work for many people. But who are the people for which it works? What are their development environment like? Does the current version of ppx-deriving work as expected for all jbuilder users, for example? Does it work for all people inside Jane Street (possibly using non-publicly-released software versions), but not outside it?

  2. Is there a simple and workable way to get ppx-deriving back to "work as expected for OCaml 4.05 and 4.06, even if it is not the best long-term way to do things"? For example, would reverting the use of ocaml-migrate-parsetree and going back to cppo IFDEFS solve the problem? (Would it work for some of the current or prospective users of ppx-deriving, but not the others?).

Finally, I am not trying to blame, but I cannot help but notice that we seem to have a bus-factor problem with the ppx driver technology. It is very nice when it works, but it seems that no one else than @diml is able to understand the various pieces of the ecosystem enough to fix the current problem (I know for example that @let-def spent some time working on this problem and was not able to figure it out alone). This is not necessarily a big deal, some other parts of the ecosystem have this property (not many people understand the internals of Camlp4's dynamic parser composition algorithm), but it got me to rethink my understanding of the general stance of the jbuilder community of deciding, by choice, not to play nice with non-driverized ppx extensions. This may be a good idea in the absolute (standardize and improve the ppx-extension ecosystem), but right now a consequence is that it forces people to adopt a technology that nobody understands, and creates a shift/split in the cases where it does not magically work. Would it be easier to get out of the current mess, and have a transition that does not break tools for many people, if this requirement was relaxed, that is if jbuilder accepted to play nice with non-driverized extensions during a transition period?

@xclerc
Copy link

xclerc commented Oct 14, 2017

Hi Gabriel,

I do not have yet a complete understanding of the situation, but I don't think
that going back to cppo is a good idea (for instance, the interaction with
Merlin is less than ideal). I have started to prepare patches to switch the
various repositories from the ocaml-ppx organization to jbuiler+omp, but
it is true that these are breaking changes. For example, when switching to
omp drivers (arguably a step forward) you have to change the way deriving
analyzes the command-line.

I understand this is an issue, but jbuilder and omp seems to pave the way
to future-proof implementations at the price of breaking the backward
compatibility at one point (I am aware of the irony...). My knowledge of
opam is limited, but would the following plan make sense?

  • restore the previous state (i.e. 4.1) of deriving with a constraint on the
    OCaml version such that <= 4.05;
  • accept breaking changes for the upcoming version with a constraint
    such as >= 4.06;
  • use the time until the official release of 4.06 to advertise the breaking
    changes.

My feeling is that the breaking changes we are talking about are mitigated
by the visible benefits provided by jbuilder and the less visible benefits
provided by omp. Combining several ppx rewriters with ocamlbuild is
quite complicated even if efficiency is not a concern, while the use case is
efficeintly supported by jbuilder out of the box.

@gasche
Copy link
Contributor Author

gasche commented Oct 14, 2017

I don't think that going back to cppo is a good idea (for instance, the interaction with Merlin is less than ideal)

Using cppo is clearly not ideal (and I am looking forward for a better way that works), but if it lets us easily go from a current state that does not work for most users to a state that works for most users, I'm happy to do the conditional-directive-introduction work myself. The difficulties to combine Merlin and cppo do not affect users of ppx-deriving, so I don't think it matters that much.

My feeling is that the breaking changes we are talking about are mitigated
by the visible benefits provided by jbuilder and the less visible benefits
provided by omp. Combining several ppx rewriters with ocamlbuild is
quite complicated even if efficiency is not a concern, while the use case is
efficiently supported by jbuilder out of the box.

The migration plan you propose, if I understand correctly, is to migrate ppx-extension codebases to use jbuilder. How would that affect the ability of ppx users to combine ppx rewriters? My suspicion is that the fact that nobody understand ppx drivers enough to suggest another solution than "let's rewrite everything using jbuilder and maybe things will be better" is part of the problem.

Besides, it seems easy enough to combine several ppx rewriters with ocamlbuild. I just prepared the following repro case to check:

[%%cstruct
type pcap_header = {
  magic_number: uint32_t;   (* magic number *)
  version_major: uint16_t;  (* major version number *)
  version_minor: uint16_t;  (* minor version number *)
  thiszone: uint32_t;       (* GMT to local correction *)
  sigfigs: uint32_t;        (* accuracy of timestamps *)
  snaplen: uint32_t;        (* max length of captured packets, in octets *)
  network: uint32_t;        (* data link type *)
} [@@little_endian]]

let here = [%here]
ocamlbuild -use-ocamlfind -tag 'package(ppx_cstruct)' -tag 'package(ppx_here)' test.inferred.mli

I have no problem with people migrating to jbuilder (as far as I can tell it's nicer than ocamlbuild in many respects) if they like, and it is not my intent here to criticize the tool (or its authors or users). I was asking about it because I have the impression that some of the changes that got us into the current mostly-broken state are related to design decisions in jbuilder that it may be possible to relax temporarily (see this message that suggests that it is possible to support old-style derivers in a jbuilder project). If that were a possible path, it may require less work on everyone's part than some other solutions, so it should also be discussed.

@hcarty
Copy link

hcarty commented Oct 14, 2017

When I've hacked on ppx_deriving and related derivers with the cppo codebase I found it very difficult to navigate due to my lack of understanding around the differences in ppx-related API differences. merlin not working with the codebase amplified this problem, leaving me in something of a trial and error approach.

I bring this up because, while I was able to create a new deriver at the time, it was a rather painful process. That pain was largely due to my own ignorance of the APIs involved, but the level of detailed understanding required to work with the cppo-based code highlights the bus factor @gasche mentioned.

Staying in an omp world makes the code much easier to understand, even if not easy, just by having only one set of code to understand while reading a file. I agree with @xclerc that staying with omp and breaking some compatibility will benefit us more as a community going forward.

@gasche
Copy link
Contributor Author

gasche commented Oct 14, 2017

If we decide to "break some compatibility" to be able to use ocaml-migrate-parsetree, then what is the right way to do this? Is the current state of ppx-deriving doing things correctly in this respect? If not, which of the error reports of the users above are issues to be fixed, and which are inevitable consequences of this decision to break compatibility? What is the recommended action for users to get their ppx usage working again?

@xclerc
Copy link

xclerc commented Oct 15, 2017

Regarding the switch to jbuilder and and the difficulties with ocamlbuild,
I meant if you have to apply rewriters in a specific order to get the code
you need.

which of the error reports of the users above are issues to be fixed

Most of the issues you referenced can be solved by fixing the interactions between
deriving and type_conv/ppx_core interactions. However, I linked them to jbuilder/omp
because it seems much easier to treat the problems if you have clean foundations
and proper tooling.

and which are inevitable consequences of this decision to break compatibility?

For instance the way to invoke deriving, instead of passing the list of derivers on the
command-line directly, one would have to go through switches. This is the way omp
drivers work, and it allows you combine several rewriters in the same process.

@ejgallego
Copy link

@gasche ocaml-ppx/ppx_import#17 was also claimed to be due to this.

@rgrinberg
Copy link
Contributor

First of all, I'd like to say that I'm really grateful for the creation ppx_deriving and I've gotten a ton of mileage out of it over the years. But perhaps we should rethink some fundamental assumptions. Like if our man power is low, what are the reasons to maintain the ppx_deriving parallel universe in the first place? Are there any unique features that ppx_deriving offers over ppx_type_conv?

For such a small community we are surely spreading ourselves thin here with all these build systems and ppx frameworks.

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

My worry is that if there is something basic in the ppx ecosystem that no one understands, it is going to be a problem anyway.

@rgrinberg
Copy link
Contributor

That's not an independent issue. Part of the problem is that there's simply too much overlapping stuff out there. To comment about some of the concrete issues you've brought up:

  • The issue with the -no-check thing is unfortunate and there's no good workaround for it yet. The spellchecking feature is very important, and without it you can publish packages with some silly typos. I wish that ppx_deriving would just use ppx_core to avoid this issue altogether.

  • Many of the breakages are due to the fact that people maintain META files manually and use fragile build systems. We have frequent breakages along these lines without omp either. It's particularly unfortunate here as there's really no build infrastructure to sheild ppx_deriving plugins from this churn when jbuilder offers this support for the type_conv side of things.

  • Some issues are caused by the fact that some plugins are incompatible with 4.05 because they don't use omp and rely on ifdefs instead! I can't see how getting rid of omp could possibly help these issues.

@ejgallego you should reconsider your use of ppx_import because of the issues pointed out here: ocaml/dune#193.

@ejgallego
Copy link

@ejgallego you should reconsider your use of ppx_import because of the issues pointed out here: ocaml/dune#193.

So what is the proposed path forward then? Should I abandon my software that works very well with ppx_import? Sorry but that sounds really bad.

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

The ppx_core approach may be technically superior and may win out in the long run. However, I don't think that breaking people's tools as a way to force them to migrate to a completely different stack is a reasonable approach to change management. It is perfectly fine to encourage people to move to ppx_type_conv, or to try to incrementally evolve ppx_deriving (possibly with well-justified compatibility breakages; but then you need to provide clear instructions on how people should adapt their code).

In the current state of things, no one has been able or willing to answer this very basic question:

There are plenty of examples of the fact that ppx-deriving currently does not work for many people. But who are the people for which it works? What are their development environment like? Does the current version of ppx-deriving work as expected for all jbuilder users, for example? Does it work for all people inside Jane Street (possibly using non-publicly-released software versions), but not outside it?

Or this also fairly important question:

If we decide to "break some compatibility" to be able to use ocaml-migrate-parsetree, then what is the right way to do this? Is the current state of ppx-deriving doing things correctly in this respect? If not, which of the error reports of the users above are issues to be fixed, and which are inevitable consequences of this decision to break compatibility? What is the recommended action for users to get their ppx usage working again?

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

To me, today, it seems that the right way forward to help ppx_deriving users is to revert the ocaml-migrate-parsetree work, and add ifdefs for 4.05 and 4.06. This is simple, I can easily do this work, and it will let people that wrote code under 4.04 or older keep compiling it just fine under future OCaml versions -- which is what they expected and what they deserve. This approach does have drawbacks (those that @hcarty mentioned), and maybe it would be possible to re-introduce partial use of ocaml-migrate-parsetree to mitigate some of them. But at least it is simple and it works.

If someone has a better way forward to propose, I'm all ears, but it should have one of those two properties:

  1. It makes people's code work as before, or
  2. There are simple, clear, instructions on the minimal changes necessary to port their code

@rgrinberg
Copy link
Contributor

I have no opinion on whether ppx_deriving should get rid of omp but I think that your approach is basically deprecating ppx_deriving without announcing it publicly to end users. First of all, you'll be making the plugin effectively unavailable to jbuilder users which are now a huge chunk of users (the majority?). And second of all, my memories of the old cppo hell must be not be as fond as yours. I remember issues like this with ppx_deriving on almost every upgrade of the compiler. I think that any informed user will simply avoid ppx_deriving altogether if they don't want to commit a huge chunk of their time to this churn.

Maybe this is for the best after all?

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

You seem to think that ppx_deriving currently works correctly for jbuilder users. Have you tested this, can you confirm? Does it work under all OCaml versions? I asked this as part of my first question, but nobody answered it so far.

I think that your approach is basically deprecating ppx_deriving without announcing it publicly to end users

I'm proposing to fix a long-stanging breakage by the only way anyone here has proposed. You are proposing to ignore the issue instead and tell users that their code is going to stay broken forever and that they should just use a different library. I don't see how you conclude that my approach is worse for ppx_deriving or its users.

Again, if anyone has an idea for how to either (1) fix the issue or (2) tell users what they need to change to un-break their code, I am all ears. I never asked for having to work on this problem, and I would glad to let someone else handle it.

First of all, you'll be making the plugin effectively unavailable to jbuilder users which are now a huge chunk of users (the majority?)

If the current style works for jbuilder users, but not the others, would it be possible to maintain two separate versions as long as we haven't been able to bridge the gap?

I remember issues like this with ppx_deriving on almost every upgrade of the compiler.

With the ifdef appraoch, the only thing to do is to add more ifdefs on each release, before the release. Again, this is simple work that I understand how to do and am happy to do as long as the issue of "how to use omp without fucking everything up" is not resolved. (sexplib is currently broken under 4.06, and yet we don't try to tell all its users to migrate away from it.)

I remember issues like this with ppx_deriving on almost every upgrade of the compiler.

You are talking about ppx_deriving taking a couple weeks to update after an OCaml release -- and this can be easily avoided by investing a bit more work into it before the release. As far as I can tell, ppx_deriving has been completely broken since the 4.05 release -- isn't this much worse?

@rgrinberg
Copy link
Contributor

So what is the proposed path forward then? Should I abandon my software that works very well with ppx_import? Sorry but that sounds really bad.

I didn't mean to trivialize the issue. But I just don't see how ppx_import in its current form can exist in a frictionless ppx world. It doesn't use omp so you will possibly have to update it on every new version of the compiler. Additionally, it contradicts the view that preprocessing and compiling are done at 2 separate stages in your build pipeline. This is an important assumption for fast builds. It's up to you to decide how important that is to you.

By the way, why would abandoning your software is an option? While ppx_import is convenient, you can certainly find work arounds. See ocaml/dune#193 (comment)

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

I think that some of the disconnect in the discussion comes from the sort of user model one has in mind. We are fortunate in the OCaml community to have many people interested in actively maintaining their project, and adopting new libraries, tools or technology when it improves over the statu quo. For example, everyone using jbuilder today is in this category. This is very good, and making sure that life is good for these people is important.

But we also have OCaml users that don't want to update their code every OCaml release, or even every two OCaml releases -- yet they expect their codebase to work under future OCaml releases. Think of the research project that was written two years ago, has been sleeping since, and whose development is restarting today because a new intern is coming to work on it. The new intern expects to use the latest OCaml version (or is given this advice by the community), but also the libraries that the project was built against -- maybe they have to add a version bound since the library had incompatible release since then.

The people in the second category also deserve support.

@rgrinberg
Copy link
Contributor

rgrinberg commented Oct 23, 2017

You seem to think that ppx_deriving currently works correctly for jbuilder users. Have you tested this, can you confirm? Does it work under all OCaml versions? I asked this as part of my first question, but nobody answered it so far.

I'm not the best person to ask as I've stopped using ppx_deriving for much of the reasons discussed in this thread. But yes, ppx_deriving and its core suite of plugins seem to work well for me on jbuilder and 4.05.0. I haven't tested it on any other versions of OCaml. Others are free to add more data points here.

I don't see how you conclude that my approach is worse for ppx_deriving or its users.

I didn't conclude it's worse. I only pointed out that it cast some serious doubt about the long term future of the project. Maybe it's still the best course of action. And if in fact it can be recommended at all to users in your 2nd category, who want an easy upgrade path to newer versions of OCaml. I'd also like to support this group of users by the way, but as it stands today those who have adopted pre-omp ppx, have adopted a technology with a lot of rough edges when it comes to upgrading and packaging. This leads me to believe that there's no way out for them out of this mess (that keeps them out of it in the medium term!) without changing their code.

Again, if anyone has an idea for how to either (1) fix the issue or (2) tell users what they need to change to un-break their code, I am all ears. I never asked for having to work on this problem, and I would glad to let someone else handle it.

I think what remains is to simply adapt all 3rd party ppx_deriving plugins to omp? There might be some bugs remaining here. In particular, how well does ppx_deriving work in conjunction with ppx_type_conv? There are some bugs in this area as mentioned in your linked tickets.

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

I only pointed out that it cast some serious doubt about the long term future of the project.

I agree that, long-term, using omp is a better technical choice for the project. I am only proposing to use ifdefs as a temporary solution, as long as the current mess is not resolved in a long-term way. For example, we could make a 4.1.1 release that only adds support OCaml 4.05 and 4.06. I don't see how that would be worse than the current situation.

those who have adopted pre-omp ppx, have adopted a technology with a lot of rough edges when it comes to upgrading and packaging. This leads me to believe that there's no way out for them out of this mess (that keeps them out of it in the medium term!) without changing their code.

That sounds like a reasonable opinion, but we need people that understand what is the proper way to change their code, can clearly explain it, and volunteer to help users out -- by giving good answers in the opened tickets, for example. So far, no one has offered to do this. If no one with domain knowledge is stepping up, we have to do thing in simple ways that don't require this domain knowledge.

@trefis
Copy link

trefis commented Oct 23, 2017

In the current state of things, no one has been able or willing to answer this very basic question:

There are plenty of examples of the fact that ppx-deriving currently does not work for many people. But who are the people for which it works? What are their development environment like? Does the current version of ppx-deriving work as expected for all jbuilder users, for example? Does it work for all people inside Jane Street (possibly using non-publicly-released software versions), but not outside it?

JaneStreet doesn't use ppx_deriving internally, it never has.
I know @diml invested a fair amount of time in making sure that, externally, people could use our ppxs, and type_conv, with ppx_deriving (and ocamlbuild).
But that was before omp and jbuilder, I'm not sure we still have anything of the sort.

Hope that helps.

@whitequark
Copy link
Collaborator

whitequark commented Oct 23, 2017

[edit--this comment used to be much more strongly worded, in ways that I no longer think are fair--wq]

After returning from eight months of involuntary leave due to undiagnosed bipolar II disorder, I find that the latest Jane Street packages are incompatible with ppx_deriving, and make it essentially impossible to use ppx_deriving with jbuilder or ppx_type_conv. As the maintainer of ppx_deriving and many other ppx packages I find this behavior quite hostile.

@rgrinberg
Copy link
Contributor

rgrinberg commented Oct 23, 2017 via email

@whitequark
Copy link
Collaborator

whitequark commented Oct 23, 2017

I am willing to rethink my
approach, but it seems like what you have in mind is the sort of loyalty
that only football fans have for their favorite clubs.

When I started with OCaml with 4.02 just coming out, there wasn't ppx_anything [edit--there was Alain's ppx_tools, of course--wq]. I wrote guides for getting starting with it for the sole reason that someone asked me on IRC, ppx_deriving, ppx_import, and others, for me and anyone else to use.

When OCaml updated to 4.03 and 4.04, I've kept updating it, for (a bit) me and (mostly) anyone else to use.

When OCaml had absolutely nothing as a cross-compilation story, I fixed the compiler, I fixed the build systems and packagers, and I wrote the Android, iOS and Windows repositories, for me and anyone else to use. (Still updating them, by the way.)

By "abandoned" I do not mean "stopped using" or "stopped recommending", that would have been perfectly fine. By "abandoned" I meant "sit quietly as the corporation we all know about pushed yet another pointless not-invented-here non-solution over everyone's heads, and pretended this is Actually Good".

In retrospect I should probably just never have touched OCaml.

@whitequark
Copy link
Collaborator

Or to paraphrase the above: maybe the reason you have one capable maintainer per two similar projects is that Jane Street is actively (by using a closed, opaque development and release process, and ignoring most of the community feedback) driving away the ones it does not employ.

@Drup
Copy link
Contributor

Drup commented Oct 23, 2017

@rgrinberg I don't think you realize how hostile janestreet behaviour is towards libraries maintainers, ppx maintainers in particular. This is a general problem with their attitude towards FOSS, but before, it was just confined to users of their libraries and didn't affect the rest of the ecosystem much. With jbuilder, it affects all the maintainers of ppx libraries that have users using jbuilder, aka, all of them.

The problem can be summarized as follow: jbuilder doesn't support non-driverised ppxs. This has numerous consequences, from incompatibility with old ppxs to impossibility to support ppxs that uses build-time information such as ppx_import. jbuilder maintainers specifically refuse to provide this. Yes, there are good arguments for that, but they don't justify the complete lack of support.

Unfortunately, jbuilder also does not support numerous other features (that are not of interest to janestreet). Furthermore, switching to driverised-ppx is non-trivial (as the situation with ppx_deriving demonstrates, but there are other reasons). The result is that you get an extremely maintainer-hostile environment where jbuilder forces maintainers to adopt unfavourable technical decisions now instead of just supporting non-driverised ppxs and the ability to have some gradual migration.

omp's ppx-driver is clearly the future, and jbuilder does provide numerous advantages. Janestreet people just have to be less of a pain about how better their technical solution is to everyone else and allow for a smooth transition period, instead of strong-arming people.

@rgrinberg
Copy link
Contributor

rgrinberg commented Oct 23, 2017

jbuilder doesn't support non-driverised ppxs

And it's impossible to write omp based ppx's without jbuilder because oasis and friends lack support, am I right? While that's an issue, let's keep in mind that omp doesn't have any special support or treatment for jbuilder. The way omp support appeared in jbuilder is simply because diml put some work to make it happen. I don't see how jbuilder is privileged by omp in any way here. Furthermore, the classical build tools already contain support for creating drivers using an ocamlbuild plugin. The only thing that remains is generating proper META files in oasis. I'm just not convinced that adding proper, omp aware, META generation to oasis is so much harder than adding classical ppx support to jbuilder.

But I think your point is not completely relevant here. ppx_deriving already uses omp (despite it being hard without jbuilder) and is fully usable by jbuilder users. The issues that remains is that some external derivers haven't moved to omp yet and that there might be some ppx_type_conv/ppx_deriving bugs remaining.

Unfortunately, jbuilder also does not support numerous other features (that are not of interest to janestreet)

I'm curious what those are apart from the classical ppx issue. There seems to be plenty of features in jbuilder that are of little interest to janestreet.

@Drup
Copy link
Contributor

Drup commented Oct 23, 2017

But I think your point is not completely relevant here. ppx_deriving already uses omp (despite it being hard without jbuilder) and is fully usable by jbuilder users

It is ? I tried a few month ago and couldn't manage to do it.

In any case, I though that the problem addressed by this discussion was precisely that (forced) migration to omp made everything very complicated.

@rgrinberg
Copy link
Contributor

From my own setup on 4.05:

[rgrinberg:~/tmp/jbuilder-tests/ppx-deriving-test] % cat jbuild test.ml
(jbuild_version 1)

(executable
 ((name test)
  (preprocess (pps (ppx_deriving.show ppx_sexp_conv)))))

open Sexplib.Conv

type x = { f: int; g: string * string } [@@deriving show, sexp]

let () = print_endline (show_x { f = 123; g = "test", "me" })
[rgrinberg:~/tmp/jbuilder-tests/ppx-deriving-test] % jbuilder build test.exe
    ocamlopt .ppx/ppx_deriving.show+ppx_sexp_conv/ppx.exe
         ppx test.pp.ml
    ocamldep test.depends.ocamldep-output
      ocamlc test.{cmi,cmo,cmt}
    ocamlopt test.{cmx,o}
    ocamlopt test.exe
[rgrinberg:~/tmp/jbuilder-tests/ppx-deriving-test] % ./_build/default/test.exe
{ test.ml.f = 123; g = ("test", "me") }
[rgrinberg:~/tmp/jbuilder-tests/ppx-deriving-test] %

Seems to work with a ppx_deriving and ppx_type_conv deriver for this simple case.

@avsm
Copy link
Contributor

avsm commented Oct 23, 2017

Dear all, my apologies for taking so long to come in on this thread. The road to ppx has been a bumpy, multi-year migration away from camlp4, and the issues raised here are all solvable with level-headed communication.

The original charter of creating this ocaml-ppx organisation a few months ago was to have a neutral place where all the valid approaches of ppx could be placed, and then to document the state of the art. There are a significant number of users who are using the pre-ppx_driver mechanisms, and the first clear-cut decision we should make is to support them as best we can. Even when there is a valid emerging alternative, maintaining compatibility for source-level rewriters is important, to give users the ability to migrate towards it across a year or two.

We have also learnt that it takes time and scale (both in terms of one large codebase, and many small libraries) to figure out what the right approach is in the longer term. Jbuilder is a young, promising build system, but may yet have breaking changes imposed as it settles down. Therefore it is important to have the existing, stable mechanism continue to work in a build-system agnostic way before we charge forwards with a new system.

When there is a disagreement about resourcing and direction, then the core OCaml development team (e.g. @gasche at INRIA and myself at Cambridge University) stand ready to try to resolve the immediate issue and help provide support when one of the complex pieces that we build our tools on inevitably has a short term blip. This includes questions such as resourcing and finding maintainership help, since it is unreasonable to expect every individual to maintain an ecosystem as complex as ppx by themselves.

Regarding the more emotive comments, I would request that we all show each other a good dose of respect in our conversations. The developments in the OCaml tooling over the past fifteen years have been a pleasure for me to work with due to the talented individual maintainers that have poured sustained hard work into it. Most of us have done so across multiple jobs in industry and academia and multiple use-cases, and feel individual responsibility for our components. The ppx ecosystem is no exception, with valuable contributions from @whitequark, @diml, @let-def, @xclerc, @lpw25, @trefis, @rgrinberg, @alainfrisch and many more over the years. There is no question about the value of each your contributions. As with many problems in PL tooling, if we had known how complex the camlp4 migration would be when we started in 2012, we probably would have balked at the thought of starting.

But here we are now in 2017, and we are almost there :-) Therefore, moving forward I propose that we:

  • revert the omp migration and switch to cppo. This will restore expected functionality to the ppx_deriving.4.0 state, and unbreak users immediately.
  • rename this issue to "reverting omp migration" to give it a concrete resolution.
  • open up a jbuilder feature request for the maintainers to consider a non-ppx-driver alternative for extensions. It is clear from this discussion that there is real need for it, if at a minimum for the purposes of smooth migration.
  • invest time in CI scripts that do continuous testing on trunk compiler versions to help us spot regressions across a set of build systems (e.g. ocamlbuild, make, jbuilder, omake and oasis). There are quite a few permutations, all used in the wild, that will need support until a unifying answer is found.

@Drup
Copy link
Contributor

Drup commented Oct 23, 2017

open up a jbuilder feature request for the maintainers to consider a non-ppx-driver alternative for extensions. It is clear from this discussion that there is real need for it, if at a minimum for the purposes of smooth migration.

While this is all great, I had a discussion on that precise subject with @diml and @yminsky where I explained these points in, I hope, a clear and civilized fashion, and the answer was a non-negotiable no. So please, do weight in. :)

@gasche gasche closed this as completed Oct 23, 2017
@avsm
Copy link
Contributor

avsm commented Oct 23, 2017

@Drup

While this is all great, I had a discussion on that precise subject with @diml and @yminsky where I explained these points in, I hope, a clear and civilized fashion, and the answer was a non-negotiable no.

Please remember that ppx_driver, ocaml-migrate-parsetree, and Jbuilder itself are all new, fast-moving efforts that are also coping with a very new fast rate of OCaml compiler releases in the past few years. It could well be that there is a misunderstanding between a feature that has been implemented or in the pipeline, and how it interacts with the rest of the library world. I haven't seen the specific discussion that you are referencing, and cannot offer any more comment beyond that.

More broadly, maintainership of open source tools often involves saying no to new features, and then changing your mind when circumstances around the decision change. Jbuilder has recently expanded its maintainership team since @diml has taken a well-earnt vacation, and so there is ample scope to bring up the question properly over there. As a long-time contributor to OCaml, I am sure you are aware of the fine tradition of saying "no by default" to new features in the language -- the same applies to tooling :-)

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

I think that I learned from the discussions in this thread as much as I could reasonably hope to learn while opening the topic, which is:

  1. The matter of managing the needs of both driverized-ppx and non-driverized-ppx users is delicate, and no one is willing or able to step up to resolve it shortly. We need to think more about how to increase the bus factor there, and a smooth transition plan, but I can't expect a solution by the 4.06 release time.

  2. Making a minor maintenance release of the pre-omp ppx-deriving codebase that supports new OCaml version is probably the easiest short-term solution to the problem users experience. I may try to work on it this week.

I am somewhat curious, having never used ocaml-migrate-parsetree myself, of whether it is possible to use it in a non-driverized project. (Not that non-driverized are better in absolute.) I am also curious of why non-jbuilder systems don't manage to correctly use driverized extensions, but that is learning for another time.

@let-def
Copy link
Contributor

let-def commented Oct 23, 2017

Answering @gasche question about ocaml-migrate-parsetree:

At the basic level, OMP provides nothing more than a function from parsetree to parsetree, where you can choose an abitrary version on either side (so the actual interface looks more like: 'from_parsetree ocaml_version -> 'to_parsetree ocaml_version -> 'from_parsetree -> 'to_parsetree).

Two issues appear:

  • one of convenience, because each OMP-enabled PPX has to convert to and from the version it is implemented against and the compiler-libs version (this is an OMP invocation ritual of two lines that can simply be copy-pasted from PPXs to PPXs)
  • one of performance, because with many PPXs there is a risk of converting back and forth similar versions many time (which was not acceptable in, e.g., JaneStreet case if I understand properly)

So OMP also offers an alternative to Ast_mapper driver where one registers one or more PPX and OMP builds a minimal "rewriting" pipeline. This interface also allows each rewriter to specify one or more options (or cookies).
This can be used standalone (you are a PPX writer and wants the benefits of OMP while using the classic pipeline), or by linking a custom rewriter that register all rewriters at preprocessing-time.

In the end, OMP can be used in three ways:

  • as a simple library
  • as an alternative driver, with some similarities and differences compared to existing solutions (ast_mapper, PPX-driver, and the derivers from PPX-deriving)
  • as a common interface for linking the composition of different PPXs (implemented with different versions of the Ast) in a single binary.

@gasche
Copy link
Contributor Author

gasche commented Oct 23, 2017

Thanks for the clarification -- that is close to my cloudy understanding, only more precise.

This means that experimenting with "just enough of OMP in ppx-driver in ppx_deriving itself and its drivers to build properly on new OCaml versions even if they don't handle the new constructions completely right" should be within reach, incrementally, from the pre-omp state. I'm sure you and Jérémie already did most of that work when you proposed an omp-ized ppx-deriving, so one could study your approaches to understand what is in there. However, I don't have high hopes of personally having time to devote to this before January, so that's why I discussed a much dumber approach above.

@ejgallego
Copy link

But I just don't see how ppx_import in its current form can exist in a frictionless ppx world.

Well, I guess the it should exist the same way it has existed before for quite a long time? Introducing better and faster build/preprocessing methods seems fine to me, but producing hard-to-solve breakage in users it is never going to be worth the tradeoff IMVHO.

It doesn't use omp so you will possibly have to update it on every new version of the compiler.

That looks easy to solve to me, and if I understand correctly, unrelated to the current bug with ppx_import.

Additionally, it contradicts the view that preprocessing and compiling are done at 2 separate stages in your build pipeline. This is an important assumption for fast builds. It's up to you to decide how important that is to you.

Umm, I am afraid that I am lost here, why is that so important for fast builds? It seems to me that you just need finer dependency management. I'm sure there are some use cases where exposing typing to the preprocessor would make sense.

By the way, why would abandoning your software is an option? While ppx_import is convenient, you can certainly find work arounds. See ocaml/dune#193 (comment)

I've added some more details there as not to pollute this [closed] thread more, but I am not so sure about that I can certainly find workarounds. Cheers!

@whitequark
Copy link
Collaborator

I'm just not convinced that adding proper, omp aware, META generation to oasis is so much harder than adding classical ppx support to jbuilder.

And why even should oasis adapt to your new shiny way of doing things, that is incidentally only better in your opinion and not necessarily mine, and not jbuilder adapt to the old way of doing things, which people have used for literal years?

That is exactly how jbuilder privileges omp: because of jbuilder's corporate backing, it essentially makes a core compiler interface, Ast_mapper, impossible to use for ppx authors, and forces them to switch to omp.

If something, it is your responsibility and it should be your job to find ocamlbuild, OASIS, OCamlMakefile, etc, and make them work well together with omp, if you want to push jbuilder. Not the other way around.

@rgrinberg
Copy link
Contributor

@avsm @gasche

While I don't have a problem with your decision to revert omp in ppx_deriving, please don't forget that there might be existing users of ppx_deriving and jbuilder out there already. Those users will find themselves in a very uncomfortable situation and we should make sure they know what their options and workarounds are.

I would not rely on jbuilder getting support for classical ppx anytime soon. None of the arguments for this feature that are presented here are new and as Drup said, diml remained unconvinced. So we'd have to wait until he's back to even consider this, even if a patch was ready today.

This was referenced Oct 24, 2017
@gasche
Copy link
Contributor Author

gasche commented Oct 24, 2017

So I took the initiative to create a 4.01-maintenance branch on the upstream repository -- because it's easier to review PRs from there. I chose the point right after the revert of @let-def's omp patch, which has all the non-omp-related changes that later went into 4.2. This is an experiment, please have a look and feel free to give your opinion on the approach.

Then I added support for OCaml 4.05 ( #154 ) and 4.06 ( #155 ) on top of it, using ugly conditionals. This was very easy.

What I would eventually like to get is something that we could release to users of 4.1 to support newer OCaml versions. If you like the direction and the PRs eventually get approved, hopefully we can get a release with this support in opam-repository (I am optimistic we can do this without breaking the build of jbuilder users). I would be happy to try to commit to maintaining this branch in the future, if nothing else.

@gasche
Copy link
Contributor Author

gasche commented Oct 24, 2017

@whitequark: I'm sorry I did not make more efforts to work on this issue earlier. I dropped the ball on this one.

To others: I must say that I'm a bit disappointed with the response to this issue. I generally try to stay away from PPX matters, and I only created it when I, as an external observer, got to the point where I found the current state too embarrassing to wait any longer. I was frustrated (and a bit angry) when I created the issue, and I think that (1) we collectively failed to address it for too long (I shouldn't have had to create it) and (2) the response to me raising the issue was not adequate. I have the impression that people take the problem more seriously now that @whitequark expressed publicly that he is angry and hurt -- I wish it would not have come to this. I'm working on this on my free time, and I know very well it's none of us' explicit job to take care of it -- I don't mean to tell others people what to do. But hopefully we can collectively try harder to fix breakage faster when it affect many OCaml users.

@let-def
Copy link
Contributor

let-def commented Oct 24, 2017

@gasche: I don't know much about ppx, I got involved through OMP. I would be happy to adapt OMP to specific needs if I can get a good understanding of what is involved. I don't want to lose time on PPX because, as I am not an user, I can't take informed decisions. And btw I am frustrated by the situation too (but not angry).

TL;DR If there is something that can help the situation from OMP side, I will by happy to try to implement it. I don't want to get involved directly on PPX stuff.

@emillon
Copy link
Contributor

emillon commented Oct 24, 2017

Hi,

First, I'd like to thank @whitequark for this library. It saved us countless lines of boilerplate code and has given us the closest to "typeclasses" we can have these days. I also second @avsm's call to calm and respect to all parties involved.

To answer the initial question (1), here is my experience as a happy user and contributor of ppx_deriving:

In our code base we're heavily using ppx_deriving (eq, ord, show) with ounit to pretty-print and compare values without having to rely on polymorphic comparison. We're also using ppx_deriving_yojson for lots of types to define serialization formats. In both of these use cases, we rely on ppx_deriving's options to override the defaults, for example to ignore a field in comparisons, or change the name of JSON keys. We're also using ppx_bin_prot which adds a dependency to ppx_driver.

For now we use ocamlbuild, ocaml 4.04 and ppx_deriving 4.1 and it's working very well.

The problems we're hitting are:

  • we can't upgrade to 4.05 because it's not supported by 4.1;
  • we can't easily use 4.2 because ppx_driver fails with "unused attributes";
  • we can't use jbuilder because it relies on a driver-aware version of ppx_deriving_yojson.

Using jbuilder is not a short term goal, but being able to use a recent compiler is important to us.

I'm a bit surprised to see that using OMP requires changing the external interface, or switching to ppx_driver. It would be nice to have a decoupled version, as @gasche said.

As for the solution, I agree that a non-OMP "fork" will be useful to solve most of these issues at least in the short term.

@let-def
Copy link
Contributor

let-def commented Oct 24, 2017

@emillon:

I'm a bit surprised to see that using OMP requires changing the external interface, or switching to ppx_driver. It would be nice to have a decoupled version, as @gasche said.

Yesterday, I said:

At the basic level, OMP provides nothing more than a function from parsetree to parsetree, where you can chose an abitrary version on either side (so the actual interface looks more like: 'from_parsetree ocaml_version -> 'to_parsetree ocaml_version -> 'from_parsetree -> 'to_parsetree).

Everything else is opt-in.

@emillon
Copy link
Contributor

emillon commented Oct 24, 2017

Absolutely, I was referring to the ppx_deriving <-> OMP integration in particular. Sorry for the confusion.

@rleonid
Copy link

rleonid commented Oct 24, 2017

I'd also like to echo the sentiments of support for @whitequark 's work. I still remember reading your blog post while trying to figure out how to convert bisect to ppx. Thank you!

I think that praise is in order for @gasche for voicing concerns and stepping in to provide solutions. Sometimes, done is better than perfect. Thank you as well.

@rgrinberg
Copy link
Contributor

@emillon is porting ppx_deriving_yojson to omp an option at all? That seems like it will fix all the issues for you. It will make ppx_deriving_yojson compatible with 4.05.0, allow you to use either jbuilder or ocamlbuild in harmony with all the other deriving plugins that you've listed.

we can't easily use 4.2 because ppx_driver fails with "unused attributes";

Could you elaborate on this? Seems like your blocker is the fact that ppx_deriving_yojson simply doesn't support 4.05.0

@bikallem
Copy link
Contributor

bikallem commented Oct 25, 2017

@gasche

Making a minor maintenance release of the pre-omp ppx-deriving codebase that supports new OCaml version is probably the easiest short-term solution to the problem users experience. I may try to work on it this week.

This seems sensible in the short term to target 4.06 release.

I am somewhat curious, having never used ocaml-migrate-parsetree myself, of whether it is possible to use it in a non-driverized project. (Not that non-driverized are better in absolute.) I am also curious of why non-jbuilder systems don't manage to correctly use driverized extensions, but that is learning for another time.

As it stands currently, I think the usage of ocaml-migrate-parsetree is probably not an issue. The issue is probably the deriving approach as used by ppx_deriving and the deriving approach ppx_driver as championed by jbuilder. One difference that I have noticed is that ppx_deriving favours plugin-based, dynamic-loading approach while ppx_driver/jbuilder favours statically linking all ppxes in a single binary. However, I am not sure if this in itself is the cause of the issue.

Maybe if ppx_deriving is to move forward with supporting jbuilder then we probably need to go full in with the ppx_driver based approach, i.e. using ppx_type_conv, jbuilder itself in ppx_deriving project and statically compiled ppxes approach. ppx_driver/jbuilder does present compelling benefits including support for ocamlbuild based build systems.

@bikallem
Copy link
Contributor

@whitequark

After returning from eight months of involuntary leave due to undiagnosed bipolar II disorder,

My wishes to you getting better.

Indeed, the machinery that you have implemented to make ppx_deriving work so far is very impressive and well appreciated by the community.

@emillon
Copy link
Contributor

emillon commented Dec 28, 2017

@rgrinberg it looks like this was unrelated to OMP, but rather an incompatibility between ppx_type_conv and ppx_deriving. It's getting better with the latest releases of ppx_driver.

@vassilmladenov
Copy link

@rgrinberg Thanks for the worked example for jbuilder. I was able to adapt it for Dune with the following file

(executable
 (name a)
 (libraries ppx_deriving.std core)
 (preprocess (pps ppx_deriving.std ppx_jane)))

@rgrinberg
Copy link
Contributor

This should be closed as it's not really relevant anymore. (@gasche and/or the other maintainers).

@vassilmladenov glad to know things are working for you. Everything should work out of the box now so there are no longer any issues.

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

No branches or pull requests