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

[coq] Ignore contents in coqdep rule #5547

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

ejgallego
Copy link
Collaborator

@ejgallego ejgallego commented Apr 5, 2022

This should help reduce the coqdep calls drastically.

Improves #5100 .

In particular, while a build from scratch has still one coqdep call per file overhead, incremental builds don't anymore. This makes one coqdep call per theory (or -modules) much less critical.

@ejgallego ejgallego added the coq label Apr 5, 2022
@ejgallego ejgallego added this to the 3.1.0 milestone Apr 5, 2022
@rgrinberg
Copy link
Member

Doesn't coqdep read these files? If so, then this optimization isn't correct.

@ejgallego
Copy link
Collaborator Author

Doesn't coqdep read these files? If so, then this optimization isn't correct.

No, it doesn't read them, they are only used because Coq has no -modules.

@ejgallego

This comment was marked as outdated.

@ejgallego ejgallego force-pushed the coq+ignore_contents_for_coqdep branch 4 times, most recently from f818b6b to a29c786 Compare April 5, 2022 19:12
@Alizter
Copy link
Collaborator

Alizter commented Apr 5, 2022

I am also puzzled here. What happens if I add extra Requires in a .v file, since the file system hasn't been updated, wouldn't coqdep need to be called again?

@ejgallego
Copy link
Collaborator Author

@Alizter only for that file, as shown in the test.

@ejgallego ejgallego force-pushed the coq+ignore_contents_for_coqdep branch from a29c786 to 63d2b76 Compare April 6, 2022 14:23
@ejgallego
Copy link
Collaborator Author

ejgallego commented Apr 6, 2022

@Alizter only for that file, as shown in the test.

In particular, note that that coqdep_rule has:

  let file_flags =
    [ Command.Args.S file_flags
    ; As [ "-dyndep"; "opt" ]
    ; Dep (Path.build source)
    ]
  in
  let stdout_to = Coq_module.dep_file ~obj_dir:cctx.dir coq_module in
  (* Coqdep has to be called in the stanza's directory *)
  let open Action_builder.With_targets.O in
  Action_builder.with_no_targets cctx.mlpack_rule
  >>> Action_builder.(with_no_targets (goal source_rule))
  >>> Command.run ~dir:(Path.build cctx.dir) ~stdout_to cctx.coqdep file_flags

so Dep (Path.build source) forces the rule to rerun when the .v file changes.

This should help reduce the coqdep calls drastically.

Improves ocaml#5100 .

In particular, while a build from scratch has still one coqdep call
per file overhead, incremental builds don't anymore. This makes one
coqdep call per theory (or `-modules`) much less critical.

Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
@ejgallego ejgallego force-pushed the coq+ignore_contents_for_coqdep branch from 63d2b76 to 0eeabd4 Compare April 6, 2022 14:27
@ejgallego ejgallego merged commit 10fb5ae into ocaml:main Apr 6, 2022
@ejgallego ejgallego deleted the coq+ignore_contents_for_coqdep branch April 6, 2022 17:04
@Blaisorblade
Copy link
Contributor

This is great and works indeed as intended!

@ejgallego
Copy link
Collaborator Author

It must been annoying to work with dune before this PR @Blaisorblade !

We still have plenty of low-hanging fruit to reap.

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 15, 2022
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info and dune-action-plugin (3.1.0)

CHANGES:

- Add `sourcehut` as an option for defining project sources in dune-project
  files. For example, `(source (sourcehut user/repo))`. (ocaml/dune#5564, @rgrinberg)

- Add `dune coq top` command for running a Coq toplevel (ocaml/dune#5457, @rlepigre)

- Fix dune exec dumping database in wrong directory (ocaml/dune#5544, @bobot)

- Always output absolute paths for locations in RPC reported diagnostics
  (ocaml/dune#5539, @rgrinberg)

- Add `(deps <deps>)` in ctype field (ocaml/dune#5346, @bobot)

- Add `(include <file>)` constructor to dependency specifications. This can be
  used to introduce dynamic dependencies (ocaml/dune#5442, @anmonteiro)

- Ensure that `dune describe` computes a transitively closed set of
  libraries (ocaml/dune#5395, @esope)

- Add direct dependencies to $ dune describe output (ocaml/dune#5412, @esope)

- Show auto-detected concurrency on Windows too (ocaml/dune#5502, @MisterDA)

- Fix operations that remove folders with absolute path. This happens when
  using esy (ocaml/dune#5507, @EduardoRFS)

- Dune will not fail if some directories are non-empty when uninstalling.
  (ocaml/dune#5543, fixes ocaml/dune#5542, @nojb)

- `coqdep` now depends only on the filesystem layout of the .v files,
  and not on their contents (ocaml/dune#5547, helps with ocaml/dune#5100, @ejgallego)

- The mdx stanza 0.2 can now be used with `(implicit_transitive_deps false)`
  (ocaml/dune#5558, fixes ocaml/dune#5499, @emillon)

- Fix missing parenthesis in printing of corresponding terminal command for
  `(with-outputs-to )` (ocaml/dune#5551, fixes ocaml/dune#5546, @Alizter)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants