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

fix: do not re-run jbuild syntax dune files on every run #7894

Conversation

rgrinberg
Copy link
Member

While this is more correct, it ends up being too slow in practice
especially since we don't have cut-off on the results

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

@rgrinberg rgrinberg requested a review from emillon June 5, 2023 20:28
@rgrinberg rgrinberg added this to the 3.9.0 milestone Jun 5, 2023
@rgrinberg rgrinberg force-pushed the ps/rr/fix__do_not_re_run_jbuild_syntax_dune_files_on_every_run branch from d7caeb9 to 2ec9f20 Compare June 5, 2023 22:02
@emillon
Copy link
Collaborator

emillon commented Jun 6, 2023

As I understand it, the rule that translates dune-in-ocaml-syntax to list-of-stanzas has an implicit (deps (universe)) on it (is it actually a rule in the build engine sense?), so if we want to respect it semantics we pretty much have to re-run.

Aside: I think it could make sense to switch to an explicit dependency specification for these files. For example it's common to use ocaml syntax to read an environment variable. One problem with this is that there's no good place to store that specification: it would be quirky to store that in the dune file itself; storing it in dune-project might work but we would need to store the list of dune files in ocaml syntax in the project (actually it might be a good idea in itself to declare that a project is using this escape hatch).
If we have this dependency mechanism in place, it's more correct to assume that by default, dune files in ocaml syntax have no dependencies (and don't need to get run at every iteration), but if necessary they can depend on (universe) or more precise deps.

If I understand correctly, the solution you're proposing is not to run the rule at every iteration in watch mode, but still run it every time dune build runs. This is probably an acceptable tradeoff, but it means that stopping the dune build -w process and starting it again can change the results. I'm not sure this is the case today.

@rgrinberg
Copy link
Member Author

so if we want to respect it semantics we pretty much have to re-run.

Actually, we do not run universe no every iteration of the watch mode - for better or for worse.

Aside: I think it could make sense to switch to an explicit dependency specification for these files. For example it's common to use ocaml syntax to read an environment variable. One problem with this is that there's no good place to store that specification: it would be quirky to store that in the dune file itself; storing it in dune-project might work but we would need to store the list of dune files in ocaml syntax in the project (actually it might be a good idea in itself to declare that a project is using this escape hatch).
If we have this dependency mechanism in place, it's more correct to assume that by default, dune files in ocaml syntax have no dependencies (and don't need to get run at every iteration), but if necessary they can depend on (universe) or more precise deps.

Right, and there's no issue with doing that. All we need is to call the explicit build_* functions in Dune_engine and the dependencies will be tracked automatically for us.

If I understand correctly, the solution you're proposing is not to run the rule at every iteration in watch mode, but still run it every time dune build runs. This is probably an acceptable tradeoff, but it means that stopping the dune build -w process and starting it again can change the results. I'm not sure this is the case today.

Yup, this is exactly what I'm proposing. Note that this is exactly the same as universe dependencies work.

CHANGES.md Outdated
@@ -1,6 +1,9 @@
Unreleased
----------

- Do not re-run jbuild syntax files on every iteration of the watch mode. This
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Do not re-run jbuild syntax files on every iteration of the watch mode. This
- Do not re-run OCaml syntax files on every iteration of the watch mode. This

Copy link
Collaborator

Choose a reason for hiding this comment

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

Internally it's known as a jbuild plugin but I think we should use the term OCaml syntax in docs.

@emillon
Copy link
Collaborator

emillon commented Jun 6, 2023

Thanks for the explanation. Sounds good to me then.

While this is more correct, it ends up being too slow in practice
especially since we don't have cut-off on the results

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

<!-- ps-id: 9d1157e2-de9a-4b99-9bd5-3a76490d42d7 -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix__do_not_re_run_jbuild_syntax_dune_files_on_every_run branch from 2ec9f20 to 6c737f6 Compare June 6, 2023 11:27
@rgrinberg rgrinberg merged commit e1f3e19 into main Jun 6, 2023
@rgrinberg rgrinberg deleted the ps/rr/fix__do_not_re_run_jbuild_syntax_dune_files_on_every_run branch June 6, 2023 12:13
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 23, 2023
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 28, 2023
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Add `--all` option to `dune rpc status` to show all Dune RPC servers running.
  (ocaml/dune#8011, fix ocaml/dune#7902, @Alizter)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
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.

dune build -w memory usage grows without bounds when using OCaml syntax in dune files
2 participants