Allow expansion in `(modules)` field #9578

nojb · 2023-12-28T16:51:52Z

Currently (modules) has to be an "ordered set" consisting of literal string elements. This PR proposes to allow expansion to take place in this field, allowing in particular the use of (:include) and %{read-lines:} to specify lists of modules that depend on other artifacts built by Dune. Apart from representing a real improvement on the expresiveness of the build language, I have a feeling this change would also cover a considerable portion of use-cases of the OCaml syntax.

I suggest to start by looking at the test to understand what is being done in the PR.

The first commit is preparatory: it moves a submodule to avoid a dependency cycle in the second commit. The second commit contains the meat of the change:

Replace Ordered_set_lang.t by Ordered_set_lang.Unexpanded.t for all (modules) fields (eg (private_modules), (modules_without_implementation), etc).
Introduce an "expand" step in the function Modules_field_evaluator.eval (which now takes and Expander.t as argument). This means that we enter the Action_builder.t monad in this code; however, immediately after expansion we exit back to Memo by calling Action_builder.evaluate_and_collect_facts.
Currently the fake_modules logic in Modules_field_evaluator is disabled; I didn't know what this was for and the existing implementation used side-effects that wouldn't work well in the Memo monad.
Note that it is not possible to have the build-time dependencies of the (modules) field in the same directory as the containing stanza (see the test); however, the error (a dependency cycle in Memo) is not currently caught and Dune brutally crashes. How do I handle this error gracefully?

Suggestions for improvements or if I missed anything are warmly welcome! Thanks.

rgrinberg · 2023-12-28T22:20:32Z

I haven't reviewed the entire PR, but I can answer this one:

Currently the fake_modules logic in Modules_field_evaluator is disabled; I didn't know what this was for and the existing implementation used side-effects that wouldn't work well in the Memo monad.

The fake_modules logic is to prevent things like:

(library
 (modules foo)
 (modules_without_implementation bar))

Here modules_without_implementation refers to a module that isn't part of the library. The fake module check warns the user about that.

nojb · 2023-12-29T00:24:50Z

The fake_modules logic is to prevent things like:

Thanks! I pushed a commit ("Handle fake modules") to do this without using a reference; seems to be working again now.

rgrinberg · 2023-12-29T04:21:11Z

test/blackbox-tests/test-cases/modules-expansion.t

+        ; ("load-dir", In_build_dir "default")
+        ]
+    })
+  Raised at Memo.Exec.exec_dep_node.(fun) in file "src/memo/memo.ml", line


We don't include stack traces in the test output (they make the tests fail due to spurious reasons). See how we filter such stacktraces elsewhere with awk.

The stack trace was meant to document the issue to aid reviewing, but I hope we will be able to get rid of it before the PR is merged :)

I pushed a commit with an attempt for a more user-friendly stack trace ("Human readable stack trace in case of cycle").

nojb · 2023-12-29T13:45:26Z

The feature is now versioned for Dune >= 3.13 and I pushed some (brief) documentation. This is now ready for a first round of review. Thanks!

rgrinberg · 2024-01-08T07:13:34Z

src/dune_rules/expander.ml

@@ -909,6 +909,24 @@ let expand_and_eval_set t set ~standard =
  Ordered_set_lang.eval set ~standard ~eq:String.equal ~parse:(fun ~loc:_ s -> s)
 ;;

+module Unordered (Key : Ordered_set_lang.Key) = struct


Can this code be moved to where it's being used? E.g. modules_field_evaluator? I don't see how it depends on any private api's of expander.

Done, thanks. 1debb81

rgrinberg

LGTM. It would be nice to add (mode byte) to the tests to make them a little faster. I don't see a need for native compilation to test the specific features here.

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

nojb · 2024-01-10T10:01:18Z

It would be nice to add (mode byte) to the tests to make them a little faster.

Fided, thanks.

Planning to merge once CI passes. Thanks for the review!

nojb · 2024-01-10T10:31:31Z

Merged, thanks!

