Don't bundle ppx_inline_test tests in release mode

[g2p]

These tests can introduce extra dependencies and bloat release binaries.
Don't build them when installing from OPAM.
See janestreet/ppx_inline_test#13 (comment)

[rgrinberg]

@diml what do you think of just hard coding -inline-test-drop in production for now as you said in janestreet/ppx_inline_test#13 (comment)

Later on, we could improve the ENV stanza to perhaps control this feature.

[ghost]

No more hard-coding please, however we can add support for release flags in ppx rewriters. For instance we could write this in the dune file of ppx_inline_test:

(library
 ...
 (kind (ppx_rewriter (release_flags (-inline-test-drop))))

[rgrinberg]

Okay, but I would suggest a syntax that is more consistent with how we handle env:

(library
 ...
 (kind (ppx_rewriter ((dev ...) (release (flags (-inline-test-drop))))

[ghost]

BTW, we were talking about this with @avsm the other day. Another way to sort this out would be to use variants and have a test variant. For instance the method above wouldn't work when the tests are written in a library inside a functor, while it would work with variants. So we should probably wait a bit before committing to this.

[rgrinberg]

That seems alright as well. Although it would also create a lot of installed bloat if we're now including a variant for every single library with tests.

Why doesn't the approach work with a functor btw? -inline-test-drop will not drop attributes inside functors?

[ghost]

Let's say Foo.Make from library foo includes inline tests and you use Foo.Make in library bar. The release flag method will only work if both foo and bar are part of the same workspace. if foo was previously installed, then it won't work. Thought maybe that's an acceptable restriction.

BTW, instead of specifying multiple flags, we could also just pass the build profile:

(library
 ...
 (kind (ppx_rewriter (flags (-profile %{profile}))))

[hannesm]

I ran into this issue when I used https://github.com/janestreet/ppx_inline_test and wanting to compile with a custom runtime, noticing that ppx_inline_test actually carries along some dependencies that require more Unix symbols than we have on MirageOS:

ld: error: undefined symbol: caml_out_channel_pos_fd
>>> referenced by expect_test_collector.ml:91 (collector/expect_test_collector.ml:91)
>>>               _build/main.native.o:(.text+0x98015)

ld: error: undefined symbol: expect_test_collector_before_test
>>> referenced by expect_test_collector.ml:96 (collector/expect_test_collector.ml:96)
>>>               _build/main.native.o:(.text+0x98092)

ld: error: undefined symbol: expect_test_collector_after_test
>>> referenced by expect_test_collector.ml:121 (collector/expect_test_collector.ml:121)
>>>               _build/main.native.o:(.text+0x98286)

ld: error: undefined symbol: Base_hash_string
>>> referenced by string.mli:329 (src/string.mli:329)
>>>               _build/main.native.o:(.text+0x9E678)
...

is the feature mentioned above on the roadmap of dune? would be great to have inline tests and being able to run on MirageOS! :)

[ghost]

To keep things simple, let's pass the profile via a cookie, just as we do for the library name. Then we can simply change ppx_inline_test to inspect this cookie.

[ghost]

Actually we should only pass it if a ppx requests it, to avoid extra rebuilds when not needed. I'm creating a ticket to describe the feature.

[ghost]

I submitted a concrete proposal in #2020.

[hannesm]

Now that > 1.5 years passed, and some work references this issue or 2020, I wonder whether it is possible to have inline tests which do not lead to runtime-dependencies in release mode?

[ghost]

The last bit to make this work, i.e. respecting Dune's setting in ppx_inline_test hasn't been done. I just wrote a patch for it: janestreet/ppx_inline_test#21

It's currently being reviewed in Jane Street internal repo. Once it's merged, I'll backport it to the last release of ppx_inline_test.

[hannesm]

thanks jeremie :)

[hannesm]

does this mean that e.g. ppx_expect will need a similar patch (from my dune file, I use (inline_tests) and (preprocess (pps ppx_expect))?

[ghost]

Nope, ppx_expect depends on ppx_inline_test and shares its frontend. So only ppx_inline_test needs to be updated.

Incidentally, we were talking last week with @NathanReb about forking ppx_expect into a simpler and leaner project. The current split ppx_inline_test/ppx_expect is because of historical reasons and not really needed anymore.

[ghost]

New release of ppx_inline_test submitted: ocaml/opam-repository#15933

At this point there is nothing more to do on the Dune side, so I'm closing this ticket.

[hannesm]

@diml my intuition is that since ppx_inline_test v0.13.1, a dune build --profile=release should avoid any ppx_expect and ppx_inline_test.runtime-deps dependencies, is this correct? what is the minimal dune version to use here?

My experience, as stated in ocaml/opam-repository#16097 (comment) is different. Am I missing anything? Did you check a complete example that the dependencies are avoided?

[ghost]

Not exactly, it makes sure the tests themselves are not part of the final executable but the code still needs to be preprocessed and there is currently no mechanism to avoid the runtime dependencies. Note that the code still needs to be preprocessed because if we leave the let%expect_test blocks in the code the compiler will choke on them.

Our plan with @NathanReb is to first add a new syntax in ppx_inline_test to have test blocks that can be safely left uninterpreted in the code. This will rely on attributes since they are ignored by the compiler. The syntax will be:

[@@@test
 ...
]

For instance:

[@@@test
 let some_helper = ...
 let%expect_test = ...
]

Then we will need a new feature in dune to have testing ppx rewriters be conditionally applied.