Enable/disable bisect_ppx via dune-workspace#3403
Conversation
|
I think it would be possible to generalize this PR to add syntax in dune-workspace files like: The ppx that would actually end up getting used for each library/executable would be the intersection of the 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 |
926718e to
774ba6e
Compare
d212acb to
186c886
Compare
|
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
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: Stephanie You <youstephanie98@gmail.com>
186c886 to
4393704
Compare
ghost
left a comment
There was a problem hiding this comment.
Looks all good apart from the extension registration that I missed in the first review
|
@stephanieyou could you also add an entry in the @rgrinberg or @aantron, does one of you want to have a quick look at this PR? |
|
Looks good to me. Usage is:
I think it would be best if this didn't require an extra boilerplate file ( |
|
That's a good point. We initially wanted to add the |
Signed-off-by: Stephanie You <youstephanie98@gmail.com>
Signed-off-by: Stephanie You <youstephanie98@gmail.com>
Signed-off-by: Stephanie You <youstephanie98@gmail.com>
ghost
left a comment
There was a problem hiding this comment.
Just a couple of suggestions for the manual, but looks all good otherwise!
Signed-off-by: Stephanie You <youstephanie98@gmail.com>
|
Thanks for this work! I'm glad we finally have an answer for users of dune and bisect_ppx :) |
|
This is fantastic! Thank you @stephanieyou! |
This reverts commit b398913.
#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:The library
foowould be instrumented for bisect_ppx, and the executabletestwould not be.