@rgrinberg

CHANGES: - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Directory targets can now be caches. (ocaml/dune#9535, @rleshchinskiy) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon)

@nojb

CHANGES: ### Added - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon) ### Changed - Check that package names in `(depends)` and related fields in `dune-project` are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS) ### Fixed - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg)

@nojb

CHANGES: ### Added - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon) ### Changed - Check that package names in `(depends)` and related fields in `dune-project` are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS) ### Fixed - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg)

nojb force-pushed the modules_unexpanded branch from b9468e9 to eb359a3 Compare December 28, 2023 21:28

rgrinberg reviewed Dec 29, 2023

View reviewed changes

nojb force-pushed the modules_unexpanded branch 4 times, most recently from c066172 to db3d657 Compare December 29, 2023 13:44

rgrinberg added this to the 3.13.0 milestone Jan 4, 2024

rgrinberg reviewed Jan 8, 2024

View reviewed changes

nojb force-pushed the modules_unexpanded branch 2 times, most recently from 1417433 to 1debb81 Compare January 9, 2024 09:58

rgrinberg approved these changes Jan 10, 2024

View reviewed changes

nojb added 12 commits January 10, 2024 10:58

Rename Ml_sources.Artifacts => Artifacts.Objs

ad5f602

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

Expand (modules) fields

1fec01d

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

Add tests

9672a0d

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

Handle fake modules

ba7dfd4

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

Human-readable stack trace in case of cycle

6f718de

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

Feature only available in Dune >= 3.13

cc49b80

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

Doc

02607ac

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

Changes

dfdaee8

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

More tests

6ed8da9

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

Better stack traces

2db5dfd

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

Move Expander.Unordered to Modules_field_evaluator

ffdcba4

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

Build tests on bytecode only

f7ca1e8

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

nojb force-pushed the modules_unexpanded branch from 1debb81 to f7ca1e8 Compare January 10, 2024 10:00

nojb merged commit 42fe6cb into ocaml:main Jan 10, 2024
26 of 27 checks passed

emillon mentioned this pull request Jan 12, 2024

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

Closed

emillon mentioned this pull request Jan 16, 2024

[new release] dune (15 packages) (3.13.0) ocaml/opam-repository#25081

Merged

This was referenced Mar 17, 2024

test: (modules_without_implementation) expansion #10280

Merged

feat: upgrade core to OCaml 5.2 melange-re/melange#1074

Merged

emillon mentioned this pull request Apr 3, 2024

test: demonstrate a dependency cycle between modules field and rule #10370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow expansion in `(modules)` field #9578

Allow expansion in `(modules)` field #9578

nojb commented Dec 28, 2023

rgrinberg commented Dec 28, 2023

nojb commented Dec 29, 2023

rgrinberg Dec 29, 2023

nojb Dec 29, 2023

nojb Dec 29, 2023

nojb commented Dec 29, 2023

rgrinberg Jan 8, 2024

nojb Jan 9, 2024 •

edited

rgrinberg left a comment

nojb commented Jan 10, 2024

nojb commented Jan 10, 2024

Allow expansion in (modules) field #9578

Allow expansion in (modules) field #9578

Conversation

nojb commented Dec 28, 2023

rgrinberg commented Dec 28, 2023

nojb commented Dec 29, 2023

rgrinberg Dec 29, 2023

Choose a reason for hiding this comment

nojb Dec 29, 2023

Choose a reason for hiding this comment

nojb Dec 29, 2023

Choose a reason for hiding this comment

nojb commented Dec 29, 2023

rgrinberg Jan 8, 2024

Choose a reason for hiding this comment

nojb Jan 9, 2024 • edited

Choose a reason for hiding this comment

rgrinberg left a comment

Choose a reason for hiding this comment

nojb commented Jan 10, 2024

nojb commented Jan 10, 2024

Allow expansion in `(modules)` field #9578

Allow expansion in `(modules)` field #9578

nojb Jan 9, 2024 •

edited