regression in inline tests

c-cube commented 3 years ago

Expected Behavior

Some tests must pass.

In tiny_httpd CI there is now a failure with inline tests (using the qtest backend) with dune 2.8.2, that does not occur with 2.7.0. It's a warn-error thing that I don't know how to remove as (flags) in the inline test stanza seems to be flags passed to the generated test executable, not to ocamlc while compiling the test executable (so I can't disable the warnings there).

Actual Behavior

warn-error again.

Reproduction

clone https://github.com/c-cube/tiny_httpd.git
dune runtest with dune 2.7.0 (should work after installing qcheck+qtest)
dune runtest with dune 2.8.2 (fails).

Specifications

Version of dune (output of dune --version): 2.7.0 vs 2.8.2
Version of ocaml (output of ocamlc --version): 4.08.1
Operating system (distribution and version): archlinux

failure looks like:

File "Tiny_httpd.ml", line 562, characters 0-20:
Error (warning 33): unused open Tiny_httpd_util.
File "Tiny_httpd.ml", line 568, characters 0-15:
Error (warning 33): unused open Tiny_httpd.

bobot commented 3 years ago

I looked at it rapidly. Dune 2.7.0 uses -w -24, dune 2.8.0 uses default flags of dune. I think it is due to #3747, specifically https://github.com/ocaml/dune/pull/3747/files#diff-8fac66f67061d79597bc62fcb3f33bbe9d886a93f2d8919c0bc372badb692bc2L283 (CC @lubegasimon)

@c-cube you can "fix" it by using the feature of #3747 which allows to set the flags used for the compilation of the inline test.

c-cube commented 3 years ago

I see. Thanks @bobot . Is there a nice way I can have conditional logic to do that only on recent versions of dune?

c-cube commented 3 years ago

strictly speaking this should have prompted a 3.0.

lubegasimon commented 3 years ago

Sorry @c-cube about that breakage. This issue caught my attention over the weekend, and I wished I didn’t have a pressing task then. Pushing it to 3.0 seems a bit tricky due to some internal plans, but enabling backward compatibility looks reasonable to me. I’ll work on it hopefully this week.

c-cube commented 3 years ago

Thank you @lubegasimon :-). Just restoring the old set of flags for inline tests should do it I think.

bobot commented 3 years ago

strictly speaking this should have prompted a 3.0

Of course, it is a bug that it was not protected by a 2.8 flag. But it is easy to fix, < 2.8 could have the old behavior, 2.8 the new; no need for 3.0. Of course perhaps generated code could be compiled with no warnings at all, or is it a bug of qtests that it opens module it don't use? We will discuss that later.

EDIT: with no warnings -> -w -all

c-cube commented 3 years ago

or is it a bug of qtests that it opens module it don't use?

it might be a small "bug", but dune breaking my build without a major version change is the bug we're discussing here :-)

lubegasimon commented 3 years ago

@c-cube, restoring the previous set of flags will bring other parts. The ideal solution I find to this is to create another field for example exec_flags within executable fields, rather than flags(to me, this is ideal). We shall then have this;

(library
 (name foo)
 (inline_tests
  (flags (-foo bar)
  (executable
   (exec_flags (-foo bar))))
  (preprocess (pps ppx_expect))))

rather than

(library
 (name foo)
 (inline_tests
  (flags (-foo bar)
  (executable
   (flags (-foo bar))))
  (preprocess (pps ppx_expect))))

Let see @rgrinberg’s suggestion here. cc @bobot @shonfeder

rgrinberg commented 3 years ago

Isn't it just a matter of changing the default value here?

         and+ executable =
           field "executable" ~default:Ocaml_flags.Spec.standard
             ( Dune_lang.Syntax.since Stanza.syntax (2, 8)
             >>> fields Ocaml_flags.Spec.decode )

We need a default value that matches the old value of -w -24 -g

emillon commented 7 months ago

We messed up by introducing an unversioned change in 2.8, but we're not going to try to fix this 3 years later. Sorry!

ocaml / dune