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.

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

Support (package) in (mdx) #4691

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Support (package) in (mdx) #4691

merged 1 commit into from
Jun 8, 2021

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jun 3, 2021

This adds a (package) field to (mdx) with the usual semantics.

The syntax is not ideal since (packages) is already supported as sugar
to specify dependencies.

Closes #3756

@emillon
Copy link
Collaborator Author

emillon commented Jun 3, 2021

I had trouble implementing this while avoiding dependency cycles. the key problem is that the central check for "should we consider this stanza based on the current value of --only-packages?" is in Dune_file.t, and we can't refer to Mdx over there. This would make Dune_file.t depend on all the extensions and we don't want that.

To solve that I created a Package_proxy constructor that implements only package filtering for an arbitrary stanza. That works but it seems a bit heavy and could be error-prone. If we're happy with that, we could use it in more places. That could also be used to implement a generic (package ...) stanza that would wrap other stanzas.

An alternative is to do the same as in Test_rules and generate "fake" user rules (Dune_file.Rule.t). These are properly filtered by the existing code since they are defined in Dune_file.

The goal is to backport and include this feature in 2.9. It is also going to be added to the "new" version of the mdx stanza (0.2) in #3956, with more pleasing syntax. But this is a larger bit of work, and probably won't be backported.

Some remarks / TODO list:

  • update documentation and note the syntax quirk
  • find a solution to Package_proxy or become happy with it
  • update changelog

Let me know what you think. Thanks!

@emillon emillon added this to Pending Approval in Release 2.9 via automation Jun 3, 2021
@voodoos voodoos added this to the 2.9 milestone Jun 3, 2021
@emillon
Copy link
Collaborator Author

emillon commented Jun 4, 2021

Using user rules does not seem to be possible because of the way deps are encoded in a file. Another alternative seems to call Only_packages directly as done in Cram_rules. I'll try that.

@emillon
Copy link
Collaborator Author

emillon commented Jun 4, 2021

That looks better now.

@emillon emillon requested review from a user, NathanReb and voodoos and removed request for NathanReb June 4, 2021 10:30
@emillon
Copy link
Collaborator Author

emillon commented Jun 4, 2021

This is now ready for review.

Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

I did not see the previous version with Package_proxy so I cannot compare, but these changes looks good to me.

@emillon emillon force-pushed the mdx-package branch 3 times, most recently from a77fa54 to 933d708 Compare June 7, 2021 08:37
@ejgallego
Copy link
Collaborator

Hi folks, if we want this backported to 2.9 we should try to get it ready before Fri, cheers.

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.

The package/packages discrepancy is unfortunate indeed. Perhaps we could replace (packages x y) by (deps (package x) (package y)). Or finally move to one package per directory and get rid of package fields altogether.

This adds a (package) field to (mdx) with the usual semantics.

The syntax is not ideal since (packages) is already supported as sugar
to specify dependencies.

Closes ocaml#3756

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon merged commit 12b3723 into ocaml:main Jun 8, 2021
@emillon emillon deleted the mdx-package branch June 8, 2021 09:39
emillon added a commit to emillon/dune that referenced this pull request Jun 8, 2021
This adds a (package) field to (mdx) with the usual semantics.

The syntax is not ideal since (packages) is already supported as sugar
to specify dependencies.

See ocaml#4691

Closes ocaml#3756

Signed-off-by: Etienne Millon <me@emillon.org>
ejgallego pushed a commit that referenced this pull request Jun 10, 2021
This adds a (package) field to (mdx) with the usual semantics.

The syntax is not ideal since (packages) is already supported as sugar
to specify dependencies.

See #4691

Closes #3756

Signed-off-by: Etienne Millon <me@emillon.org>
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 21, 2021
…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
…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
…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
Labels
None yet
Projects
No open projects
Release 2.9
Pending Approval
Development

Successfully merging this pull request may close these issues.

Support attaching MDX stanzas to packages
3 participants