Dune: How to support bisect_ppx?

[rgrinberg]

bisect_ppx is an odd ppx rewriter because you only want it active when you're measuring coverage. Usually when you're running your test suite. Dually, the tests only require the bisect ppx runtime when measuring the coverage. Ideally, this is something jbuilder would support on a global basis with minimum hassle. @diml any advice on might jbuilder accommodate bisect_ppx?

[jeremiedimino]

We can get started by using the ml syntax and setting up bisect depending on some user settings.

We could also make jbuilder​ knows about bisect, but i'd rather not hardcode the knowledge of too many tools in jbuilder itself as it will become a maintenance nightmare. Hopefully later we can have some more modular configuration and allow individual packages to provide some kind of plugins

[rgrinberg]

OK I understand the sentiment about bloating the tool. But I do think that bisect_ppx is no less important than ppx_inline_tests and certainly no less important than ppx_bench. But I guess this all depends on the size of the patch for it.

Are there concrete plans for a plugin api btw? Or is it something to think about post 1.0

[jeremiedimino]

Are there concrete plans for a plugin api btw? Or is it something to think about post 1.0

I have some ideas (see #56), but I think this should indeed come post 1.0. For 1.0 I prefer to focus on the essential stuff. Nice user features that will require careful design can be added later

[jeremiedimino]

I had a closer look at bisect_ppx and it seems to me that all we need to do the same as ocamlbuild -package bisect_ppx is a -ppx <spec> argument that would be the same as adding (preprocess (pps (<spec>))) to all relevant stanzas

[rgrinberg]

The tests would also need a bisect_ppx.runtime runtime dependency I believe to actually run and report the coverage. But yes, it's no more than that.

[jeremiedimino]

Well as long as the ppx for bisect_ppx declares the runtime dependencies in the META file, it should be added automatically

[aantron]

I'm highly interested in this, both as one of the maintainers of Bisect_ppx, and because we badly need to have usable coverage analysis in Lwt. Also, cc @rleonid.

[aantron]

We need a way to:

• trigger the preprocessor running on the files with code under test, but not the test suites,
• compile both with bisect.runtime,
• link everything with bisect.runtime,

conditionally when coverage is enabled.

[aantron]

It's also possible to restructure Bisect_ppx in various ways, so let me know the constraint space.

[jeremiedimino]

I suppose one easy way to do it would be to always specify ppx_bisect in the list of ppx, and then have ppx_bisect do nothing if BISECT_COVERAGE is unset. For the runtime dependency, you need to add this to the META file:

ppx_runtime_deps = "bisect.runtime"

and jbuilder will pick it up automatically.

[aantron]

I'm working on finishing porting Bisect to omp+jbuilder (https://github.com/aantron/bisect_ppx/pull/117, started and nearly finished by @rgrinberg – thanks), so that it's at least usable in the pps stanza, and we will use it immediately in Lwt as you describe above, minus the environment variable. However, adding it unconditionally has the effect of

• Running the preprocessor on every file no matter what, which implies a build performance penalty. I suppose if other PPXs are being run, the cost is not so great with omp, but if Bisect is the only PPX, then the whole cost of building and running the driver has to be paid.
• Introducing Bisect as a hard dependency of the project.

Before we release Lwt, we will manually remove the Bisect_ppx references. However, this isn't a user-friendly process, and not something most projects should be asked to do. So, I think we need a more sustainable solution.

Let me know any other ideas for using Bisect_ppx with Jbuilder, or if I'm missing something in my description above. I'll be happy to further adapt the project, if needed.

[andrewray]

@aantron I don't think the performance of the build matters at all for bisect. @diml However, being able to turn it off and on is important, and may need some help from jbuilder?

I would personally very much like an integrated way to run bisect in my jbuilder powered ocaml projects.

[aantron]

+1

Yep, I agree that the performance doesn't matter when actually wanting to run Bisect – but both the performance and the hard dependency matter if there is no nice way to remove Bisect for release.

Another thing is, with the environment variable, if I am triggering some kind of recursive build of multiple dependencies, and some of them are also sensitive to the variable, I might get performance penalties, rebuilds, excess bisect*out files in the current directory.

[aantron]

Ok, so Bisect_ppx master now uses Jbuilder and omp (thanks again to @rgrinberg), but of course we still don't have a good story for Jbuilder users of Bisect.

[rgrinberg]

Maybe this is short sighted, but I don't really see what's wrong with adding some hard coded bisect support to jbuilder. I imagine something as primitive as --bisect flag that will add the appropriate dependencies when doing build or runtest.

I'd be more sympathetic if this was an invasive change to jbuilder, but it doesn't seem like that to me now.

[aantron]

With the flag, I'd be concerned that the user has insufficient control over which files get instrumented. At least, the code under test needs to be instrumented, and the tests don't. I'm not sure this will be easy to get right over all users with a single flag.

[jeremiedimino]

I suppose that if support is hard-coded in jbuilder, we'd allow to write something like (bisect true) in library stanzas. In general I think it's better not to hard-code the knowledge of external tools if we can avoid it.

In this case, what we really want is add a ppx conditionally. There are other cases where this would be useful, such as a ppx tracer to print the names of every function called. We could add support for this in jbuilder. For instance you'd write ?ppx_bisect, and ppx_bisect would be considered only if PPX_BISECT is set.

[aantron]

That sounds quite reasonable. The only thing I would add is that I'd rather avoid environment variables, and have some way of triggering the condition by passing an argument to Jbuilder.

[jeremiedimino]

Seems fine to me, we could have --enable-pps ppx_bisect (pps to match (preprocess (pps (...))))

[aantron]

Yep. As long as ppxopts following bisect_ppx in the pps list is also turned on/off correctly, it should work good. In fact, this should give Jbuilder better handling of Bisect_ppx than Ocamlbuild currently offers (where we had to resort to a plugin and an environment variable, but wish for a command-line argument).

This scheme might be insufficient for some more-exotic PPX out there, but I think it's enough for most usage of Bisect. cc @rleonid

[rleonid]

Under this scheme would

ppx_runtime_deps = "bisect.runtime"

also be dynamic; in the sense that it is loaded only when the code is instrumented with bisect?

[aantron]

@rleonid I believe so, since that would be pulled in only when ?bisect_ppx ends up enabled. @diml can correct me..

[rgrinberg]

I still think the current proposal suffers from having to add this annoying boilerplate in your jbuild files just for bisect support. Just imagine trying bisect some 3rd party package. It's especially annoying given that jbuilder should be able to tell what are libraries and what are tests.

But I suppose this also gives you the flexibility to enable bisect conditionally only for a few libraries. In any case, I'm just happy we'll have something in the interim.

[aantron]

If Jbuilder can be opinionated enough to do this with a flag and no modification to Jbuilder files, that's fine too.

But the flag shouldn't be called --bisect. Ideally, it should be some kind of more-general mechanism. Also, Bisect is a weird and misleading name for a coverage tool, that also conflicts with git, and there's no need to propagate that problem into Jbuilder :)

