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

Added the direct dependencies between modules to the output of dune describe #5412

Closed
wants to merge 14 commits into from

Conversation

esope
Copy link
Collaborator

@esope esope commented Feb 4, 2022

When the new --with-deps flag is given to the command dune describe workspace, the output now contains a new field module_deps for each module entry. This new field contains the list of modules of the current library/executable a module immediately depends on.

For example, on a simple library project with two files a.ml and b.ml such that a.ml refers to the module B, with the following dune file:

(library
 (name a))

the command dune describe workspace --with-deps prints:

((library                         
  ((name a)
   (uid 3210b99379777ec5c43564aae2d79282)
   (local true)
   (requires ())
   (source_dir _build/default)
   (modules
    (((name B)
      (impl (_build/default/b.ml))
      (intf ())
      (cmt (_build/default/.a.objs/byte/a__B.cmt))
      (cmti ())
      (module_deps ()))
     ((name A)
      (impl (_build/default/a.ml))
      (intf ())
      (cmt (_build/default/.a.objs/byte/a.cmt))
      (cmti ())
      (module_deps (B)))
     ((name A__)
      (impl (_build/default/a__.ml-gen))
      (intf ())
      (cmt (_build/default/.a.objs/byte/a__.cmt))
      (cmti ())
      (module_deps ()))))
   (include_dirs (_build/default/.a.objs/byte)))))

@esope
Copy link
Collaborator Author

esope commented Feb 4, 2022

This PR results from this discussion thread, and hopefully provides a solution to issue#3425.
CC @nojb @rgrinberg

bin/describe.ml Outdated
union
~f:(fun _ () () -> Some ())
(of_list_unit deps_for_impl)
(of_list_unit deps_for_intf)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's perhaps better to print these sets separately and let callers merge them if needed?

Copy link
Collaborator Author

@esope esope Feb 7, 2022

Choose a reason for hiding this comment

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

Good idea. Implemented in the recently pushed changes.

@rgrinberg
Copy link
Member

Could you add tests for this feature?

@esope
Copy link
Collaborator Author

esope commented Feb 7, 2022

@rgrinberg: with your recent commit b5ad3a39b8debb430654665b60433a3acf5b4410 refactor: implement progress with svar I encounter the following error

File "src/dune_engine/build_system.ml", line 214, characters 11-17:
File "src/dune_engine/build_system.ml", line 214, characters 11-17: Assertion
failed
Raised at Dune_engine__Build_system.State.start_rule_exn in file
  "src/dune_engine/build_system.ml", line 214, characters 11-23
Called from Dune_engine__Build_system.Exported.execute_rule_impl in file
  "src/dune_engine/build_system.ml", line 557, characters 51-76
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/exn.ml", line 36, characters 27-56
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 878,
  characters 10-13

I must not crash.  Uncertainty is the mind-killer. Exceptions are the
little-death that brings total obliteration.  I will fully express my cases. 
Execution will pass over me and through me.  And when it has gone past, I
will unwind the stack along its path.  Where the cases are handled there will
be nothing.  Only I will remain.

On line 214 of file src/dune_engine/build_system.ml, the variable current seems to be in the Initializing case.

My code used to work properly without these svar-related changes.

@esope
Copy link
Collaborator Author

esope commented Feb 7, 2022

Could you add tests for this feature?

This is pushed in the recent commits.

But it triggered another (minor) issue: I expanded the current test for dune describe so that it contains an example that uses a PPX rewriter. As a consequence, new dependencies to external libraries appear, and they refer to absolute path names (example here)... This will create spurious mistakes on every developers' machines, since these paths are likely to refer to their own home directories, and can even differ when tests are run with two different versions of the OCaml compiler...

Do you see an easy solution to this issue?

esope referenced this pull request Feb 7, 2022
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ps-id: 52DED725-A17B-4910-8B63-4615B1B4C9A1
@rgrinberg
Copy link
Member

@esope thanks for the report. I'll fix this issue shortly.

@rgrinberg
Copy link
Member

Do you see an easy solution to this issue?

Honestly, there's no easy solution. The obvious thing to do is to add the paths to BUILD_PATH_PREFIX_MAP, but unfortunately the formatting is going to depend on the unfiltered length which still makes it not completely reproducible.

How about just writing a simple pre-processor that will redact the fields that aren't reproducible?

@esope
Copy link
Collaborator Author

esope commented Feb 8, 2022

How about just writing a simple pre-processor that will redact the fields that aren't reproducible?

