--conditional meaning and documentation (remove --conditional from README)

[mobileink]

The readme says Because of the --conditional flag, preprocessing is enabled only when BISECT_ENABLE=yes is set in the environment, so it is off by default.

I can't parse that. Reading the code, it looks to me like it's just wrong. BISECT_ENABLE=YES enables; so does --conditional. It looks like --conditional should be --enable, and either it or the env can be used to enable.

Am I misreading the code? Could be, I'm pretty new to OCaml.

Thanks,

Gregg

[aantron]

What code are you reading?

bisect_ppx/src/ppx/register.ml

Lines 11 to 22 in a4b8cb6

	let enabled () =
	match !conditional with
	| false ->
	`Enabled
	| true ->
	match Sys.getenv "BISECT_ENABLE" with
	| exception Not_found ->
	`Disabled
	| s when (String.uppercase [@ocaml.warning "-3"]) s = "YES" ->
	`Enabled
	| _ ->
	`Disabled

In this code, when --conditional is supplied, instrumentation is only enabled if BISECT_ENABLE is set and set to YES (the comparison is case-insensitive).

[mobileink]

I did mention I'm a noob. ;) I misread '!' as the negation operator rather than the defer operator. Sorry about that.

In any case, is the following accurate?

By default, bisect_ppx is disabled, which means it runs the identity transform. To enable it, you must do the following at runtime: a) pass --conditional, and b) set the env. var BISECT_ENABLE to 'YES' (case insensitive).

If it is accurate, I suggest adding something like it to the readme for the benefit of us newbies. The "at runtime" bit is important - I tried to pass the flag to the compiler at first.

Thanks,

G

[aantron]

There is indeed a problem with that part of the README, but the issue is that it is recommending an obsolete way of triggering Bisect_ppx (using --conditional), while (1) the earlier part of the README recommends a newer way of doing it and (2) there is an even better integration with Dune being tracked in #323, which should replace all the Dune instructions in the README, and why I didn't update it earlier :)

It might help to see either...

1. The older proper Dune instructions for using --conditonal: https://github.com/aantron/bisect_ppx/tree/7dbc180b0bb99fd4811161db304553f00608b711#Dune
2. The current dune file of the example project Markup.ml, https://github.com/aantron/markup.ml/blob/7e2adca8389543b8a7335a12c57bff38a797640a/src/dune, which no longer uses --conditional, but follows the Dune instructions in the current README.

In any case, is the following accurate?

By default, bisect_ppx is disabled, which means it runs the identity transform. To enable it, you must do the following at runtime: a) pass --conditional, and b) set the env. var BISECT_ENABLE to 'YES' (case insensitive).

Almost. By default, if you supply (preprocess (pps bisect_ppx)) at all, Bisect_ppx is enabled. However, if you instead supply
(preprocess (pps bisect_ppx --conditional)), Bisect_ppx is disabled, unless BISECT_ENABLE is set to YES in the environment, and not at runtime, but rather at build time.

If it is accurate, I suggest adding something like it to the readme for the benefit of us newbies. The "at runtime" bit is important - I tried to pass the flag to the compiler at first.

It is fairly accurate, but, I will make this into an issue about updating the Markup.ml example to match the current Dune instructions, rather than clarify the use of the older --conditional (which was originally clarified in the README, where I linked).

Thanks for pointing out this pretty major inconsistency in the README :)

[aantron]

(...or if we merge the newest Dune integration first, I'll just replace both parts of the README with the latest instructions.)

[mobileink]

On Sat, Sep 26, 2020 at 9:15 AM Anton Bachin ***@***.***> wrote: Almost. By default, if you supply (preprocess (pps bisect_ppx)) at all, Bisect_ppx is *enabled*.

This is something Dune does, correct? I'm not using Dune, I'm writing Bazel build rules, which is why I need detailed knowledge of how ppx stuff works. Thanks, G

…

[aantron]

I can immediately add the detail that the flag is added to the command line of the PPX rewriter (the Bisect_ppx binary run at build time, often also includes any other PPXs used by the project). The environment variable is also checked at build time by that same binary when it runs. But if you are writing rules for another build system, and it allows you to simply not run the PPX rewriter unless coverage is requested, do that instead. All these flags and environment variables are due to limitations of previous versions of Dune, which would run the rewriter unconditionally, so we had to make the rewriter itself check conditions.

[aantron]

The code in src/ppx is built into a PPX library, which is linked by Dune into a PPX binary in each user's project. The reason for that is because users typically run multiple PPXs, and there is a performance advantage to linking them all into one binary. It is this binary which takes the --conditional argument in a Dune build.

The older way was for Bisect_ppx, and each other PPX, to build its own PPX binary, and all these binaries would be triggered one after another by the compiler. I think in that case you would have passed the option as part of the -ppx argument, or by running the PPX binary yourself directly, and the compiler on its output.

Bisect_ppx still builds a modified standalone binary for Bucklescript users in src/ppx/bucklescript/ppx.exe. That binary has the --conditional flag hardcoded and some other peculiarities, though.

If you want to see what Dune is doing, you can take a Dune project that uses Bisect_ppx and run dune build --verbose (IIRC). Most of the commands you see will be repeatable directly in your shell. You will see where the built PPX binary is triggered and its path. Then, you can, for example, run it yourself, pass --help to it, and see --conditional listed in the help output. That should help with writing rules for Bazel or any other build system.