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

Bugfix: only add instrumentation flags if instrumentation enabled #4770

Merged
merged 4 commits into from
Jun 23, 2021
Merged

Bugfix: only add instrumentation flags if instrumentation enabled #4770

merged 4 commits into from
Jun 23, 2021

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Jun 23, 2021

Should fix #3932 (comment)

nojb added 2 commits June 23, 2021 18:47
@nojb
Use record type
29dc1ce 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
instrumentation: only add flags if the instrumentation is active
a4418c8 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb mentioned this pull request Jun 23, 2021
Merged
nojb added 2 commits June 23, 2021 19:02
@nojb
CHANGES.md
4861a46 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Add test
0685922 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Jun 23, 2021

@ejgallego is there time to cherry-pick this to 2.9 ?

@nojb nojb merged commit adff7bc into ocaml:main Jun 23, 2021
@nojb nojb deleted the fix_instrumentation_flags branch June 23, 2021 18:11
@ejgallego
Copy link
Collaborator

@ejgallego is there time to cherry-pick this to 2.9 ?

Yes, still waiting on the sync on the sites PR.

@ejgallego ejgallego added this to the 2.9 milestone Jun 23, 2021
@ejgallego ejgallego added this to Pending Approval in Release 2.9 via automation Jun 23, 2021
@ejgallego
Copy link
Collaborator

Anyways bugfixes like this could go into the 2.9.1 / 2.9.2 release too, so don't hesitate to submit the backport, worst case I'll go in the next point release

nojb added a commit to nojb/dune that referenced this pull request Jun 23, 2021
@nojb
Bugfix: only add instrumentation flags if instrumentation enabled (oc…
f844741 
…aml#4770)

* Use record type
* instrumentation: only add flags if the instrumentation is active
* CHANGES.md
* Add test

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb mentioned this pull request Jun 23, 2021
Merged
nojb added a commit that referenced this pull request Jun 23, 2021
@nojb
Bugfix: only add instrumentation flags if instrumentation enabled (#4770
87ec465 
) (#4771)

* Use record type
* instrumentation: only add flags if the instrumentation is active
* CHANGES.md
* Add test

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Jun 23, 2021

@ejgallego is there time to cherry-pick this to 2.9 ?

Yes, still waiting on the sync on the sites PR.

Thanks for the quick reply. I went ahead and cherry-picked the PR in #4771

@ejgallego ejgallego moved this from Pending Approval to Pending CI in Release 2.9 Jun 28, 2021
@ejgallego ejgallego moved this from Pending CI to Landed in both in Release 2.9 Jun 28, 2021
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 ejgallego mentioned this pull request Jun 29, 2021
Merged
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
No reviews
Assignees
No one assigned
Labels
None yet
Projects
No open projects
Release 2.9
Landed in both
Milestone
2.9
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nojb @ejgallego