Ensure that `dune describe` computes a transitively closed set of libraries #5395

esope · 2022-01-31T10:05:38Z

The intention is that, for every library UID that is listed as a
requirement, there must exist a library item with that very UID in the
output of dune describe.

Example:

With an empty foo.ml file, and the following dune file:

(library
 (name foo)
 (preprocess (pps ppx_deriving.ord)))

the dune describe command currently produces the following output:

((library
  ((name foo)
   (uid 5dd4bd87ad37b4f5713085aff4bee9c9)
   (local true)
   (requires (3c8689902dffccb8da94f1a9821d4753))
   (source_dir _build/default)
   (modules
    (((name Foo)
      (impl (_build/default/foo.ml))
      (intf ())
      (cmt (_build/default/.foo.objs/byte/foo.cmt))
      (cmti ()))))
   (include_dirs (_build/default/.foo.objs/byte))))
 (library
  ((name ppx_deriving.runtime)
   (uid 3c8689902dffccb8da94f1a9821d4753)
   (local false)
   (requires (62cfd7aaad132264861d2b6e82476877))
   (source_dir /home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime)
   (modules ())
   (include_dirs (/home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime)))))

That is to say, the external library ppx_deriving.runtime has a
dependency to another library with UID
62cfd7aaad132264861d2b6e82476877, and that is not listed in the
list of libraries.

With the proposed PR, dune describe now outputs:

((library
  ((name foo)
   (uid 5dd4bd87ad37b4f5713085aff4bee9c9)
   (local true)
   (requires (3c8689902dffccb8da94f1a9821d4753))
   (source_dir _build/default)
   (modules
    (((name Foo)
      (impl (_build/default/foo.ml))
      (intf ())
      (cmt (_build/default/.foo.objs/byte/foo.cmt))
      (cmti ()))))
   (include_dirs (_build/default/.foo.objs/byte))))
 (library
  ((name ppx_deriving.runtime)
   (uid 3c8689902dffccb8da94f1a9821d4753)
   (local false)
   (requires (62cfd7aaad132264861d2b6e82476877))
   (source_dir /home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime)
   (modules ())
   (include_dirs (/home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime))))
 (library
  ((name result)
   (uid 62cfd7aaad132264861d2b6e82476877)
   (local false)
   (requires ())
   (source_dir /home/bmontagu/.opam/4.13.1/lib/result)
   (modules ())
   (include_dirs (/home/bmontagu/.opam/4.13.1/lib/result)))))

where the missing library result is now part of the output.

This PR is a complement to PR#5376, that adds to the output the libraries used by executables (and that are missing so far).

Signed-off-by: Benoît Montagu benoit.montagu@inria.fr

…ibraries. The intention is that, for every library UID that is listed as a requirement, there must exist a library item with that very UID in the output of ``dune describe``. Example: With an empty ``foo.ml`` file, and the following ``dune`` file: (library (name foo) (preprocess (pps ppx_deriving.ord))) the ``dune describe command`` currently produces the following output: ((library ((name foo) (uid 5dd4bd87ad37b4f5713085aff4bee9c9) (local true) (requires (3c8689902dffccb8da94f1a9821d4753)) (source_dir _build/default) (modules (((name Foo) (impl (_build/default/foo.ml)) (intf ()) (cmt (_build/default/.foo.objs/byte/foo.cmt)) (cmti ())))) (include_dirs (_build/default/.foo.objs/byte)))) (library ((name ppx_deriving.runtime) (uid 3c8689902dffccb8da94f1a9821d4753) (local false) (requires (62cfd7aaad132264861d2b6e82476877)) (source_dir /home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime) (modules ()) (include_dirs (/home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime))))) That is to say, the external library ``ppx_deriving.runtime`` has a dependency to another library with UID ``62cfd7aaad132264861d2b6e82476877``, and that is not listed in the list of libraries. With the proposed PR, ``dune describe`` now outputs: ((library ((name foo) (uid 5dd4bd87ad37b4f5713085aff4bee9c9) (local true) (requires (3c8689902dffccb8da94f1a9821d4753)) (source_dir _build/default) (modules (((name Foo) (impl (_build/default/foo.ml)) (intf ()) (cmt (_build/default/.foo.objs/byte/foo.cmt)) (cmti ())))) (include_dirs (_build/default/.foo.objs/byte)))) (library ((name ppx_deriving.runtime) (uid 3c8689902dffccb8da94f1a9821d4753) (local false) (requires (62cfd7aaad132264861d2b6e82476877)) (source_dir /home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime) (modules ()) (include_dirs (/home/bmontagu/.opam/4.13.1/lib/ppx_deriving/runtime)))) (library ((name result) (uid 62cfd7aaad132264861d2b6e82476877) (local false) (requires ()) (source_dir /home/bmontagu/.opam/4.13.1/lib/result) (modules ()) (include_dirs (/home/bmontagu/.opam/4.13.1/lib/result))))) where the missing library ``result`` is now part of the output. Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>

