Add support for optional ppx preprocessors #168

andrewray · 2017-06-27T00:17:31Z

(mainly to support bisect_ppx, but possibly useful elsewhere).

They are specified in a (preprocess (pps (?ppx))) stanza. The
'?' marks them as optional.

A command line argument '--enable-optional-pps=...' takes a comma
separated list of ppx packages to enable.

Note; jbuild.ml; Preprocess_map.filter_optional. In the
Per_module.Per_module branch, it is possible we could end up with
an empty set of preprocessors (this case is checked in the For_all
branch). If this is a problem, we probably want a String_map.filter_map
function to be added into import.ml.

(mainly to support bisect_ppx, but possibly useful elsewhere). They are specified in a `(preprocess (pps (?ppx)))` stanza. The '?' marks them as optional. A command line argument '--enable-optional-pps=...' takes a comma separated list of ppx packages to enable. Note; jbuild.ml; Preprocess_map.filter_optional. In the Per_module.Per_module branch, it is possible we could end up with an empty set of preprocessors (this case is checked in the For_all branch). If this is a problem, we probably want a String_map.filter_map function to be added into import.ml.

ghost · 2017-06-27T13:56:34Z

src/gen_rules.ml

@@ -1023,6 +1023,23 @@ let gen ~contexts ?(filter_out_optional_stanzas_with_missing_deps=true)
               String_set.mem package.name pkgs
             | _ -> true)))
    in
+    let stanzas =
+      let spec = Option.value ~default:String_set.empty enable_optional_pps in


Why not ?(enable_optional_pps=String_set.empty)?

ghost · 2017-06-27T13:58:06Z

src/jbuild.ml

  val compare : t -> t -> int
 end = struct
-  type t = string
+  type t = string * bool


Let's use a record: { name : string; optional : bool }

ghost · 2017-06-27T14:05:10Z

src/jbuild.ml

+    match pp with
+    | Pps { pps; flags } ->
+      let pps = List.filter_map pps ~f:disable_optional in
+      if pps = [] then No_preprocessing


This should be pps = [] && flags = []

Does it really make sense to have flags but no actual preprocessor? Would this be for the driver?

ghost · 2017-06-27T14:06:46Z

src/jbuild.mli

@@ -50,6 +51,9 @@ module Preprocess_map : sig
  val find : string -> t -> Preprocess.t

  val pps : t -> Pp.t list
+
+  (* filter out optional pps which have not been enabled *)
+  val filter_optional : String_set.t -> t -> t


Let's use a label: enabled:String_set.t -> ...

ghost · 2017-06-27T14:10:06Z

bin/main.ml

