Enable/disable bisect_ppx via dune-workspace

[stephanieyou]

#57

Enable bisect_ppx in different build contexts via dune-workspace. Example dune-workspace file would contain (context (default (bisect_enabled true))).

To specify which libraries/executables should be preprocessed with bisect_ppx, add (bisect_ppx) to the stanza, instead of using the (preprocess (pps ...)) field. Example:

(library
  (name foo)
  (bisect_ppx))
(executable
  (name test)
  (libraries foo))

The library foo would be instrumented for bisect_ppx, and the executable test would not be.

[nojb]

I would be really interested in supporting the same functionality for landmarks, with a similar workflow as bisect_ppx.

Is it possible to make this feature a bit more general?

cc @mlasson

[stephanieyou]

I think it would be possible to generalize this PR to add syntax in dune-workspace files like:
(context (default (enable bisect_ppx landmarks)))
and in dune files:

(library
  (name foo)
  (optional_pps bisect_ppx landmarks))
(library
  (name bar)
  (optional_pps bisect_ppx))

The ppx that would actually end up getting used for each library/executable would be the intersection of the enable list from dune-workspace, and the optional_pps list.

That being said, I believe this is meant to be a short-term solution for bisect_ppx before some refactoring gets done and a more permanent solution can be implemented.

cc @diml

[ghost]

Generalising would be nice. However Stephanie will soon be switching project and it feels a bit too late to me to go back and generalise this feature. Additionally, support for bisect is a very long standing feature request and initially all the conversations got stuck into bike shedding because we couldn't find the right generalisation. There is such a thing as "premature generalisation". So for now, let's wrap up bisect support.

FTR, we had the same issue for inline tests. Once we decided to specialise the support for ppx_inline_test, the generalisation became obvious. i.e. it became obvious what was the general structure and what were the configuration bits.

In any case, I'm all for supporting other tools that use similar workflows. So happy to brainstorm about a generalisation after this PR. And if we can't find the right generalisation, then it's only fair that we had similar specific support for landmarks.

[ghost]

Looks good, but let's make sure to error out when the user tries to use (bisect_ppx) in combination with (preprocess (action ...)), to avoid surprises.

[ghost]

Looks all good apart from the extension registration that I missed in the first review

[ghost]

@stephanieyou could you also add an entry in the CHANGES.md file?

@rgrinberg or @aantron, does one of you want to have a quick look at this PR?

[aantron]

Looks good to me. Usage is:

1. Add dune-workspace like

   (lang dune 2.5)
   
   (context default)
   (context (default (name coverage) (bisect_enabled true)))
   

2. Run tester with dune runtest --context coverage

I think it would be best if this didn't require an extra boilerplate file (dune-workspace), and coverage settings properly belong to a project rather than to a workspace, since one is typically trying to instrument a project's code. We can debate/address that in follow-on work, however, like when generalizing for landmarks.

[ghost]

That's a good point. We initially wanted to add the bisect_enabled setting to the env stanza, but it turned out to be complicated to implement in the current state of the code base.

[ghost]

Just a couple of suggestions for the manual, but looks all good otherwise!

[ghost]

Thanks for this work! I'm glad we finally have an answer for users of dune and bisect_ppx :)

[avsm]

This is fantastic! Thank you @stephanieyou!