bsconfig: per-directory and dev ppx-flags needed for Bisect_ppx (coverage)

[aantron]

Bisect_ppx, the coverage tool, now works with BuckleScript (instructions, starter repo). It needs a bit of help from BuckleScript, however, for the integration to be truly painless.

I opened a new issue to keep the text together, and also because #3482 (comment) gave me the impression that #3715 is being suggested as an alternative, but Bisect_ppx actually needs both supported, so this is a different situation.

1. (ppx-specs field in build schema #3482) There should be per-directory ppx-flags in bsconfig.json. Right now, ppx-flags gets applied to the whole project, which causes the test cases to be instrumented, which is very noisy in the coverage report. The current workaround for this is to manually exclude the tests, but this is awkward.

   For comparison, this is exactly how Bisect has been working with Dune for years. src/ directories get preprocessed, but test/ directories do not.

2. (ppx-dev-flags support #3482 #3715) It should be possible to mark some ppx-flags as dev, so that the PPX won't even be run in release builds. If this is not done, Bisect_ppx becomes a hard dependency of the project, which has to be removed manually each time a release is tagged.

   For comparison, this is currently a problem with Dune as well (How to support bisect_ppx? ocaml/dune#57), but we plan to solve it shortly along the same lines (How to support bisect_ppx? ocaml/dune#57 (comment)). So far, because of the lack of dev-only PPX support, this is the kind of cleanup that has to be done on each release of a project that uses Bisect_ppx: aantron/markup.ml@ea68beb#diff-d218652a79a651b9be8eee7641ea0893L4

[bobzhang]

Hi @anntron, it is nice to see bisect works with bucklescript.
The ppx per directory is easy to implement, but it is hard to generate proper merlin file, how does dune solve this issue?

IIRC, there are three things from bisect_ppx, the ppx tool, ppx runtime, and ppx coverage report analzyer, do you think it is possible to subsume into bs-platform directly (mostly ppx runtime, so it can build directly?).

[aantron]

@bobzhang, can you give some detail on what is the difficulty with the .merlin files? I didn't see anything in the linked issues, and I can't think of what the problem would be.

It's possible to subsume Bisect into bs-platform directly, but how would you like to do it? You are correct about the pieces it has.

Ideally, if it's possible to keep Bisect separate, that would be best for administration, maintenance, and release cycle reasons. There is one advantage to integrating Bisect into bs-platform, which is we can make sure that Bisect's PPX tool always matches the version of OCaml in bs-platform more easily that way. But I also opened ocaml-ppx/ocaml-migrate-parsetree#78 about conclusively solving that on the Bisect end in a way that doesn't depend on bs-platform.

[bobzhang]

On Aug 19, 2019, at 9:07 PM, Anton Bachin ***@***.***> wrote: @bobzhang <https://github.com/bobzhang>, can you give some detail on what is the difficulty with the .merlin files? I didn't see anything in the linked issues, and I can't think of what the problem would be.

How do you config .merlin to have various ppx flags for each directory?

It's possible to subsume Bisect into bs-platform directly, but how would you like to do it? You are correct about the pieces it has.

Depends on the size of the project, if it is small, maybe worth venturing, otherwise, as you said, keep it separate works better

…

Ideally, if it's possible to keep Bisect separate, that would be best for administration, maintenance, and release cycle reasons. There is one advantage to integrating Bisect into bs-platform, which is we can make sure that Bisect's PPX tool always matches the version of OCaml in bs-platform more easily that way. But I also opened ocaml-ppx/ocaml-migrate-parsetree#78 <ocaml-ppx/ocaml-migrate-parsetree#78> about conclusively solving that on the Bisect end in a way that doesn't depend on bs-platform. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3761>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFWMK6W6SRKIUXGEKHDP4LQFKLKRANCNFSM4IMO7VLQ>.

[aantron]

How do you config .merlin to have various ppx flags for each directory?

You can place a .merlin file in each directory. At least, that's what Dune does in each of my projects.

[bobzhang]

Ah, I did not know that. Thanks for the info

[aantron]

Bisect_ppx is now out for BuckleScript and available as an npm package, so I'd like to ping this issue again. It would be very nice to resolve this for a fully ergonomic integration; then I will add instructions to the Bisect README about where the Bisect PPX flags should go in bsconfig.

[TheSpyder]

an option to have a dev-mode bsconfig.json, or something similar as discussed in #3716, would also help with this

[aantron]

In case we go with the idea of specifying the bsconfig.json, we still need at least #3482 (per-directory PPX flags) to avoid the annoying workaround of having to specify [coverage exclude_file] in the test suite and any other files that are not part of the code under test. There are two reasons this is annoying:

1. It is error-prone.
2. It has to be done for every file, so the larger the test suite becomes, the more punishing the workaround becomes.

It's much cleaner to be able to opt into instrumentation of the files that need it by directory, rather than try to individually opt out in every file that shouldn't get it.

[bobzhang]

@aantron I think the best way to do this is having an environment variable so when it is set, bisect will be instrumented, bisect can pick up some convention to not instrument code in test files, what do you think?

[aantron]

We already have an environment variable like this, and have to use it. See here, step 4.

This doesn't work, because the project gains a hard, non-dev dependency on Bisect, which either stays, or has to be removed manually or by fragile script during release.

There is no reason why downstream users of releases should transitively depend on Bisect. In particular, having downstream users depend on Bisect means installing binaries of a PPX, which are large, could be missing, could trigger a build from source code, etc. It's much better if all of that risk can be avoided.

We've had an analogous discussion in Dune in ocaml/dune#57 and elsewhere. In fact, we introduced BISECT_ENABLE as the result of discussions after the release of Dune, since Ocamlbuild hadn't had issues with Bisect. After trying BISECT_ENABLE for a short while, the consensus quickly developed between me, users, and Dune developers, that we need better support for it than that, in particular we need Dune to have a notion of conditional or development PPX.

The convention about test files also is questionable, see ocaml/dune#57 again for some of the discussions on that.

[aantron]

To put it another way, when the environment variable that currently enables Bisect is not set, that still doesn't remove the permanent dependency on Bisect from bsconfig.json. The project's release just ends up with an unnecessary dependency on a PPX, that is a no-op.

There is also an issue with dependency instrumentation (which we don't want). If BISECT_ENABLE is set, it should not trigger instrumentation of dependencies that also were using Bisect in their development.

[aantron]

And note that bs-dev-dependencies is not sufficient for this, because the PPX flags cannot be made dev-only at the moment.