ocaml / dune

A composable build system for OCaml.
https://dune.build/
MIT License
1.56k stars 394 forks source link

ppx_inline_tests enables backtrace mode non-deterministically #10560

Open ejgallego opened 1 month ago

ejgallego commented 1 month ago

We have some expect Coq tests that produce output, the output will include backtrace information the flag is set in the runtime.

These tests live under the runtest alias, together with some inline_tests for the libraries.

Recently, we did the following reorganization:

Before : library Util, library Core that depends on Util. Core uses inline tests, test executable depends on Core After : we move the inline tests to Util, remove the inline tests from Core

Now, running the tests, before, they didn't contain backtrace information, after they do. That seems quite bizarre / non-deterministic?

emillon commented 1 month ago

any chance there's a Printexc.record_backtrace call in one the libraries' init code?

ejgallego commented 1 month ago

Nope, ppx_inline_test is supposed to do it tho, so it gets linked to the executable. I will submit a small reproduction test ASAP.

ejgallego commented 1 month ago

There you go: https://github.com/ejgallego/inline_tests_printexn (see commit message for repro instructions)

See the dune file there.

I guess there may be two static initializers that race?

Also regarding the discussion about whether dune should mess with backtrace info, I have realized that a lot of output tests we have will output different errors when backtraces are enabled, so indeed that's a bit of a PITA as there are times developing we want to respect the developer setting OCAMLRUNPARAMS, but if dune sets it up, we have no way to know who did it (the dev, then we should respect, dune, we need to disable it)

rgrinberg commented 3 weeks ago

If your test is sensitive to the backtrace, why don't you just add Printexc.record_backtrace foo to the top of your test? That way it will work no matter how backtraces were set outside. This way you don't have to know who is setting it.

In any case, I don't think there's a dune bug here. We don't touch the setting at the moment, and if we did it would be intentional so it would still not be a bug.

ejgallego commented 3 weeks ago

If your test is sensitive to the backtrace, why don't you just add Printexc.record_backtrace foo to the top of your test? That way it will work no matter how backtraces were set outside. This way you don't have to know who is setting it.

We could do something like that, but for existing projects this would require a large amount of work, including modifications on existing developer workflow, flags settings, etc... I am still not convinced the pain is worth the gain.

I still don't expect my build system to mess with runtime settings, but I admit I may be too old school here.

I don't think there's a dune bug here

If that's not a dune bug, where should we assign the bug?

rgrinberg commented 3 weeks ago

Well we already changing the runtime environment when running your binaries. We add everything in _build/install to your PATH for starters.

Likely the issue lies in one of the libraries that are being linked to your test. One of them is setting Printexc.record_backtrace and we do not know which one. Setting a breakpoint in gdb on this function and looking at the backtrace is probably the most promising way to debug this issue.

hhugo commented 3 weeks ago

This is a default in base https://github.com/janestreet/base/blob/b6430a41ba658335aaeff796b37c10b1d1803637/src/backtrace.ml#L29C1-L30C1

( We turn on backtraces by default if OCAMLRUNPARAM doesn't explicitly mention them. )

base is used by the runtime code of ppx_inline_tests

ejgallego commented 3 weeks ago

base is used by the runtime code of ppx_inline_tests

This I was aware, but why moving the tests to Util makes the status of the flag change?

To be clear, I'm not complaining that the flag is set or not set, I'm complaining that the flag value changes by moving the tests to another library.

So we have this setup:

Then if place the inline tests in Myapp the behavior is different that if place the tests in Util

hhugo commented 3 weeks ago

Your core lib doesn't depend on util so the compiler doesn't link it to build test.exe. Adding a dependency on util (such as adding an inline test that reference util) make the ocaml compiler link util (and transitively ppx_inline_test runtime lib and base)

ejgallego commented 3 weeks ago

Thanks @hhugo , in the original example core depends on util tho:

https://github.com/ejgallego/inline_tests_printexn/blob/959fadd8b70e5aa2d2e796cff176405fe3116c7b/dune#L10-L16

In my real example I'm sure core was calling util, but thanks for this info, I'll investigate on the example a little bit more. (moreover in my example, we use -linkall for the executable, but that wasn't needed to reproduce).

hhugo commented 3 weeks ago

in the original example core depends on util tho:

The dune dep is not what the compiler cares about, core doesn't depend (implementation wise) on util based on ocamlobjinfo core.cmxa