bin/describe.ml

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

esope · 2022-02-05T17:43:52Z

@bobot are you happy with the fix I pushed?

bobot · 2022-02-07T14:51:34Z

I'm happy but @rgrinberg would be more expert to judge it, and I don't remember if it should be kept behind an option or who should be warned of changes in the format.

esope · 2022-02-07T14:54:51Z

I'm happy but @rgrinberg would be more expert to judge it, and I don't remember if it should be kept behind an option or who should be warned of changes in the format.

Importantly, the format is the same as before.
The only effect is that new library items might now belong to the output (and there are always external libraries).

rgrinberg · 2022-02-08T05:17:23Z

The fix itself looks quite good to me. Could you add a test though? It's not obvious that this should be part of the specification of dune describe so a test should witness that.

esope · 2022-02-10T15:06:53Z

This test would trigger non-reproducible outputs, as described in this related PR. I'm first waiting for a solution to that issue, since that issue contains the test you're asking for.

The tests now include an example where transitive dependencies make a difference. Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>

esope · 2022-02-15T09:42:56Z

The other PR I've mentioned was recently merged. I updated the tests.
The tests now contain an example, where the transitive closure of dependencies matters (a PPX rewriter depends on some libs from JaneStreet, that were not part of the non-transitively-closed output of dune describe).

… that must be transitively closed. Added a test to witness the change. Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>

esope · 2022-02-15T16:07:18Z

I integrated the changes from PR#5376.
@rgrinberg: I consider the current PR complete.

rgrinberg · 2022-02-16T02:37:19Z

Looks good. Could you update CHANGES?

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

esope · 2022-02-16T09:04:40Z

Sure. Done.

rgrinberg · 2022-02-16T22:58:48Z

Cherry picked onto master. Thanks.

@rgrinberg

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

bobot reviewed Jan 31, 2022

View reviewed changes

bin/describe.ml Show resolved Hide resolved

esope and others added 2 commits January 31, 2022 12:08

Use the builtin Lib.closure function to compute the transitive closure

235d43e

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

Merge branch 'main' into describe_trans

19145af

esope mentioned this pull request Feb 1, 2022

Added dependencies of executables to the output of dune describe #5376

Closed

esope added 2 commits February 1, 2022 12:47

Merge branch 'main' into describe_trans

c30802a

Merge branch 'main' into describe_trans

5ee8bf5

Merge branch 'main' into describe_trans

e786dc3

bobot requested a review from rgrinberg February 7, 2022 14:51

rgrinberg self-assigned this Feb 7, 2022

esope and others added 2 commits February 15, 2022 10:08

Merge branch 'main' into describe_trans

c482d4f

Fixed type error after merge, and updated tests.

a8f7553

The tests now include an example where transitive dependencies make a difference. Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>

Added the library dependencies of executables to the set of libraries…

b057dc9

… that must be transitively closed. Added a test to witness the change. Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>

rgrinberg added this to the 3.1.0 milestone Feb 16, 2022

Added an entry to CHANGES

57ae197

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

esope added 4 commits February 16, 2022 10:20

Merge branch 'main' into describe_trans

8054043

Merge branch 'main' into describe_trans

ec5bffc

Merge branch 'main' into describe_trans

4036e68

Merge branch 'main' into describe_trans

fdcc730

rgrinberg closed this Feb 16, 2022

esope deleted the describe_trans branch February 17, 2022 08:16

rgrinberg mentioned this pull request Apr 15, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info and dune-action-plugin (3.1.0) ocaml/opam-repository#21217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that `dune describe` computes a transitively closed set of libraries #5395

Ensure that `dune describe` computes a transitively closed set of libraries #5395

esope commented Jan 31, 2022 •

edited

Loading

esope commented Feb 5, 2022

bobot commented Feb 7, 2022

esope commented Feb 7, 2022

rgrinberg commented Feb 8, 2022

esope commented Feb 10, 2022

esope commented Feb 15, 2022

esope commented Feb 15, 2022 •

edited

Loading

rgrinberg commented Feb 16, 2022

esope commented Feb 16, 2022

rgrinberg commented Feb 16, 2022

Ensure that dune describe computes a transitively closed set of libraries #5395

Ensure that dune describe computes a transitively closed set of libraries #5395

Conversation

esope commented Jan 31, 2022 • edited Loading

esope commented Feb 5, 2022

bobot commented Feb 7, 2022

esope commented Feb 7, 2022

rgrinberg commented Feb 8, 2022

esope commented Feb 10, 2022

esope commented Feb 15, 2022

esope commented Feb 15, 2022 • edited Loading

rgrinberg commented Feb 16, 2022

esope commented Feb 16, 2022

rgrinberg commented Feb 16, 2022

Ensure that `dune describe` computes a transitively closed set of libraries #5395

Ensure that `dune describe` computes a transitively closed set of libraries #5395

esope commented Jan 31, 2022 •

edited

Loading

esope commented Feb 15, 2022 •

edited

Loading