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?

[ghost]

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

[ghost]

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

[ghost]

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.

[ghost]

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.

[ghost]

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 (aantron/bisect_ppx#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.

[ghost]

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.

[ghost]

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 :)

[ghost]

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

[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?

[ghost]

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.

[ghost]

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.

[ghost]

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?

[ghost]

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.

[ghost]

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!