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

feature: remove limitations on [enabled_if] on libraries #10250

Merged

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented Mar 11, 2024

Previously, the enabled_if field on libraries had a bunch of limitations. Basically, only variables that could be computed from the context could be used. E.g. profile, context_name, etc. This PR removes this limitation and now every variable is allowed provided that it doesn't introduce a memoization cycle.

Normally, such a feature would be trivial, but here we some module dependency cycles that prevent the natural solution from working. If it was possible, we would just call Super_context.expander ~dir sctx |> Expander.eval_blang everywhere and be done with it. Unfortunately, we have to solve two problems:

  1. Expander depends on scope, and we need enabled_if to compute the scope
  2. Super_context depends on scope, and therefore on various enabled_if through 1.

So we break this cycle by introducing an Expander0.t that doesn't depend on scope and has an expander db that is the equivalent of Super_context.expander.

@rgrinberg rgrinberg added this to the 3.15.0 milestone Mar 11, 2024
@rgrinberg rgrinberg force-pushed the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch 2 times, most recently from 90557ed to 6030170 Compare March 12, 2024 19:41
@rgrinberg rgrinberg requested a review from jchavarri March 12, 2024 19:42
@rgrinberg
Copy link
Member Author

@jchavarri since you've expressed interest in this PR and have looked at this code lately, I've added you as a reviewer. I've also improved the description of the implementation.

Copy link
Collaborator

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

I asked some questions, mostly trying to follow what's going on. Probably this change needs a changelog entry.

So we break this cycle by introducing an Expander0.t that doesn't depend on scope and has an expander db that is the equivalent of Super_context.expander.

I took a look at removing the cycle by moving lookup_artifacts out of Expander, but more cycles surfaced:

Error: Dependency cycle between:          
   _build/default/src/dune_rules/.dune_rules.objs/dune_rules__Scope.intf.all-deps
-> _build/default/src/dune_rules/.dune_rules.objs/dune_rules__Expander.intf.all-deps
-> _build/default/src/dune_rules/.dune_rules.objs/dune_rules__Library.intf.all-deps
-> _build/default/src/dune_rules/.dune_rules.objs/dune_rules__Library_redirect.intf.all-deps
-> _build/default/src/dune_rules/.dune_rules.objs/dune_rules__Deprecated_library_name.intf.all-deps
-> _build/default/src/dune_rules/.dune_rules.objs/dune_rules__Scope.intf.all-deps

I wonder if it'd be worth exploring that path more, as increasing the amount of code in Expander0 seems like a short term solution rather than something maintainable in the long term to me? Probably this feeling comes from code duplication and new state being added in set_db, which I understand could lead to situations where Expander and Expander0 could get "out of sync".

src/dune_rules/super_context.ml Show resolved Hide resolved
src/dune_rules/scope.ml Outdated Show resolved Hide resolved
src/dune_rules/expander0.ml Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch from 6030170 to 883f040 Compare March 13, 2024 18:22
@rgrinberg
Copy link
Member Author

I asked some questions, mostly trying to follow what's going on. Probably this change needs a changelog entry.

So we break this cycle by introducing an Expander0.t that doesn't depend on scope and has an expander db that is the equivalent of Super_context.expander.

I took a look at removing the cycle by moving lookup_artifacts out of Expander, but more cycles surfaced:

..

I wonder if it'd be worth exploring that path more, as increasing the amount of code in Expander0 seems like a short term solution rather than something maintainable in the long term to me? Probably this feeling comes from code duplication and new state being added in set_db, which I understand could lead to situations where Expander and Expander0 could get "out of sync".

You can try exploring it, but there are fundamental issues here that make it impossible to factor our code enough to prevent cycles. There's three different concerns:

  1. Expanding a percent form - adds a dependency on the public API of the expander (pforms, templates, projects, etc)
  2. Implementing an expander - requires knowing all the types of data that can exist inside an expander (scopes, artifacts, etc)
  3. Constructing a percent form - requires knowing how to make an expander and populate it with data. The data can come from just about everywhere (super_context, env_node, dir_contents, etc)

You can see how steps 2. and 3. are limiting. Anytime you want to expand something, pulling in 2 or 3 might not be an option.
So in the end we do need a way to expand things with only 1.

Another thing we can try is to pass an expansion function around to those that need to expand. That ends up creating a bunch of inconsistency bugs because this exapnsion function is being constructed in a slightly different way everywhere.

@rgrinberg rgrinberg force-pushed the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch 2 times, most recently from 826ec1c to eb86a85 Compare March 13, 2024 19:54
@rgrinberg rgrinberg force-pushed the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch from eb86a85 to 867cc55 Compare March 15, 2024 21:58
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: 1e2e990c-3ec5-41f1-a973-1509516ee987 -->
@rgrinberg rgrinberg force-pushed the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch from 867cc55 to c08d3d2 Compare March 15, 2024 22:21
@rgrinberg rgrinberg merged commit 1b794da into main Mar 15, 2024
22 of 25 checks passed
@rgrinberg rgrinberg deleted the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch March 15, 2024 22:22
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 26, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Apr 3, 2024
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants