Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow to declare (instrumentation) dependencies #4210

Merged
merged 4 commits into from
Feb 17, 2021
Merged

Allow to declare (instrumentation) dependencies #4210

merged 4 commits into from
Feb 17, 2021

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Feb 10, 2021

Fixes #3983

@aantron if you could give this one a spin that would be great. The syntax is as discussed in #3983.

@aantron
Copy link
Collaborator

aantron commented Feb 10, 2021

Looks good and seems to work.

ghost
ghost reviewed Feb 11, 2021
View reviewed changes
src/dune_rules/lib_rules.ml Outdated
in
(* Preprocess before adding the alias module as it doesn't need preprocessing *)
let pp =
Preprocessing.make sctx ~dir ~dep_kind ~scope ~preprocess ~expander
~preprocessor_deps:lib.buildable.preprocessor_deps
~preprocessor_deps:(lib.buildable.preprocessor_deps @ instrumentation_deps)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth sharing this code betweem exe_rules.ml and lib_rules.ml.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I added an extra argument to Preprocessing.make ~instrumentation_deps and do the concatentation inside that function.

ghost
ghost approved these changes Feb 11, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@nojb
(instrumentation): add support for (deps) field
7ebe68c 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Add test
7de3167 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Doc
f7afc9b 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
CHANGES.md
7f4a952 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Feb 17, 2021

Will merge this once CI is green.

@rgrinberg this should go into 2.8.3... to this end, should I change the version guard in the code from (3, 0) to (2, 8) ?

@rgrinberg
Copy link
Member

This is a new feature, so it cannot go into 2.8.3

@nojb
Copy link
Collaborator Author

nojb commented Feb 17, 2021

This is a new feature, so it cannot go into 2.8.3

Right, sorry, I got confused for a moment :)

@nojb nojb merged commit fb6ab57 into ocaml:main Feb 17, 2021
@nojb nojb deleted the instrumentation_deps branch February 17, 2021 20:23
@rgrinberg
Copy link
Member

If you'd like we could include it in 2.9 however. The PR will need to be modified slightly though.

nojb added a commit to nojb/dune that referenced this pull request Apr 7, 2021
@nojb
Allow to declare (instrumentation) dependencies (ocaml#4210)
80af4ea 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb added this to the 2.9 milestone Apr 7, 2021
@ejgallego ejgallego added this to Pending Backport in Release 2.9 Apr 8, 2021
@ejgallego
Copy link
Collaborator

Hi folks, deadline for submitting the backport of this PR is June 11th, please do so or remove the 2.9 milestone from this PR if you don't want this in 2.9 anymore.

nojb added a commit to nojb/dune that referenced this pull request Jun 2, 2021
@nojb
Allow to declare (instrumentation) dependencies (ocaml#4210)
4e5f66a 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Jun 2, 2021

PR backport at #4686

@nojb nojb removed this from Pending Backport PR in Release 2.9 Jun 2, 2021
@ejgallego
Copy link
Collaborator

PR backport at #4686

Thanks a lot @nojb !

nojb added a commit to nojb/dune that referenced this pull request Jun 2, 2021
@nojb
Allow to declare (instrumentation) dependencies (ocaml#4210)
b046104 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
ejgallego added a commit that referenced this pull request Jun 10, 2021
@nojb @ejgallego
Allow to declare (instrumentation) dependencies (#4210) (#4686)
900e7a9 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Co-authored-by: Emilio Jesús Gallego Arias <e+git@x80.org>
@ejgallego ejgallego mentioned this pull request Jun 21, 2021
Merged
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 21, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
da0714f 
…ator, dune-private-libs, dune and dune-build-info (2.9-rc1)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 29, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
a13f842 
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jul 1, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
96c14a5 
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon Awaiting requested review from emillon

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.9
Development

Successfully merging this pull request may close these issues.

Is it possible to specify dependencies for an instrumenter?
4 participants
@nojb @aantron @rgrinberg @ejgallego