+  let enable_optional_pps =
+    Arg.(value
+         & opt (some (list string)) None
+         & info ["enable-optional-pps"] ~docs ~docv:"PACKAGES"


PACKAGES is not very clear here, as we usually don't talk about findlib packages with jbuilder. Let's just use PPX-REWRITERS

ghost · 2017-06-27T14:15:44Z

Thanks. It seems fine to me to keep a map with entries mapping to No_preprocessing

Flags are associated with the preceeding ppx definition. This allows the flags for optional ppx's to be removed from the command line if not enabled.

andrewray · 2017-06-28T10:57:02Z

I've added logic to deal with the issues around flags for optional ppx rewriters, and also command line options to add ppx's globally (except where a preprocess action stanza is used).

ghost · 2017-06-29T10:50:18Z

I prefer to have the global pps as a separate PR. Moreover, I don't think it should go in until we can chain pre-processors and so use both an action and pps preprocessors on the same file. Otherwise it feels a bit like a half-implemented feature.

andrewray · 2017-06-29T11:22:42Z

global ppxs removed.

ghost · 2017-06-30T10:16:03Z

Thanks, I'll merge this after the 1.0.0 release

aantron · 2017-06-30T11:27:14Z

@diml, for planning purposes in other projects, could you give a very rough ETA (e.g. 1 month, 3 months, 6 months, 1 year) for 1.1.0 or when this will be released? We won't try to hold Jbuilder to a timeline. It's just so we know if it's worth working around now, or 1.1.0 will be released so soon that it's better to wait for the release.

ghost · 2017-06-30T11:46:23Z

So I'm away next week until the 11th. I'm planning to do the 1.0.0 when I get back. After that I think we can do minor releases fairly often. I'd expect the 1.1.0 to come no more than a month later as there are already a lot of new features ready. I just don't want to keep adding new features and postponing the 1.0.0 for now.

aantron · 2017-06-30T11:57:59Z

Thanks!

rgrinberg · 2017-07-10T05:47:16Z

IMO the value of this feature is questionable. It doesn't really make it easier to use bisect because making preprocessors optional is easy enough with the OCaml syntax for jbuild files. And I can't imagine any other use case for this.

aantron · 2017-07-10T11:06:32Z

I don't know if this PR is the right solution, however here is a jbuild file in OCaml syntax for making Bisect_ppx optional:

https://github.com/ocsigen/lwt/blob/0e3ade3b740d2b267dc6fe893ce7e656a6d314d6/src/core/jbuild

I wouldn't say it's that easy – this file is much more difficult to read that a regular jbuild file.
If I want to have this in multiple subdirectories, do I need to convert all the jbuild files? Will they have duplicate code? How do I share code between jbuild files? Do I need to write a jbuild-generating build step?
The Jbuilder manual mentions:

It is not clear whether the OCaml syntax will be supported in the long term as it doesn’t work well with incremental builds. It is possible that it will be replaced by just an include stanza where one can include a generated file.

lpw25 · 2017-07-10T12:06:55Z

I don't know whether this has been suggested before, but couldn't you always run ppx_bisect and then pass it an option to tell it whether to do anything. If this option were set through a global variable of some sort, and these variables were settable from the command line, wouldn't that give you the same experience without needing hard-coded support for optional ppxs. It also seems more general, for example allowing you to turn tests on/off: which can't be done with an optional ppx since the ppx still needs to run in both cases (to remove the tests when they are disabled).

aantron · 2017-07-10T12:48:19Z

...couldn't you always run ppx_bisect and then pass it an option to tell it whether to do anything...

There is some discussion around here: #57 (comment).

ghost · 2017-08-15T07:39:31Z

Looking at this again, it seems to me that #136 is the right solution to this problem.

ppx_bisect could have two implementations:

a default one that would do nothing and declare no runtime dependencies
a coverage one, that would be the real one

Then, to enable coverage one would do:

$ jbuilder build -variants coverage

where -variants coverage would have the effect of enabling the coverage variant globally. This would work nicely for things such as inline tests as well, as when dropping tests we wouldn't have to add the runtime dependency.

rgrinberg · 2017-09-12T05:22:02Z

@diml I think bisect already supports pretty much what you're saying through a runtime flag. It uses a shallow identity mapper to implement the "do nothing" variant. @aantron points out that this requires all packages to take on a false dependency and I'm inclined to agree with him that it makes the support partial when compared to other build systems.

aantron · 2017-09-12T06:03:38Z

Furthermore, if one wants to avoid the false dependency, one has to edit opam files manually for release. I do this for Lwt. I've heard it mentioned by others, that they don't want to do it for their projects, so they have their jbuild file(s) in OCaml syntax.

Before Bisect_ppx 1.3.0, Lwt also had jbuild in OCaml syntax, in part for this reason.

ghost · 2017-09-22T11:05:24Z

Hmm, I see. I was thinking of dependencies at the library level, but you are thinking of dependencies at the package level. Indeed, with my solution we must always depend on the ppx_bisect package. Although not necessarily the whole ppx_bisect package: the dummy implementation could be distributed as a small standalone package.

I suppose if we do want 0 dependencies when coverage is not enable and we want to keep the syntax light, we don't have much choice. Unless someone has another idea, we should just go ahead with this PR.

mseri · 2017-11-28T14:35:53Z

What is going on with this? It would be great indeed to use something like it to enable/disable ppx_bisect at build time, instead of relying on manual interventions or ugly autogenerations

rgrinberg · 2017-11-28T14:42:33Z

From my perspective this is a non solution. Because I don't want to be editing every single jbuild file I ever wrote just to gain bisect support.

edwintorok · 2017-11-28T15:01:39Z

I think that having optional ppx support in jbuilder is a good starting point, and would remove some of the boilerplate in using bisect_ppx.
To avoid changing all the jbuild files could the idea be extended to jbuilder workspaces as well? Then you'd just drop a workspace file wherever you want to enable coverage builds.

mseri · 2017-11-28T15:11:40Z

That sounds better, indeed.

But at the moment bisect support requires changing all the jbuild files anyways, and having something more natural than autogenerating jbuild files or manually editing them each time, would already be a good start.

rgrinberg · 2017-11-28T15:21:00Z

It might be a good start for now, but it would be just cruft down the road. If the optional preprocessing has no use other than enabling/disabling bisect. I also don't see why only preprocessors need to be optional this way. Seems like libraries, module names, flags, etc. should all work with mechanism if we find it desirable. From my experience with oasis, such a "flags" feature did prove to be useful, and specializing to preprocessors does not seem warranted.

mseri · 2017-11-28T15:36:29Z

Fair point, I completely agree with what you say. I think a solution is still needed for the coverage. Would you consider the extension of the workspaces suggested by @edwintorok as a better approach?

rgrinberg · 2017-11-28T15:40:58Z

I support the ability of jbuilder automatically configuring coverage for a build because doing so is easy from an implemention perspective and requires no boilerplate from users. I don't see why enabling bisect via workspaces is better than say a command line flag for example. But I have nothing against it either.

…

On Tue, Nov 28, 2017, 11:36 PM Marcello Seri ***@***.***> wrote: Fair point, I completely agree with what you say. I think a solution is still needed for the coverage. Would ypu consider the extension of the workspaces suggested by @edwintorok <https://github.com/edwintorok> as a better approach? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-9wkaYGxdZDh9Hdw-k0G1igB8gOfks5s7Ch-gaJpZM4OGAPs> .

aantron · 2017-11-28T15:44:28Z

If any modifications or restructuring of Bisect are needed, please let me know. We'll be happy to adapt the tool for a well-factored build process.

mseri · 2017-11-28T15:56:41Z

@rgrinberg that is great. Last time there was a discussion on bisect I had understood that making jbuilder deal with it was out of the question

rgrinberg · 2017-11-28T16:14:48Z

Yes, hardcoding it to be only usable for bisect is a no-no. But I'm thinking perhaps adding a flag (or a workspace config) to allow to add preprocessors/libraries to libraries/executables. Preferably it would be something that is useful in other circumstances as well. Though if not, we could always get rid of it later. At least users won't have to change their jbuild files.

…

On Tue, Nov 28, 2017, 11:56 PM Marcello Seri ***@***.***> wrote: @rgrinberg <https://github.com/rgrinberg> that is great. Last time there was a discussion on bisect I had understood that making jbuilder deal with it was out of the question — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-4jVPm-LbCKuJjyJxbTf2I4Z7-RGks5s7C05gaJpZM4OGAPs> .

rgrinberg · 2018-02-15T04:46:54Z

I'm closing this PR as we have a much better story for how to implement bisect support and profile support could subsume this use case as well.

ghost suggested changes Jun 27, 2017

View reviewed changes

ghost mentioned this pull request Jun 27, 2017

How to support bisect_ppx? #57

Closed

andrewray added 2 commits June 27, 2017 18:09

optional ppx; address code review issues

fbec7f0

Parse pps command and associate flags with ppx

d19b7b7

Flags are associated with the preceeding ppx definition. This allows the flags for optional ppx's to be removed from the command line if not enabled.

andrewray force-pushed the bisect branch from 91f20a4 to d19b7b7 Compare June 29, 2017 11:21

ghost added this to the 1.1.0 milestone Jun 30, 2017

aantron mentioned this pull request Jul 8, 2017

Testing: conditional coverage analysis for Lwt core ocsigen/lwt#438

Merged

gaborigloi mentioned this pull request Jul 25, 2017

Port version 2.x to Jbuilder xapi-project/nbd#61

Merged

aantron mentioned this pull request Nov 7, 2017

[WIP] Enable ppx_bisect. ocsigen/lwt#424

Closed

rgrinberg closed this Feb 15, 2018

aantron mentioned this pull request Aug 6, 2019

Figure out the Dune integration aantron/bisect_ppx#203

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for optional ppx preprocessors #168

Add support for optional ppx preprocessors #168

andrewray commented Jun 27, 2017

ghost Jun 27, 2017

ghost Jun 27, 2017

ghost Jun 27, 2017

andrewray Jun 27, 2017

ghost Jun 27, 2017

ghost Jun 27, 2017

ghost commented Jun 27, 2017

andrewray commented Jun 28, 2017

ghost commented Jun 29, 2017

andrewray commented Jun 29, 2017

ghost commented Jun 30, 2017

aantron commented Jun 30, 2017

ghost commented Jun 30, 2017

aantron commented Jun 30, 2017

rgrinberg commented Jul 10, 2017

aantron commented Jul 10, 2017

lpw25 commented Jul 10, 2017

aantron commented Jul 10, 2017

ghost commented Aug 15, 2017 •

edited by ghost

Loading

rgrinberg commented Sep 12, 2017

aantron commented Sep 12, 2017

ghost commented Sep 22, 2017

mseri commented Nov 28, 2017

rgrinberg commented Nov 28, 2017

edwintorok commented Nov 28, 2017

mseri commented Nov 28, 2017

rgrinberg commented Nov 28, 2017

mseri commented Nov 28, 2017 •

edited

Loading

rgrinberg commented Nov 28, 2017 via email

aantron commented Nov 28, 2017

mseri commented Nov 28, 2017

rgrinberg commented Nov 28, 2017 via email

rgrinberg commented Feb 15, 2018

Add support for optional ppx preprocessors #168

Add support for optional ppx preprocessors #168

Conversation

andrewray commented Jun 27, 2017

ghost Jun 27, 2017

Choose a reason for hiding this comment

ghost Jun 27, 2017

Choose a reason for hiding this comment

ghost Jun 27, 2017

Choose a reason for hiding this comment

andrewray Jun 27, 2017

Choose a reason for hiding this comment

ghost Jun 27, 2017

Choose a reason for hiding this comment

ghost Jun 27, 2017

Choose a reason for hiding this comment

ghost commented Jun 27, 2017

andrewray commented Jun 28, 2017

ghost commented Jun 29, 2017

andrewray commented Jun 29, 2017

ghost commented Jun 30, 2017

aantron commented Jun 30, 2017

ghost commented Jun 30, 2017

aantron commented Jun 30, 2017

rgrinberg commented Jul 10, 2017

aantron commented Jul 10, 2017

lpw25 commented Jul 10, 2017

aantron commented Jul 10, 2017

ghost commented Aug 15, 2017 • edited by ghost Loading

rgrinberg commented Sep 12, 2017

aantron commented Sep 12, 2017

ghost commented Sep 22, 2017

mseri commented Nov 28, 2017

rgrinberg commented Nov 28, 2017

edwintorok commented Nov 28, 2017

mseri commented Nov 28, 2017

rgrinberg commented Nov 28, 2017

mseri commented Nov 28, 2017 • edited Loading

rgrinberg commented Nov 28, 2017 via email

aantron commented Nov 28, 2017

mseri commented Nov 28, 2017

rgrinberg commented Nov 28, 2017 via email

rgrinberg commented Feb 15, 2018

ghost commented Aug 15, 2017 •

edited by ghost

Loading

mseri commented Nov 28, 2017 •

edited

Loading