I'll try this solution. I've also noticed that since the UIDs depend on the paths, they are subject to instability too...

@esope
Copy link
Collaborator Author

esope commented Feb 10, 2022

How about just writing a simple pre-processor that will redact the fields that aren't reproducible?

I'll try this solution. I've also noticed that since the UIDs depend on the paths, they are subject to instability too...

I'm trying to write a simple executable that reads the output of dune describe on stdin, parses it, sanitizes it to normalize the non-reproducible parts, and prints its result on stdout. This executable will be part of the test/blackbox-tests/test-cases/describe.t directory.

To do this, I need to parse the contents of stdin to produce something akin to a S-expression, for example using the functions of dune's dune_lang library. It seems I cannot access this library at all from the test/blackbox-tests/test-cases/describe.t directory. @rgrinberg: do you have any idea on how to to this?

An alternative path would be to add a flag to dune describe workspace, that would enable the sanitization before printing, but running dune's internal tests would be this flag's sole purpose...

@rgrinberg
Copy link
Member

To do this, I need to parse the contents of stdin to produce something akin to a S-expression, for example using the functions of dune's dune_lang library. It seems I cannot access this library at all from the test/blackbox-tests/test-cases/describe.t directory. @rgrinberg: do you have any idea on how to to this?

The easiest way is to introduce a subcommand to dune_cmd. Have a look at how that executable is defined.

An alternative path would be to add a flag to dune describe workspace, that would enable the sanitization before printing, but running dune's internal tests would be this flag's sole purpose...

If you think this is simpler, it is fine by me.

@esope
Copy link
Collaborator Author

esope commented Feb 11, 2022

An alternative path would be to add a flag to dune describe workspace, that would enable the sanitization before printing, but running dune's internal tests would be this flag's sole purpose...

If you think this is simpler, it is fine by me.

Done in commit 72cdde1.

@rgrinberg
Copy link
Member

CI isn't happy:

Run opam exec -- make dune.exe
[11](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:11)
ocamlc -output-complete-exe -w -24 -g -o .duneboot.exe -I boot unix.cma boot/libs.ml boot/duneboot.ml
[12](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:12)
./.duneboot.exe
[13](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:13)
cd _boot && /home/runner/.opam/4.12.1/bin/ocamldep.opt -modules -map main__.ml -map dune_rpc_impl.ml -map build_info__.ml -map dune_rules__.ml -map dune_engine__.ml -map async_inotify_for_dune__.ml -map ocaml_inotify.ml -map dune_rpc_private__.ml -map dune_meta_parser.ml -map dune_action_plugin__.ml -map dune_glob__.ml -map dune_re__.ml -map dune_cache.ml -map dune_cache_storage__.ml -map dune_util__.ml -map memo__.ml -map dag__.ml -map incremental_cycles__.ml -map dune_lang__.ml -map dune_graph__.ml -map stdune__.ml -args source_files
[14](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:14)

[15](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:15)
File "bin/describe.ml", line 838, characters 28-30:
[16](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:16)
Error: Syntax error: 'end' expected
[17](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:17)
File "bin/describe.ml", line 8[18](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:18), characters 17-23:
18
  This 'struct' might be unmatched
[19](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:19)
make: *** [Makefile:48: dune.exe] Error 2
[20](https://github.com/ocaml/dune/runs/5161075121?check_suite_focus=true#step:8:20)

@esope
Copy link
Collaborator Author

esope commented Feb 12, 2022

CI is much happier now.
I had to reimplement String.ends_with, that is only available in recent version of the standard library. @rgrinberg: I wrote the implementation in describe.ml, but do you think of a better module where this implementation should be?

esope and others added 13 commits February 14, 2022 19:20
…mmediate dependencies of a module

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
… of preprocessors that are registered for a library

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
…ules

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
…cribe``.

TODO: add a flag to the CLI, and introduce a new dune lang version

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
…dependencies in the output of ``dune describe workspace``

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
… provided

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
…nterface, and the dependencies of its implementation. We used to return the union of the two.

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
This flag does its best to produce stable outputs across different
machines, when the output involves absolute paths.

The tests for the ``dune describe workspace`` are updated so that they
use this flag.

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
…d produce a file that older OCaml versions could not parse

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
…ns of the standard library

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Member

Merged but I simplified the sanitation quite a bit. It's not as robust, but good enough for our testing needs.

@rgrinberg rgrinberg closed this Feb 15, 2022
@esope esope deleted the describe_module_deps branch February 15, 2022 08:56
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants