feature: remove limitations on [enabled_if] on libraries by rgrinberg · Pull Request #10250 · ocaml/dune

rgrinberg · 2024-03-11T20:29:01Z

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:

Expander depends on scope, and we need enabled_if to compute the scope
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 · 2024-03-12T19:43:07Z

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

jchavarri

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

test/blackbox-tests/test-cases/enabled_if/eif-library-name-collision.t

src/dune_rules/scope.ml

src/dune_rules/expander0.ml

rgrinberg · 2024-03-13T18:27:53Z

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:

Expanding a percent form - adds a dependency on the public API of the expander (pforms, templates, projects, etc)
Implementing an expander - requires knowing all the types of data that can exist inside an expander (scopes, artifacts, etc)
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.

src/dune_rules/super_context.ml

test/blackbox-tests/test-cases/enabled_if/enabled-if-lib-other-vars.t

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

@frejsoya

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)

@frejsoya

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)

rgrinberg added this to the 3.15.0 milestone Mar 11, 2024

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 requested a review from jchavarri March 12, 2024 19:42

jchavarri reviewed Mar 13, 2024

View reviewed changes

src/dune_rules/super_context.ml Show resolved Hide resolved

test/blackbox-tests/test-cases/enabled_if/eif-library-name-collision.t Outdated Show resolved Hide resolved

src/dune_rules/scope.ml Outdated Show resolved Hide resolved

src/dune_rules/expander0.ml Show resolved Hide resolved

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

jchavarri approved these changes Mar 14, 2024

View reviewed changes

src/dune_rules/super_context.ml Show resolved Hide resolved

test/blackbox-tests/test-cases/enabled_if/enabled-if-lib-other-vars.t Show resolved Hide resolved

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

feature: remove limitations on [enabled_if] on libraries

c08d3d2

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

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 merged commit 1b794da into main Mar 15, 2024

rgrinberg deleted the ps/rr/feature__remove_limitations_on__enabled_if__on_libraries branch March 15, 2024 22:22

Leonidas-from-XIV mentioned this pull request Mar 26, 2024

[new release] dune (15 packages) (3.15.0~alpha1) ocaml/opam-repository#25585

Closed

Leonidas-from-XIV mentioned this pull request Apr 3, 2024

[new release] dune (15 packages) (3.15.0) ocaml/opam-repository#25615

Merged

anmonteiro mentioned this pull request Apr 5, 2024

Allow rules with directory targets to be disabled (again) #10382

Merged

moyodiallo mentioned this pull request Sep 23, 2024

fix: enabled_if with env variable #10936

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

feature: remove limitations on [enabled_if] on libraries#10250
rgrinberg merged 1 commit intomainfrom
ps/rr/feature__remove_limitations_on__enabled_if__on_libraries

rgrinberg commented Mar 11, 2024 •

edited

Loading

Uh oh!

rgrinberg commented Mar 12, 2024

Uh oh!

jchavarri left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Mar 13, 2024

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rgrinberg commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgrinberg commented Mar 12, 2024

Uh oh!

jchavarri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Mar 13, 2024

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rgrinberg commented Mar 11, 2024 •

edited

Loading

jchavarri left a comment •

edited

Loading