Add support for passing arguments to instrumentation backends #3932

nojb · 2020-11-10T08:38:26Z

It implements the syntax suggested in #3906, not sure if we want to do something else instead.

Review appreciated.

ghost

LGTM!

nojb · 2020-11-10T16:42:59Z

@aantron If you could give this one a try with bisect_ppx that would be great. Otherwise, we will merge in a few days.

rgrinberg · 2020-11-10T18:21:29Z

Needs a CHANGES + manual update as well

nojb · 2020-11-10T21:37:56Z

Needs a CHANGES + manual update as well

Indeed, will do those shortly. Thanks!

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

nojb · 2020-11-11T06:34:57Z

Needs a CHANGES + manual update as well

Indeed, will do those shortly. Thanks!

This is now done.

nojb · 2020-11-12T16:26:34Z

I realized the new feature was not versioned; should I add a guard for version >= 2.8 @rgrinberg ?

rgrinberg · 2020-11-12T17:03:53Z

Indeed you should.

aantron · 2020-11-12T18:35:05Z

Thanks! I will try this shortly.

aantron · 2020-11-27T09:31:19Z

Is it possible to specify dependencies for an instrumenter? For example, in Bisect, we previously recommended using (preprocessor_deps ...) to give the filename of the exclusions file, i.e.

(preprocess (pps bisect_ppx --exclusions bisect.exclude))
(preprocessor_deps (file bisect.exclude))

Doing this now with

(instrumentation (backend bisect_ppx --exclusions bisect.exclude))
(preprocessor_deps (file bisect.exclude))

results in

5 |  (preprocessor_deps (file bisect.exclude))
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This preprocessor_deps field will be ignored because no preprocessor
that might use them is configured.

nojb · 2020-11-28T05:14:39Z

Is it possible to specify dependencies for an instrumenter?

I created a new issue with this #3983, I propose we continue discussion there.

@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)

bobzhang · 2021-06-23T13:56:58Z

src/dune_rules/preprocess.ml

        | Pps { loc; pps; flags; staged } ->
          let pps =
            With_instrumentation.Instrumentation_backend libname :: pps
          in
-          Pps { loc; pps; flags; staged }
+          Pps { loc; pps; flags = flags @ flags'; staged }


This seems to be wrong when I have an instrumentation backend and pps at the same time.
In that case, when instrumentation is off, that flags are still passed to pps which caused a build error
@jeremiedimino

It indeed looks like we are adding instrumentation flags unconditionally. Thanks @bobzhang for uncovering this and pin-pointing the root of the issue. Do you mind opening an issue about this so that we can track it?

Fix merged, will be in 2.9

Great, thanks @nojb for the prompt fix!

ghost approved these changes Nov 10, 2020

View reviewed changes

nojb added 4 commits November 11, 2020 06:07

Add support for passing arguments to instrumentation backends

6c10ed5

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Update tests

947e923

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

CHANGES.md

05e132d

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

Update manual

1e93f79

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

nojb force-pushed the instrumentation_arguments branch from f4ef049 to 1e93f79 Compare November 11, 2020 06:34

nojb requested review from emillon and rgrinberg as code owners November 11, 2020 06:34

rgrinberg approved these changes Nov 11, 2020

View reviewed changes

nojb merged commit 5e03523 into ocaml:master Nov 12, 2020

nojb deleted the instrumentation_arguments branch November 12, 2020 06:34

nojb mentioned this pull request Nov 12, 2020

Add version check for instrumentation parameters #3940

Merged

aantron mentioned this pull request Nov 12, 2020

Update "advanced" help with new argument passing instructions aantron/bisect_ppx#356

Closed

nojb mentioned this pull request Nov 28, 2020

Is it possible to specify dependencies for an instrumenter? #3983

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

bobzhang reviewed Jun 23, 2021

View reviewed changes

nojb mentioned this pull request Jun 23, 2021

Bugfix: only add instrumentation flags if instrumentation enabled #4770

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for passing arguments to instrumentation backends #3932

Add support for passing arguments to instrumentation backends #3932

nojb commented Nov 10, 2020

ghost left a comment

nojb commented Nov 10, 2020

rgrinberg commented Nov 10, 2020

nojb commented Nov 10, 2020

nojb commented Nov 11, 2020

nojb commented Nov 12, 2020

rgrinberg commented Nov 12, 2020

aantron commented Nov 12, 2020

aantron commented Nov 27, 2020

nojb commented Nov 28, 2020

bobzhang Jun 23, 2021 •

edited

Loading

ghost Jun 23, 2021

nojb Jun 23, 2021

nojb Jun 23, 2021

ghost Jun 24, 2021

Add support for passing arguments to instrumentation backends #3932

Add support for passing arguments to instrumentation backends #3932

Conversation

nojb commented Nov 10, 2020

ghost left a comment

Choose a reason for hiding this comment

nojb commented Nov 10, 2020

rgrinberg commented Nov 10, 2020

nojb commented Nov 10, 2020

nojb commented Nov 11, 2020

nojb commented Nov 12, 2020

rgrinberg commented Nov 12, 2020

aantron commented Nov 12, 2020

aantron commented Nov 27, 2020

nojb commented Nov 28, 2020

bobzhang Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

ghost Jun 23, 2021

Choose a reason for hiding this comment

nojb Jun 23, 2021

Choose a reason for hiding this comment

nojb Jun 23, 2021

Choose a reason for hiding this comment

ghost Jun 24, 2021

Choose a reason for hiding this comment

bobzhang Jun 23, 2021 •

edited

Loading