[jeremiedimino]

What's the story regarding ppxopts? Currently jbuilder ignores this field

[aantron]

What's the story regarding ppxopts? Currently jbuilder ignores this field

I don't understand this query.

[jeremiedimino]

This is regarding this sentence:

As long as ppxopts following bisect_ppx in the pps list is also turned on/off correctly, it should work good.

Does it mean that jbuilder needs to interpret ppxopts fields in order for bisect_ppx to work?

[aantron]

No, not any more than I believe it already does. I was just referring to the ability to use

pps (... ?bisect_ppx -simple-cases ...)

and that not confusing Jbuilder if bisect_ppx is not enabled, i.e. disabling bisect_ppx should also disable any subsequent arguments to it in pps.

[aantron]

(For any users of Bisect that may be watching, please don't use -simple-cases. I'm just using it as a simple example).

[jeremiedimino]

Ah, I see. So that would have to be supported by #168. Alternatively bisect_ppx could take options via cookies: --cookie bisect.simple-cases=true.

BTW, before we commit to this solution I'd like to explore a bit the alternatives. I think @rgrinberg has a point about having to write a lot of boilerplate.

I think the solution of adding a ppx everywhere via a command line flag could work. We just need some way to disable instrumentation for some files. It seems to me that instrumenting only public libraries by default is a good approximation. Then bisect_ppx could take a cookie to force enabling or disabling instrumentation. Jbuilder already communicates the name of the current library to the ppx rewriters via a cookie (http://jbuilder.readthedocs.io/en/latest/advanced-topics.html#driver-expectation). We could add another cookie to communicate the visibility of a library.

[aantron]

I think we can wait for the results of exploring for a good solution.

I'm not sure about the cookie approach for arguments. I was confused by the manual. When you do

(preprocess (pps (ppx1 -foo ppx2 (-bar 42))))

do all of the PPXs get -foo -bar 42? Or ppx1 gets -foo and ppx2 gets -bar 42? If the former, then it seems like properly supporting ?bisect_oox -simple_cases should be straightforward (cc @andrewray, who I'm sure is already subscribed anyway :)).

This sentence is the source of my confusion:

The list of libraries will be ppx1 and ppx2 and the command line arguments will be: -foo -bar 42.

Regarding the cookie for enabling/disabling, I'm mildly concerned about the performance when Bisect is the only PPX, and is disabled (whether due to lack of a command-line flag, or due to being applied to testing source files).

I'm not advocating premature optimization by any means, but it would be regrettable if Jbuilder built the ppx program, only containing a single disabled PPX, and ran everything through that, and that later turned out to be a performance problem, and Jbuilder had to add hacks like examining the PPX list to work around it. But then again, maybe that's the right way. I don't know enough about the build/PPX problem space at this point; I'm just raising a potential concern here.

[andrewray]

The command line flags are an issue at the moment. All the arguments are grouped together and applied to the single ppx driver. This doesn't work well with ?ppx which, if it had arguments, must remove them if the ppx is not applied.

The obvious solution would be to group options (left-to-right) to the nearest specified ppx driver. They would still get flattened eventually, but it would provide enough information for the optional ones to get filtered out first. I can implement this.

I am not so convinced by the argument of it being a hassle to add ?bisect_ppx in jbuild files - especially since they can be added and committed without affecting other functionality.

That said, if a global ppx option (--add-global-pps / --add-global-pps-options ?) is used, I am not sure why it shouldn't apply to the entire build, rather than some heuristics to only apply to certain parts. In particular, I am not sure why you wouldn't sometimes want to run code-coverage on the code for an executable, or indeed what the problem is with including it even if you dont want it (something to do with the overall coverage % maybe?).

[jeremiedimino]

We could measure the impact of switching from no_preprocessing to a no-op ppx, but my gut feeling is that the performance degradation is going to be close to 0.

or indeed what the problem is with including it even if you dont want it (something to do with the overall coverage % maybe?).

I suppose that the reporter could filter out the parts we are not interested in. I'm curious of how bad this would be. If the difference in terms of performances is negligible, then instrumenting everything unconditionally seems like the best solution.

[aantron]

instrumenting everything unconditionally

This has two consequences.

1. There is a performance impact to running instrumented code, in addition to performing the act of instrumenting itself. Whether this impact is significant, I don't know. We have a test in Bisect_ppx for measuring this on some simple program:

   λ make performance
   jbuilder build
   jbuilder build test/performance/test_performance.exe
   cd _build/default/test/ && \
           performance/test_performance.exe -runner sequential
   
    uninstrumented (ocamlc)
   
   real    0m0.013s
   user    0m0.009s
   sys 0m0.001s
   .
    uninstrumented (ocamlopt)
   
   real    0m0.004s
   user    0m0.002s
   sys 0m0.001s
   .
    instrumented (ocamlc)
   
   real    0m0.092s
   user    0m0.087s
   sys 0m0.001s
   .
    instrumented (ocamlopt)
   
   real    0m0.010s
   user    0m0.008s
   sys 0m0.001s
   .
   Ran: 4 tests in: 0.59 seconds.
   OK
   

   We run the instrumented loop 1,000,000 times, and IIRC this is enough to make the running time dominated by instrumentation, rather than by the outputting of .out files at process exit.

2. Pushing decisions about what to instrument into the reporter is something I'd rather avoid doing. We already have issues around how to ignore files (e.g. aantron/bisect_ppx#130). Adding a new class of files (test runners) to ignore through Bisect (rather than Jbuilder) would be a usability loss (including teachability and maintainability), as users would have to learn Bisect's mechanisms, in addition to Jbuilder mechanisms they already have to learn for managing which libraries apply where.

   Right now, with Ocamlbuild, only a subset of users has to learn the -exclude-file options, or use comments like in the linked issue. Everyone else handles it using what they already know: the _tags file. If we push the decision about what to instrument or report entirely into Bisect, every user is likely to have to deal with these things, promoting them from hacky emergency escape hatches into necessary features for using Bisect effectively.

   There was already an issue (aantron/bisect_ppx#89) about enhancing Bisect_ppx's exclusion mechanism, due to a build system (omake) that didn't allow sufficient control over the build. We fixed that by extending Bisect's ad-hoc exclude language [[1](https://github.com/aantron/bisect_ppx/blob/07a994f6bbe8724ad62e77b75f2c326810da33ea/src/syntax/excludeLexer.mll)] [[2](https://github.com/aantron/bisect_ppx/blob/07a994f6bbe8724ad62e77b75f2c326810da33ea/src/syntax/excludeParser.mly)]. I'd much rather be on the way to removing this type of thing from Bisect, instead of coming to rely on it more and more due to build systems.

   cc @rleonid on this subject.

So, I prefer to avoid instrumenting unconditionally on the Jbuilder level, but please offer any counterarguments.

[aantron]

There are also more exotic testing setups than explicit source files for test cases. For example, I believe containers generates test files from these annotations. cc @c-cube.

As I understand it, Jane Street also has some libraries like that, though I can't remember all the testing frameworks without searching for them again.

It would be good not to

• Have Bisect or Jbuilder try to predict all such setups.
• Have the user be surprised by names of generated files, or struggle to exclude them.
• Have the developers of test frameworks worry about whether changing filenames, or other such activities, might negatively affect Jbuilder+Bisect and downstream users.

I think the easiest way to achieve all that is to specify what is instrumented, not what is not instrumented. The user (and Jbuilder?) is most aware of their own project's actual source files.

Also, on the subject of exclusion in Bisect, it seems better to maintain one inclusion (or exclusion) mechanism in Jbuilder, than one in each Bisect-like instrumentation library. Doing it centralized that way is again a win for learning and maintainability (of both user projects, and of Bisect and other instrumenters).

[c-cube]

Just fyi: yes, tests in containers (and batteries) are generated from comment using a command-line tool which looks for comments starting with (*$T or (*$Q. The build system must call the tool to generate a test file that is then compiled and run. And I'm not going to switch to ppx because of 4.01.

[rgrinberg]

Simon, then I think jbuilder wouldn't work for your use case anyway.
Jbuilder already requires 4.02.3 as a minimum

On Wed, Jun 28, 2017, 10:08 PM Simon Cruanes [e-mail]
wrote:

Just fyi: yes, tests in containers (and batteries) are generated from
comment using a command-line tool which looks for comments starting with
($T or ($Q. The build system must call the tool to generate a test file
that is then compiled and run. And I'm not going to switch to ppx because
of 4.01.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/janestreet/jbuilder/issues/57#issuecomment-311689676,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIe-47zK_dUahKJlNW-2Vi3jh95Q5hkks5sImyDgaJpZM4M9TSl
.

[aantron]

I think it's good to have the perspective. There are likely to be users of such setups that do use PPX and can use Jbuilder; it's not the testing setup that inherently precludes a minimum of 4.02.3.

[jeremiedimino]

Thanks for the explanation, I didn't realized that we needed more fine grained selection that whole libraries or executables. In this case yh, specifying bisect explicitly when and where we want to use it seems best.

For performances, I did expect instrumented code to be slower. What I was more curious about is the impact of instrumenting the test code in addition to the library code being tested. I guess it boils down to how much test code vs code being tested is run.

[rgrinberg]

I'm also convinced that fine grained selection of the code to instrument is essential. But I'm still not convinced of the current solution of modifying lots and lots of jbuild files just to instrument code. Of course, providing all this information through a few couple arguments also seems impractical.

I suppose this is one of those things that ocamlbuild handled gracefully, with its damn _tag files :sweat_smile: Never mind, we need something that's a little more ocaml like. How about a plugin system that just supports mapping over the entire tree of stanzas? OCaml programmers are well versed with writing such mappers already. I'm only half joking here...

So for now our best solution seems to be to add these "optional" preprocessors that can be toggled. A few questions/comments about this:

1. The fact that this "optionality" works on preprocessors alone makes the feature look a little half baked to me. Why can't it work on libraries, flags, etc.?

2. There's already an (optional) stanza, isn't there? These 2 features are too similar in my opinion. Can they be unified somehow?

3. How do we handle passing the same set of arguments to bisect? Do we have to copy these arguments in every optional stanza? What about when we have different flags for different pps? Suppose that we want more than 1 instrumentation profile here. Seems like we'd quickly end up back in square 1 and write jbuild files in OCaml syntax.

Also, on the subject of exclusion in Bisect, it seems better to maintain one inclusion (or exclusion) mechanism in Jbuilder, than one in each Bisect-like instrumentation library. Doing it centralized that way is again a win for learning and maintainability (of both user projects, and of Bisect and other instrumenters).

And yet bisect has its own exclusion facility already (linking for people other than Anton). Which I find myself heavily preferring over anything suggested in this thread at the moment. Not that I want us to give up, but hopefully this gains us some perspective.

Also, let's not forget that bisect has users other than jbuilder. While I don't see a reason to use build systems other than jbuilder, perhaps other users of bisect would appreciate if this was done outside of jbuilder?

[aantron]

Thanks for the explanation, I didn't realized that we needed more fine grained selection that whole libraries or executables. In this case yh, specifying bisect explicitly when and where we want to use it seems best.

It may be that instrumenting whole libraries (but probably not whole executables) is what we want. I just mean I think it makes more sense to make this on an inclusion rather than exclusion basis. It's possible that all users, even with exotic test setups, only pull those test files into tester executables, and never into libraries under test. But it requires study, which I haven't done.

For performances, I did expect instrumented code to be slower. What I was more curious about is the impact of instrumenting the test code in addition to the library code being tested. I guess it boils down to how much test code vs code being tested is run.

It's likely a minimal impact, hence the the mention earlier of avoiding premature optimization :) However, it's a bit clumsy, together with the other drawbacks of instrumenting everything.

@rgrinberg raises good points that I have to agree with, like the one about currently having to maintain the passing of arguments to Bisect (and I guess PPXs in general) from multiple jbuild files.

Also, let's not forget that bisect has users other than jbuilder. While I don't see a reason to use build systems other than jbuilder, perhaps other users of bisect would appreciate if this was done outside of jbuilder?

It is already done outside Jbuilder, and will have to remain due to omake, etc. Indeed, Bisect has to remain usable from the other build systems.

However, I think it would be unfortunate if we had to rely on these Bisect_ppx-specific mechanisms to use Jbuilder at all with Bisect:

• Jbuilder is supposed to be a nice new build system, not a pain in the rear for anybody that wants to use Bisect or a similar tool on their project.
• The exclusion mechanisms basically exist for inferior (relative to Ocamlbuild) build systems like omake (Jbuilder is not trying to be inferior to Ocamlbuild), and for weird situations like certain kinds of generated files in the source tree.
• As the OCaml community continues to mature, it's likely that there will be other instrumenters, PPXs, or general preprocessors, that run as part of testing. Should each one develop its own exclusion mechanism, because of a clumsy approach (not yet!!!) taken in Jbuilder? Should each maintainer team maintain this, and should each user have to learn and suffer bugs in all of them? If Jbuilder addresses this well, Bisect and these tools can avoid the burden, if they just don't promise to support Makefiles and/or omake :) Ocamlbuild already doesn't have this problem. Hopefully we can do at least as well as Ocamlbuild here.

I also want to repeat that, if there is a nice way to restructure Bisect_ppx to help with this issue, we will gladly do it. We can even accept exclusion in Bisect if absolutely necessary, but it's not a nice way :)

[rgrinberg]

@aantron can we close this? I think your workaround with bisect is more than good enough

[aantron]

I don't think it's good enough.

It introduces a false dependency on Bisect_ppx for development that has to be manually removed for release.

The dependency also affects CI. That's currently blocking testing Lwt on MinGW, as a result of which I'm getting Bisect_ppx into the MinGW OPAM repo. Note that we're not trying to run Bisect_ppx on MinGW. It just gets pulled in because of the overestimated dependencies.

[samoht]

any update on this? I'd love to use bisect on my jbuilder-ized projects :-)

[aantron]

@samoht Bisect_ppx 1.3.0 was released with a Jbuilder workaround: https://github.com/aantron/bisect_ppx/blob/master/doc/advanced.md#Jbuilder

Here's an example of it being used in Lwt:

• jbuild
• Makefile
• opam

[jeremiedimino]

Are there other coverage frameworks in OCaml that are actively maintained and widely used?

If not, I think we should apply the same policy as for ppx_inline_test here: we add hardcoded support for bisect_ppx in jbuilder, and if one day we need to support multiple coverage framework, we will look at generalising the system so that other frameworks are usable as well.

What information would the user need to put in jbuild files? Do we still need to select what .ml files are being instrumented?

[rgrinberg]

Yay. I concur, there's no need to make the users wait while we figure out how to generalize things.

There's nothing for the users to specify in the jbuild file I believe. bisect_ppx has its own config file for controlling which module gets instrumented. One thing that we might want to do is think of a way to fit this into the profile work you have in progress. For example, enable bisect support in a particular profile (or have a default bisect profile).

[jeremiedimino]

Ok, that simplify things.

I suppose we can add a coverage variable to the environment, maybe with the possibility to set it from the command line. It would accept the following values:

• no_coverage, the default
• bisect_ppx, to enable coverage with bisect_ppx

This should allow to add support for other frameworks in the future.

Other question: I assume that the ppx rewriter needs to be applied before all other ppx rewriters, right? i.e., if the user already has a (preprocess (pps (...))), we can't just add it to the list.

[jeremiedimino]

A few other thoughts:

circular dependency

Let's say we want to measure the coverage of bisect_ppx itself, then we have a circular dependency problem. Though it shouldn't be an issue, because technically you can always build bisect_ppx first without instrumenting it and then use this version to rebuild it with instrumentation.

It might not be a huge deal, but it does feel unsatisfactory to have this arbitrary limitation.

what to instrument

My mental model of coverage is the following: you have a library mylib that depends on foo and bar, and test programs test1.exe and test2.exe that together contain all the tests for library foo.

What you want to do is the following: build test1.exe and test2.exe using the instrumented version of mylib, but using the standard versions of foo and bar.

If you have a big workspace with many libraries and tests, that becomes complicated.

solution: instrument in separate build contexts

I think all that could be solved using separate build contexts for instrumentation. For instance, you could define a build context mylib-coverage where you would specify that you want to instrument library mylib, and in this context, you would always use the bisect_ppx tool from default.

This could reuse the same mechanism as cross-compilation, except that only instrumentation tools would be taken from the host context.

For instance, we could have a jbuild-workspace file that looks like this:

(context default)

(context
 (derived
  (from default)
  (name mylib-coverage)
  (env
   (coverage bisect_ppx)
   (covered_libs (mylib)))))

Additionally, instead of coverage, we could just use the term instrumentation, since from the point of view of jbuilder there is nothing specific to coverage.

[jeremiedimino]

That said, we should just start with the coverage and covered_libs variables and keep the other stuff for later.

[aantron]

Are there other coverage frameworks in OCaml that are actively maintained and widely used?

None that I am aware of.

Do we still need to select what .ml files are being instrumented?

Yes. Typically users want to instrument src/ but not test/. We could move the logic for that into Bisect_ppx if needed (it already has the ability to exclude files based on name, etc). However, if it's easy in Jbuilder, it would be nice to support it there.

bisect_ppx has its own config file for controlling which module gets instrumented.

This is an awkward mechanism, it's yet another config file to maintain and deal with, so again, if controlling what gets instrumented can fit nicely into the Jbuilder model, then it would be welcome. For comparison, we currently choose what is instrumented in Lwt and other projects by either listing bisect_ppx or not as a preprocessor in each jbuild file. In Ocamlbuild days, we chose which files the coverage tag applies to with the right glob expressions.

I assume that the ppx rewriter needs to be applied before all other ppx rewriters, right? i.e., if the user already has a (preprocess (pps (...))), we can't just add it to the list.

I think it can be applied in any order (and we test the results), but it makes the most sense to apply it first – otherwise it can instrument generated code that may be meaningless to the user.

Let's say we want to measure the coverage of bisect_ppx itself

We're unlikely to do this, as it's annoying to maintain. See https://github.com/aantron/bisect_ppx/issues/140.

My mental model of coverage is the following: [...]

It seems accurate, except that I think you have a typo: "test1.exe and test2.exe that together contain all the tests for library foo." That should be for library mylib. If so, then I agree. I guess Jbuilder workspaces complicate choosing what to instrument.

(covered_libs (mylib)))))

There is also the possibility to instrument complete executables.

[jeremiedimino]

Ok, so in the end it seems to me that there are two configuration aspects:

• static configuration: for every library, we might want to statically states that some files should never be instrumented by bisect_ppx. Without special configuration, we should assume that all files should be instrumented
• dynamic configuration: we want to express that some libraries and or executables should be considered for instrumentation in a specific jbuilder run

For the static part, it's easy, we can just have:

(library
 ((name foo)
  (covered_modules (:standard \ a b c))))

to say that all modules but a, b and c should be instrumented when coverage is enabled. And we'd have the same for executables.

For the dynamic part, I suggest to allow filtering per directories. So for instance you might write:

$ jbuilder build --env '(coverage bisect_ppx) (covered_dirs (src bin))' 

where --env <sexp> is a flag that will mean to add this entry to the environment. i.e. it will set the bisect_ppx.dirs variable globally to (src bin).

Here, I'm assuming that file selection is all we need to specify. In practice, do people sometimes need to pass specific flags to the ppx rewriter?

[aantron]

do people sometimes need to pass specific flags to the ppx rewriter?

There are the -exclude and -exclude-file options, which are the only "nice" way of getting Bisect_ppx to selectively not instrument certain functions in generated files. These have some users.

There is also the -conditional option for using Bisect_ppx with current Jbuilder, but of course that will be made obsolete by solving this PR.

The interaction looks decent to me. Only concern is that "covered" is used here as if it means "instrumented for coverage," in fact "covered" means "instrumented for coverage and then visited during testing." I don't immediately have a replacement to offer, though.

[aantron]

The -exclude-file option is documented here: https://github.com/aantron/bisect_ppx/blob/master/doc/advanced.md#files-and-top-level-values

IIRC we are discouraging using the -exclude option, and left it out of the docs to avoid having people know about it.

[jeremiedimino]

There are the -exclude and -exclude-file options, which are the only "nice" way of getting Bisect_ppx to selectively not instrument certain functions in generated files. These have some users.

I see. I'm surprised you can't you attributes for that?

The interaction looks decent to me. Only concern is that "covered" is used here as if it means "instrumented for coverage," in fact "covered" means "instrumented for coverage and then visited during testing." I don't immediately have a replacement to offer, though.

Ok. Another suggestion is to make it looks like bisect_ppx is a plugin. It will take a bit of time to add proper support for external plugins in jbuilder, but we could say that bisect is a plugin distributed with dune for now and might be distributed externally in the future. We'll have to discuss the exact syntax for plugins while we switch to dune, but it could look like this:

(lang ocaml 1.0 (bisect_ppx 1.0))

(library
 ((name foo)
  (bisect_ppx (
   (modules (:standard \ a b c))
   (flags (per_module (...)))))))

that would give you more freedom to add more things to the jbuild files.

[aantron]

I see. I'm surprised you can't you attributes for that?

We have some mechanisms superseding nearly all use cases of -exclude-file, just not the above one because we and our users have little control over generated files.

I don't have an opinion on whether Bisect_ppx should be a plugin – I'm not informed enough about the internals of Jbuilder.

[jeremiedimino]

I don't have an opinion on whether Bisect_ppx should be a plugin – I'm not informed enough about the internals of Jbuilder.

It's going to be the same implementation-wise at the moment. It's just that if we use a generic interface, it's harder to change it in the future if you want to add things.

[rgrinberg]

static configuration: for every library, we might want to statically states that some files should never be instrumented by bisect_ppx. Without special configuration, we should assume that all files should be instrumented

Yes, these statically excluded files are almost always generated sources. For example, atdgen generated serializers.

For the dynamic part, I suggest to allow filtering per directories. So for instance you might write:
$ jbuilder build --env '(coverage bisect_ppx) (covered_dirs (src bin))'

How would this look like from the workspace file?

[rleonid]

Yes, these statically excluded files are almost always generated sources.

Not necessarily, for example, one would probably exclude the source of the tests.

[rgrinberg]

Tests should already be excluded by the fact that they're executables. But
you're right, libraries for tests only could be exploded as well.

On Fri, Feb 16, 2018, 10:49 PM Leonid Rozenberg [e-mail]
wrote:

Yes, these statically excluded files are almost always generated sources.

Not necessarily, for example, one would probably exclude the source of the
tests.

—
You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub
https://github.com/ocaml/dune/issues/57#issuecomment-366273060, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIe-5EoeShNDxa7X09nxW-kw9en8oltks5tVaOEgaJpZM4M9TSl
.

[rgrinberg]

Just to clarify this example:

For instance, we could have a jbuild-workspace file that looks like this:

(context default)

(context
 (derived
  (from default)
  (name mylib-coverage)
  (env
   (coverage bisect_ppx)
   (covered_libs (mylib)))))

mylib-coverage is the context that will provide the ppx's binaries that will be linked with the instrumentation code (bisect_ppx) and will be used to preproces the context named default?

What I find a bit annoying about this setup is that switching between a normal and a bisect build will trigger a full rebuild. It would be nice to have an "instrumented" context instead. This is easy enough to accomplish by using an opam context, but it would be nice if we could just do it on the default.

[jeremiedimino]

mylib-coverage is the context that will provide the ppx's binaries that will be linked with the instrumentation code (bisect_ppx) and will be used to preproces the context named default?

It's the opposite actually. Basically it'd be similar to something like this:

(context (default (targets (native covered))))

[jeremiedimino]

But if it's not clear, that probably mean it's over-engineered...

[rgrinberg]

Jérémie Dimino [e-mail] writes:

But if it's not clear, that probably mean it's over-engineered...

In my opinion the first approach you took where we explicitly specified
the "build" context was the clearest. You could very easily tell which
context's toolchain was used for building intermediate artifacts.

(context default)

(context
((name mylib-coverage)
(host (derived (from default) (env (coverage bisect_ppx))))))

And this would create a mylib-coverage.default context that will host
all the instrumentation. Therefore, normal builds are completely
separate from bisected builds. I don't suggest switching to this
however, as it would be really confusing in conjunction with the target
stuff.

To confirm, your definition would only definition would only result in 2
contexts:

default: contains the ppx binaries with bisect_ppx

mylib-coverage: contains code preprocessed with bisect_ppx

But hopefully, we'll have a high level command line interface that will
hide all this ugly stuff from the user. E.g. some --bisect flag.

[jeremiedimino]

Yes, that's the idea.

[rgrinberg]

I've discussed this offline with @emillon and @diml and the plan is as follows:

• Support instrumentation as a separate preprocessor. This is necessary to guarantee that it runs first and that it works with non ppx preprocessed code. Actually, the latter point is achievable using driver priorities, but it just complicates things.

• We'll have a default profile to support instrumentation. The name would be coverage.

• We'll still have a way to ignore modules/directories/coverage ppx when instrumentation is set.

• @ostera pointed out to me that other instrumentation ppx's: https://github.com/LexiFi/landmarks/ . While this shouldn't present a problem for us, we might need the options in the point above to be configurable per profile.

[jeremiedimino]

Sounds good. We should also allow to enable coverage in arbitrary contexts, so that we can combine it with the dev more for instance.

[nojb]

Sorry to come late to the discussion, but regarding landmarks: indeed, being able to enable/disable the instrumentation based on a flag or environment variable on arbitrary contexts would be pretty useful to us.

[avsm]

In the meanwhile, avsm/arp@63826d0d95e9403f6c380b6427110a6a08c876f8 uses dynamic dune file generation to avoid the need for the unconditional runtime dependency when bisect is disabled, in case anyone needs a workaround

[aantron]

What I would want, based on my experience to date:

1. Don't add special hardcoded support for Bisect_ppx.
2. Project developers should opt into coverage. I don't find this problematic at all, because there are few dune files, and project structure rarely changes. Opting in is the easiest option to understand, because it doesn't require learning any hidden rules about where Bisect gets applied.
3. We don't want Dune controlling the order of when Bisect gets applied. We had an issue in Bisect recently about changing the order from first to last, and we are able to deal with this using OMP's API. Special handling would make this kind of change difficult.
4. We just need a simple and general mechanism for inserting S-expressions into (preprocess (pps ...)).
5. Use a variable, (preprocess (pps {:bisect})). Please correct me if the syntax is wrong.

6. Set the variable in a coverage profile in dune-project:

   (env
   (coverage
    (flags -g -opaque)
    (:bisect (bisect_ppx --flags))))
   

   I'm not sure about the exact syntax.

   This allows setting the command line in one place. It also allows for each dune file to specify other PPXs, by listing them after {:bisect}.

   {:bisect} (or whatever variable) should expand to nothing if undefined (i.e., when not using the coverage profile) The preprocess stanza needs to be prepared for no atoms after pps.

7. A build with instrumentation is triggered with dune build --profile coverage.

8. All other builds are unaffected by this scheme. Dune never looks for Bisect_ppx or links it in other builds, so Bisect truly becomes a {dev} dependency in opam terms, requiring no manual handling during release.
9. My only remaining concern is that in a workspace, if there are multiple projects with coverage profiles, I typically want only one that I'm focusing actually built with coverage. This is minor, though.

Using env was originally proposed by @jberdine in https://github.com/ocaml/dune/pull/419#issuecomment-386535109.

@nojb, is this scheme suitable for landmarks?

[jeremiedimino]

Inserting arbitrary s-expressions at any place is not currently supported, and I'm not sure it will ever be. So I'd suggest something slightly more specific here, such as the ability do define "special" preprocessors. You'd define a special preprocessor using the env stanza:

(env
 (<profile>
  (define_special_pp <name> <ppx-rewriters-and-flags>)))

Where <ppx-rewriters-and-flags> would use the same syntax as in (pps <ppx-rewriters-and-flags>).

For instance:

(env
 (coverage
  (define_special_pp bisect bisect_ppx -- -foo))
 (_
  (define_special_pp bisect)))

And we would use a special preprocessor with the syntax (special <name>), for instance:

(library
 ...
 (preprocess (pps ppx_sexp_conv (special bisect))))

[aantron]

That seems quite reasonable to me, @diml.

Just to add clarity,

 (_
  (define_special_pp bisect)))

is so that bisect expands to nothing in profiles other than coverage, rather than being undefined, and this is indeed triggered with dune build --profile coverage? I am asking so that I can get this right in the Bisect documentation.

[jeremiedimino]

That's right. Another option would be to say that (special bisect) expands to nothing when bisect is not defined. However, typos would then go unnoticed which is not good.

[aantron]

I think using _ is a good idea. It shouldn't be too obscure, as the Bisect docs will tell users what to paste into dune-project.

My only additional comment so far is I hope we can come up with a better adjective than "special." Perhaps we don't even need one, and we can just have (define_pp) and (expand ...) or (global ...) or something along those lines. I don't think either expand or global is a good fit, but some other word might be.

[aantron]

Is the -- on the command line interpreted by Dune?

[edwintorok]

Might be useful to have the ability to define additional preprocessors at the package and workspace level than just the individual library/executable level (or alternatively via dune upgrade). This would allow enabling coverage builds project wide with relatively little change to the dune files themselves, which is useful when a project has close to a hundred opam packages spread across a dozen repositories.
Ideally the ocaml CI scripts should be able to enable coverage for a project mechanically and with ease (without having to resort to complicated sed expressions).

[rgrinberg]

Actually that is something that we've been just discussing. The current plan is to allow one to add instrumentation using the env stanza:

(env
 (bisect
  (preprocess (pps :standard (special bisect)))))

Do you think that would be sufficient?

[rgrinberg]

@diml this special constructor is a bit confusing to me, if it's just a variable, why don't we just call it %{bisect} then? Then in the env stanza we could have something simple like:

(env
 (coverage
  (define
   (bisect bisect_ppx))
 (_
  (define bisect ()))

Or something like that. That fits much better with other features and doesn't affect the functionality here.

[nojb]

@nojb, is this scheme suitable for landmarks?

cc @mlasson who is the landmarks expert.

[jeremiedimino]

@rgrinberg no objection in principles, however the variable handling inside Dune is currently too ad-hoc. So before we do anything clever with variables, we need #1888.

[nobrowser]

The latest consensus solution seems to have lost sight of one objective, that to me (a user of bisect_ppx) is pretty important - to isolate instrumented and non-instrumented builds from each other, so a manual full rebuild is not necessary when switching back and forth. Am I right?
This is related to a more general problem I have trying to use dune features beyond the utterly trivial. There are contexts and there are profiles. They seem to be similar but some things (such as isolating builds in separate subdirectories of _build) can only be done with one and some with the other. Is it possible that we have more entities than necessary?

[jeremiedimino]

I wondered about that. Profiles are a bit lighter though. Originally, additional contexts where tied to an opam switch, so switching contexts would have required installing a new opam switch. At the same time, the concept of release/dev modes is universal and shouldn't be tied to what switches the user has installed.

In any case, the shared cache will change a bit the equation here, as switching back and forth between different profiles will eventually become fast.

[jberdine]

I have also wondered about the necessity of both contexts and profiles, as IME most contexts are added in order to specify a different profile. It seems necessary to have an additional context in order to have a separate copy of the build artifacts produced using a different profile. And the fact that a separate context enables building multiple profiles in parallel is important, and it is useful to be able to use e.g. an flambda compiler only in release profiles. But perhaps that sort of functionality could usefully be disentangled with e.g. opam switches.

[nobrowser]

But as far as the coverage proposal is concerned, if I do the normal development edit/compile/test loop for a while, and then want to check coverage, I still have to do a dune clean. True or false? I think it is true and I wonder if I'm the only one here bothered by it.

[jeremiedimino]

Well, you don't need to run dune clean manually, dune will automatically delete and/or rebuild what needs to be deleted/rebuilt. But yh, you'd essentially restart a build from scratch. However, dune is flexible enough in this regard, so if you wanted to define a separate build context where coverage is always activated, then you could do that to keep the two builds.

[aantron]

But as far as the coverage proposal is concerned, if I do the normal development edit/compile/test loop for a while, and then want to check coverage, I still have to do a dune clean. True or false? I think it is true and I wonder if I'm the only one here bothered by it.

This will be false. Dune dependency tracking will take care of rebuilding the needed files, and again when not doing coverage anymore. The dependencies of files will change to include bisect_ppx when coverage is enabled.

This is an explicit goal of the integration (and has always been), and I wouldn't approve integration that didn't have this property. All discussion above has had this as a basic assumption. You're not the only one that would be bothered by not having this :)

[nobrowser]

@diml :

dune is flexible enough in this regard, so if you wanted to define a separate build context where coverage is always activated, then you could do that to keep the two builds.

I've been reading and re-reading the dune doc since before I first commented here, but I don't see how this would work. The only two documented ways to define a context are:

(context default)

(context (opam (switch 4.foo.bar)))

I must be missing something basic, but the doc must be failing to explain something basic.

[jeremiedimino]

You are right, the doc needs a refresh here. You can actually write this since a while:

(context (default (profile foo))
(context (default (profile bar) (bar bar)))

[nobrowser]

@diml : Looks promising! I'll try it soon. Thanks.

[nobrowser]

Well, it looked promising at first, but now I'm confused again.

Should I perhaps start a new issue, since this is only tangentially related to bisect_ppx?

[jeremiedimino]

Opening a new issue seems good

[aantron]

@diml, @rgrinberg, I'd like to ask you to address this issue (as opposed to me; I was previously looking into fixing it). The problem with me doing it is that, IIRC, when I looked at it in the summer, I found that the expander needed to replace variables depends on having a context, dependency information was already computed by the time a context was created, and there was no reasonable way to amend it. As a result, there was no (simple) way to introduce a new dependency from variable expansion. It seems necessary to introduce the dependency only after expansion, since a non-coverage build should not require Bisect to be installed — a typical non-coverage build would be any release build during normal installation by a user. On the other hand, a coverage build should depend on Bisect, including so that a rebuild is triggered if coverage is enabled and Bisect has changed.

When I considered how to fix that, it seemed that a pretty extensive refactoring would be necessary, including of LibDB. I don't think I can, from my outsider position, efficiently make the design decisions such a refactoring would entail.

[jeremiedimino]

The master of dune now has a bisect_ppx extension to make it easy to setup code coverage via bisect_ppx in dune projects.

It doesn't yet have a file exclusion mechanism or a way to specify flags, but the basis is here and such features can now be added incrementally.

In the meantine, please test it and report feedback!