add flexibility to compile a test executable #3747

lubegasimon · 2020-08-26T22:11:00Z

PR opened in favour of closing #3679

shonfeder · 2020-08-27T02:58:13Z

Nice work! For the one failing test (https://github.com/ocaml/dune/pull/3747/checks?check_run_id=1033774622) I think you can fix it by running make fmt and committing the result :)

shonfeder

Looks great! I left some feedback. It's all just on the style/readability front, so apply at your own discretion :)

The one change I would request is making sure to update the documentation to record this new field!

shonfeder · 2020-08-27T03:02:32Z

src/dune_rules/inline_tests.ml

@@ -281,7 +283,10 @@ include Sub_system.Register_end_point (struct
      Compilation_context.create () ~super_context:sctx ~expander ~scope
        ~obj_dir ~modules ~opaque:(Explicit false) ~requires_compile:runner_libs
        ~requires_link:(lazy runner_libs)
-        ~flags:(Ocaml_flags.of_list [ "-w"; "-24"; "-g" ])
+        ~flags:


Now that this is long enough that it goes across 3 lines, it would be more readable (imho) to use a temporary variable before the call to Compilation_context.create like

let flags = Ocaml_flags.append_common. ... in Compilation_context.create () ~super_context:sctx ... ~flags ... <etc.>

Of course, this may just be a matter of taste. Feel free to disregard if you disagree on whether it's more readable.

shonfeder · 2020-08-27T03:03:57Z

src/dune_rules/super_context.ml

@@ -288,10 +288,10 @@ let build_dir_is_vendored build_dir =
  in
  Option.value ~default:false opt

-let ocaml_flags t ~dir (x : Dune_file.Buildable.t) =
+let ocaml_flags t ~dir (f : Ocaml_flags.Spec.t) =


Since f is often used for functions, it might be clearer to have this be

Suggested change

let ocaml_flags t ~dir (f : Ocaml_flags.Spec.t) =

let ocaml_flags t ~dir (spec : Ocaml_flags.Spec.t) =

shonfeder · 2020-08-27T03:04:13Z

src/dune_rules/super_context.ml

  let expander = Env_tree.expander t.env_tree ~dir in
  let flags =
-    Ocaml_flags.make ~spec:x.flags
+    Ocaml_flags.make ~spec:f


In which case, this could just be

Suggested change

Ocaml_flags.make ~spec:f

Ocaml_flags.make ~spec

test/blackbox-tests/test-cases/inline_tests/compile-flags.t/dune-file-with-valid-flag/dune

rgrinberg · 2020-08-27T07:14:06Z

src/dune_rules/ocaml_flags.ml

@@ -84,6 +84,13 @@ module Spec = struct
    and+ native = field_oslu "ocamlopt_flags" in
    let specific = Mode.Dict.make ~native ~byte in
    { common; specific }
+
+  let decode_compile_flags_field field_name =


I think this function should be defined near where it's called. I only see it being used in one place.

On the other hand, @rgrinberg, isn't it a pretty reasonable extension to the interface of the Ocaml_flags module? It seems like it could be of general use, but if we hide it away in the inline_tests module, anyone looking for this functionality in the future are likely to miss the function and reimplement themselves.

IMO, the "defined near call site" maxim should be counter-balanced by the generality of the function. and this decoder seems quite general. I wonder if the Right (tm) approach here isn't to generalize the decode function above by supporting optional parameters for the names of the fields, and then just use that generalized decode function at all call sites?

If you can find a way to generalize this function to use it in more than one place, that's certainly preferable. Let's just not mix up refactoring and feature development. Therefore, such an improvement should come in a separate PR.

Note that the type of Ocaml_flags.Spec.t is opaque. So just moving this function near the site of use is not really an option without worse compromises (needless exposure of the value constructors, making the API of Spec more brittle). It seem then that the only real option here is to open a separate PR that generalizes the Ocaml-flags.Spec.decode function, and make that a prerequisite for this PR.

Well the idea is that we can customize compilation, then almost field that is available in the executable stanza should be available. It follows that the name should be the same in both situations. So exe is supposed to reflect that some configurations from executable are permitted.

Perhaps we should call the field executable then, and then document that this supports a subset of the fields allowed in the executable stanza?

@lubegasimon you can try this improvement if you'd like. Otherwise, I can give it a shot.

cool, no problem, I'll do it. @rgrinberg

(inline_tests
(backend ..)
(exe
(flags ..)))

@rgrinberg, won't another flags field bring about confusion because we already have a one optional flags field for building a test executable ( https://dune.readthedocs.io/en/stable/tests.html#passing-special-arguments-to-the-test-runner ) ?

That's why I like putting it under (executable ..) The following doesn't seem confusing to me:

(inline_tests (flags ..) (executable (flags ..)))

Although I can see that the other flags field would have been better named as runtime_flags or something.

rgrinberg · 2020-08-27T07:15:20Z

src/dune_rules/inline_tests.ml

@@ -183,6 +184,7 @@ include Sub_system.Register_end_point (struct
        (let+ loc = loc
         and+ deps = field "deps" (repeat Dep_conf.decode) ~default:[]
         and+ flags = Ordered_set_lang.Unexpanded.field "flags"
+         and+ compile_flags = Ocaml_flags.Spec.decode_compile_flags_field "compile_flags"


This should be guarded behind a 2.8 version check.

it's not clear what you mean here @rgrinberg.

Allow me to clarify:

In dune, we may not introduce new features without guarding them behind the version of the dune language in which they were introduced. In this particular case, we are introducing a new compile_flags field in version 2.8 of dune. Hence, it should only be available once the user selected this version in the project file: (lang dune 2.8).

So you need a check like this field "compile_flag" (Dune_lang.Syntax.since syntax (2, 8) >>> ..) when defining this field. See dune_file.ml for instances of such checks.

Once you rebase (2, 8) should now be available and you'll be able to add the syntax check on (2, 8) above.

shonfeder

Oop, I forgot to push "submit" on this reply I wrote several days back.

shonfeder · 2020-08-29T15:42:31Z

src/dune_rules/ocaml_flags.ml

@@ -84,6 +84,13 @@ module Spec = struct
    and+ native = field_oslu "ocamlopt_flags" in
    let specific = Mode.Dict.make ~native ~byte in
    { common; specific }
+
+  let decode_compile_flags_field field_name =


On the other hand, @rgrinberg, isn't it a pretty reasonable extension to the interface of the Ocaml_flags module? It seems like it could be of general use, but if we hide it away in the inline_tests module, anyone looking for this functionality in the future are likely to miss the function and reimplement themselves.

IMO, the "defined near call site" maxim should be counter-balanced by the generality of the function. and this decoder seems quite general. I wonder if the Right (tm) approach here isn't to generalize the decode function above by supporting optional parameters for the names of the fields, and then just use that generalized decode function at all call sites?

rgrinberg · 2020-09-09T22:43:46Z

I think you need to make the executable field optional like this:

diff --git a/src/dune_rules/inline_tests.ml b/src/dune_rules/inline_tests.ml
index 6bb4a2af4..cec2a82b8 100644
--- a/src/dune_rules/inline_tests.ml
+++ b/src/dune_rules/inline_tests.ml
@@ -186,7 +186,7 @@ include Sub_system.Register_end_point (struct
          and+ flags = Ordered_set_lang.Unexpanded.field "flags"
          and+ executable =
            Dune_lang.Syntax.since Stanza.syntax (2, 8)
-           >>> field "executable"
+           >>> field "executable" ~default:Ocaml_flags.Spec.standard
                  (fields (Ocaml_flags.Spec.decode_compile_flags_field "flags"))
          and+ backend = field_o "backend" (located Lib_name.decode)
          and+ libraries =

This way you won't have to specify in all those other tests.

rgrinberg · 2020-09-09T22:45:37Z

After changing the language version in the root dune-project file, you should do a clean build. That is, you must delete ./dune.exe. Perhaps that is why you're seeing the extra

+  Error: Field 'inline_tests' is only available since version 2.8 of the dune
+  language. Please update your dune-project file to have (lang dune 2.8).

errors.

lubegasimon · 2020-09-10T06:24:57Z

After changing the language version in the root dune-project file, you should do a clean build.

the language version is currently 2.8

dune/dune-project

Line 1 in 75b0184

(lang dune 2.8)

so am still getting

Error: Field 'inline_tests' is only available since version 2.8 of the dune

language. Please update your dune-project file to have (lang dune 2.8).

lubegasimon · 2020-09-10T18:29:36Z

After changing the language version in the root dune-project file, you should do a clean build. That is, you must delete ./dune.exe. Perhaps that is why you're seeing the extra
+  Error: Field 'inline_tests' is only available since version 2.8 of the dune
+  language. Please update your dune-project file to have (lang dune 2.8).
errors.

order here mattered, because I was guarding a field before it was parsed, so the check had to catch inline_tests, which is why it appeared in the error message ( sorry if am not using the terminologies right ). The right order is

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

emillon · 2020-09-11T11:39:06Z

doc/tests.rst

+In some situations, some control on how the test executable is built is needed.
+In such cases, it’s possible to customize a subset of compilation options for an


Suggested change

In some situations, some control on how the test executable is built is needed.

In such cases, it’s possible to customize a subset of compilation options for an

To control how the test executable is built, it’s possible to customize a subset of compilation options for an

Seems a bit lighter like this?

rgrinberg

Looks good.

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg · 2020-09-12T03:33:59Z

Merging. I squashed the commits and added a CHANGES entry. Thanks for all your hard work.

lubegasimon · 2020-09-12T06:24:17Z

Merging. I squashed the commits and added a CHANGES entry. Thanks for all your hard work.

Am happy to, thanks.

@mrmr1993

…ne-action-plugin, dune-private-libs and dune-glob (2.8.0) CHANGES: - `dune rules` accepts aliases and other non-path rules (ocaml/dune#4063, @mrmr1993) - Action `(diff reference test_result)` now accept `reference` to be absent and in that case consider that the reference is empty. Then running `dune promote` will create the reference file. (ocaml/dune#3795, @bobot) - Ignore special files (BLK, CHR, FIFO, SOCKET), (ocaml/dune#3570, fixes ocaml/dune#3124, ocaml/dune#3546, @ejgallego) - Experimental: Simplify loading of additional files (data or code) at runtime in programs by introducing specific installation sites. In particular it allow to define plugins to be installed in these sites. (ocaml/dune#3104, ocaml/dune#3794, fixes ocaml/dune#1185, @bobot) - Move all temporary files created by dune to run actions to a single directory and make sure that actions executed by dune also use this directory by setting `TMPDIR` (or `TEMP` on Windows). (ocaml/dune#3691, fixes ocaml/dune#3422, @rgrinberg) - Fix bootstrap script with custom configuration. (ocaml/dune#3757, fixes ocaml/dune#3774, @marsam) - Add the `executable` field to `inline_tests` to customize the compilation flags of the test runner executable (ocaml/dune#3747, fixes ocaml/dune#3679, @lubegasimon) - Add `(enabled_if ...)` to `(copy_files ...)` (ocaml/dune#3756, @nojb) - Make sure Dune cleans up the status line before exiting (ocaml/dune#3767, fixes ocaml/dune#3737, @alan-j-hu) - Add `{gitlab,bitbucket}` as options for defining project sources with `source` stanza `(source (<host> user/repo))` in the `dune-project` file. (ocaml/dune#3813, @rgrinberg) - Fix generation of `META` and `dune-package` files when some targets (byte, native, dynlink) are disabled. Previously, dune would generate all archives for regardless of settings. (ocaml/dune#3829, ocaml/dune#4041, @rgrinberg) - Do not run ocamldep to for single module executables & libraries. The dependency graph for such artifacts is trivial (ocaml/dune#3847, @rgrinberg) - Fix cram tests inside vendored directories not being interpreted correctly. (ocaml/dune#3860, fixes ocaml/dune#3843, @rgrinberg) - Add `package` field to private libraries. This allows such libraries to be installed and to be usable by other public libraries in the same project (ocaml/dune#3655, fixes ocaml/dune#1017, @rgrinberg) - Fix the `%{make}` variable on Windows by only checking for a `gmake` binary on UNIX-like systems as a unrelated `gmake` binary might exist on Windows. (ocaml/dune#3853, @kit-ty-kate) - Fix `$ dune install` modifying the build directory. This made the build directory unusable when `$ sudo dune install` modified permissions. (fix ocaml/dune#3857, @rgrinberg) - Fix handling of aliases given on the command line (using the `@` and `@@` syntax) so as to correctly handle relative paths. (ocaml/dune#3874, fixes ocaml/dune#3850, @nojb) - Allow link time code generation to be used in preprocessing executable. This makes it possible to use the build info module inside the preprocessor. (ocaml/dune#3848, fix ocaml/dune#3848, @rgrinberg) - Correctly call `git ls-tree` so unicode files are not quoted, this fixes problems with `dune subst` in the presence of unicode files. Fixes ocaml/dune#3219 (ocaml/dune#3879, @ejgallego) - `dune subst` now accepts common command-line arguments such as `--debug-backtraces` (ocaml/dune#3878, @ejgallego) - `dune describe` now also includes information about executables in addition to that of libraries. (ocaml/dune#3892, ocaml/dune#3895, @nojb) - instrumentation backends can now receive arguments via `(instrumentation (backend <name> <args>))`. (ocaml/dune#3906, ocaml/dune#3932, @nojb) - Tweak auto-formatting of `dune` files to improve readability. (ocaml/dune#3928, @nojb) - Add a switch argument to opam when context is not default. (ocaml/dune#3951, @tmattio) - Avoid pager when running `$ git diff` (ocaml/dune#3912, @AltGr) - Add `(root_module ..)` field to libraries & executables. This makes it possible to use library dependencies shadowed by local modules (ocaml/dune#3825, @rgrinberg) - Allow `(formatting ...)` field in `(env ...)` stanza to set per-directory formatting specification. (ocaml/dune#3942, @nojb) - [coq] In `coq.theory`, `:standard` for the `flags` field now uses the flags set in `env` profile flags (ocaml/dune#3931 , @ejgallego @rgrinberg) - [coq] Add `-q` flag to `:standard` `coqc` flags , fixes ocaml/dune#3924, (ocaml/dune#3931 , @ejgallego) - Add support for Coq's native compute compilation mode (@ejgallego, ocaml/dune#3210) - Add a `SUFFIX` directive in `.merlin` files for each dialect with no preprocessing, to let merlin know of additional file extensions (ocaml/dune#3977, @vouillon) - Stop promoting `.merlin` files. Write per-stanza Merlin configurations in binary form. Add a new subcommand `dune ocaml-merlin` that Merlin can use to query the configuration files. The `allow_approximate_merlin` option is now useless and deprecated. Dune now conflicts with `merlin < 3.4.0` and `ocaml-lsp-server < 1.3.0` (ocaml/dune#3554, @voodoos) - Configurator: fix a bug introduced in 2.6.0 where the configurator V1 API doesn't work at all when used outside of dune. (ocaml/dune#4046, @aalekseyev) - Fix `libexec` and `libexec-private` variables. In cross-compilation settings, they now point to the file in the host context. (ocaml/dune#4058, fixes ocaml/dune#4057, @TheLortex) - When running `$ dune subst`, use project metadata as a fallback when package metadata is missing. We also generate a warning when `(name ..)` is missing in `dune-project` files to avoid failures in production builds. - Remove support for passing `-nodynlink` for executables. It was bypassed in most cases and not correct in other cases in particular on arm32. (ocaml/dune#4085, fixes ocaml/dune#4069, fixes ocaml/dune#2527, @emillon) - Generate archive rules compatible with 4.12. Dune longer attempt to generate an archive file if it's unnecessary (ocaml/dune#3973, fixes ocaml/dune#3766, @rgrinberg) - Fix generated Merlin configurations when multiple preprocessors are defined for different modules in the same folder. (ocaml/dune#4092, fixes ocaml/dune#2596, ocaml/dune#1212 and ocaml/dune#3409, @voodoos) - Add the option `use_standard_c_and_cxx_flags` to `dune-project` that 1. disables the unconditional use of the `ocamlc_cflags` and `ocamlc_cppflags` from `ocamlc -config` in C compiler calls, these flags will be present in the `:standard` set instead; and 2. enables the detection of the C compiler family and populates the `:standard` set of flags with common default values when building CXX stubs. (ocaml/dune#3875, ocaml/dune#3802, fix ocaml/dune#3718 and ocaml/dune#3528, @voodoos)

lubegasimon requested review from a user, shonfeder and rgrinberg August 26, 2020 22:11

lubegasimon mentioned this pull request Aug 26, 2020

Compile flags #3679

Closed

lubegasimon changed the title ~~enable specifying flags when compiling binaries from inline tests~~ add compile flags feature Aug 26, 2020

lubegasimon self-assigned this Aug 26, 2020

shonfeder suggested changes Aug 27, 2020

View reviewed changes

rgrinberg reviewed Aug 27, 2020

View reviewed changes

lubegasimon force-pushed the inline-tests-compile-flags branch from 8ad4df2 to b73b3fa Compare September 1, 2020 06:20

shonfeder reviewed Sep 4, 2020

View reviewed changes

lubegasimon force-pushed the inline-tests-compile-flags branch 2 times, most recently from ac8420b to f798367 Compare September 4, 2020 19:37

lubegasimon force-pushed the inline-tests-compile-flags branch 2 times, most recently from 361b024 to 0e6205a Compare September 10, 2020 21:42

lubegasimon requested review from emillon and nojb as code owners September 10, 2020 23:05

lubegasimon added the enhancement label Sep 11, 2020

lubegasimon changed the title ~~add compile flags feature~~ add flexibility to compile a test executable Sep 11, 2020

lubegasimon added the ready for review label Sep 11, 2020

emillon reviewed Sep 11, 2020

View reviewed changes

rgrinberg approved these changes Sep 11, 2020

View reviewed changes

lubegasimon force-pushed the inline-tests-compile-flags branch from dd2a7ad to e64ae57 Compare September 11, 2020 23:05

lubegasimon and others added 2 commits September 11, 2020 20:29

Signed-off-by: lubegasimon <[email protected]>

67f5169

Fix formatting of snippets in tests.rst

851445e

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg force-pushed the inline-tests-compile-flags branch from e64ae57 to 851445e Compare September 12, 2020 03:33

rgrinberg merged commit bf02581 into ocaml:master Sep 12, 2020

lubegasimon deleted the inline-tests-compile-flags branch September 12, 2020 06:31

lubegasimon removed the ready for review label Sep 14, 2020

lubegasimon mentioned this pull request Sep 14, 2020

more flexibility for compiling binaries from inline tests #766

Closed

rgrinberg mentioned this pull request Jan 13, 2021

[new release] dune-build-info, dune, dune-configurator, dune-site, dune-action-plugin, dune-private-libs and dune-glob (2.8.0) ocaml/opam-repository#17967

Merged

bobot mentioned this pull request Feb 1, 2021

regression in inline tests #4177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add flexibility to compile a test executable #3747

add flexibility to compile a test executable #3747

lubegasimon commented Aug 26, 2020 •

edited

Loading

shonfeder commented Aug 27, 2020

shonfeder left a comment

shonfeder Aug 27, 2020

shonfeder Aug 27, 2020

shonfeder Aug 27, 2020

rgrinberg Aug 27, 2020

lubegasimon Aug 27, 2020

shonfeder Aug 29, 2020

rgrinberg Sep 4, 2020

shonfeder Sep 4, 2020

rgrinberg Sep 4, 2020

shonfeder Sep 4, 2020

lubegasimon Sep 4, 2020 •

edited

Loading

lubegasimon Sep 4, 2020 •

edited

Loading

rgrinberg Sep 4, 2020

rgrinberg Aug 27, 2020

lubegasimon Aug 27, 2020

rgrinberg Aug 28, 2020

rgrinberg Sep 3, 2020

shonfeder left a comment

shonfeder Aug 29, 2020

rgrinberg commented Sep 9, 2020

rgrinberg commented Sep 9, 2020

lubegasimon commented Sep 10, 2020 •

edited

Loading

lubegasimon commented Sep 10, 2020 •

edited

Loading

emillon Sep 11, 2020

lubegasimon Sep 11, 2020

rgrinberg left a comment

rgrinberg commented Sep 12, 2020

lubegasimon commented Sep 12, 2020

	let ocaml_flags t ~dir (f : Ocaml_flags.Spec.t) =
	let ocaml_flags t ~dir (spec : Ocaml_flags.Spec.t) =

		In some situations, some control on how the test executable is built is needed.
		In such cases, it’s possible to customize a subset of compilation options for an

	In some situations, some control on how the test executable is built is needed.
	In such cases, it’s possible to customize a subset of compilation options for an
	To control how the test executable is built, it’s possible to customize a subset of compilation options for an

add flexibility to compile a test executable #3747

add flexibility to compile a test executable #3747

Conversation

lubegasimon commented Aug 26, 2020 • edited Loading

shonfeder commented Aug 27, 2020

shonfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubegasimon Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

lubegasimon Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shonfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Sep 9, 2020

rgrinberg commented Sep 9, 2020

lubegasimon commented Sep 10, 2020 • edited Loading

lubegasimon commented Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg left a comment

Choose a reason for hiding this comment

rgrinberg commented Sep 12, 2020

lubegasimon commented Sep 12, 2020

lubegasimon commented Aug 26, 2020 •

edited

Loading

lubegasimon Sep 4, 2020 •

edited

Loading

lubegasimon Sep 4, 2020 •

edited

Loading

lubegasimon commented Sep 10, 2020 •

edited

Loading

lubegasimon commented Sep 10, 2020 •

edited

Loading