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.

Sign up for GitHub

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

Jump to bottom

Make dune describe correctly handle overlapping implementations for virtual libraries #5971

Merged
merged 2 commits into from
Jul 20, 2022
Merged

Make dune describe correctly handle overlapping implementations for virtual libraries #5971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
423bbbd
test: `dune describe` and virtual libraries
esope Jul 14, 2022
f9d7471
fix: dune describe & virtual libraries
esope Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
3.4.0 (Unreleased)
------------------

- Make `dune describe` correctly handle overlapping implementations
for virtual libraries (#5971, fixes #5747, @esope)

- Building the `@check` alias should make sure the libraries and executables
don't have dependency cycles (#5892, @rgrinberg)

Expand Down
12 changes: 1 addition & 11 deletions bin/describe.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,16 +404,6 @@ module Crawl = struct
in
Some (Descr.Item.Library lib_descr)

(** [add_transitive_deps libs] returns the union of [libs] and of its
transitive dependencies *)
let add_transitive_deps (libs : Lib.t list) =
(* get the transitive closure using [Lib.closure] *)
Lib.closure libs ~linking:false
>>| Resolve.to_result >>| Result.value ~default:[]
>>| (* then, remove duplicates, and ensure the list is sorted, for
reproducibility concerns *)
Lib.Set.of_list >>| Lib.Set.to_list

(** Builds a workspace description for the provided dune setup and context *)
let workspace options
({ Dune_rules.Main.conf; contexts = _; scontexts } :
Expand Down Expand Up @@ -452,7 +442,7 @@ module Crawl = struct
let+ libs =
(* the executables' libraries, and the project's libraries *)
Lib.Set.union exe_libs project_libs
|> Lib.Set.to_list |> add_transitive_deps
|> Lib.Set.to_list |> Lib.descriptive_closure
>>= Memo.parallel_map ~f:(library ~options sctx)
>>| List.filter_opt
in
Expand Down
34 changes: 34 additions & 0 deletions src/dune_rules/lib.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1600,6 +1600,40 @@ let closure l ~linking =
Resolve_names.compile_closure_with_overlap_checks None l
~forbidden_libraries

let descriptive_closure (l : lib list) : lib list Memo.t =
(* [add_work todo l] adds the libraries in [l] to the list [todo],
that contains the libraries to handle next *)
let open Memo.O in
let add_work todo l = if List.is_empty l then todo else l :: todo in
(* [register_work todo l] reads the list of libraries [l] and adds
them to the todo list [todo] *)
let register_work todo l =
let+ l = Resolve.read_memo l in
add_work todo l
in
(* [work todo acc] adds the transitive-reflexive closure of the
libraries that are contained in the todo list [todo] and are not
in the set of libraries [acc] to the initial set of libraries
[acc] *)
let rec work (todo : lib list list) (acc : Set.t) =
match todo with
| [] -> Memo.return acc
| [] :: todo -> work todo acc
| (lib :: libs) :: todo ->
if Set.mem acc lib then work (add_work todo libs) acc
else
let todo = add_work todo libs
and acc = Set.add acc lib in
let* todo = register_work todo lib.pps in
let* todo = register_work todo lib.ppx_runtime_deps in
let* todo = register_work todo lib.requires in
work todo acc
in
(* we compute the transitive closure *)
let+ trans_closure = work [ l ] Set.empty in
(* and then convert it to a list *)
Set.to_list trans_closure

module Compile = struct
module Resolved_select = Resolved_select

Expand Down
8 changes: 8 additions & 0 deletions src/dune_rules/lib.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,14 @@ end

val closure : t list -> linking:bool -> t list Resolve.Memo.t

(** [descriptive_closure libs] computes the smallest set of libraries that
contains the libraries in the list [libs], and that is transitively closed.
The output list is guaranteed to have no duplicates and to be sorted. The
difference with [closure libs] is that the latter may raise an error when
overlapping implementations of virtual libraries are detected.
[descriptive_closure libs] makes no such check. *)
val descriptive_closure : t list -> t list Memo.t

(** {1 Sub-systems} *)

module Sub_system : sig
Expand Down
145 changes: 143 additions & 2 deletions test/blackbox-tests/test-cases/describe.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,28 @@ Setup

$ touch re_lib1.re re_lib2.re re_lib2.rei

$ mkdir virtual
$ cat >virtual/dune <<EOF
> (library
> (name virtual)
> (virtual_modules virtual))
> EOF
$ touch virtual/virtual.mli
$ mkdir virtual_impl1
$ cat >virtual_impl1/dune <<EOF
> (library
> (name virtual_impl1)
> (implements virtual))
> EOF
$ touch virtual_impl1/virtual.ml
$ mkdir virtual_impl2
$ cat >virtual_impl2/dune <<EOF
> (library
> (name virtual_impl2)
> (implements virtual))
> EOF
$ touch virtual_impl2/virtual.ml

Describe various things
-----------------------

Expand Down Expand Up @@ -598,7 +620,60 @@ not stable across different setups.
(requires ())
(source_dir /FINDLIB//stdlib-shims)
(modules ())
(include_dirs (/FINDLIB//stdlib-shims)))))
(include_dirs (/FINDLIB//stdlib-shims))))
(library
((name virtual)
(uid f0299ba46dc29b8d4bd2f5d1cf82587c)
(local true)
(requires ())
(source_dir _build/default/virtual)
(modules
(((name Virtual)
(impl ())
(intf (_build/default/virtual/virtual.mli))
(cmt ())
(cmti (_build/default/virtual/.virtual.objs/byte/virtual.cmti)))))
(include_dirs (_build/default/virtual/.virtual.objs/byte))))
(library
((name virtual_impl1)
(uid 243949502d62f27969aff867fdfb0c6a)
(local true)
(requires (f0299ba46dc29b8d4bd2f5d1cf82587c))
(source_dir _build/default/virtual_impl1)
(modules
(((name Virtual)
(impl (_build/default/virtual_impl1/virtual.ml))
(intf ())
(cmt
(_build/default/virtual_impl1/.virtual_impl1.objs/byte/virtual.cmt))
(cmti ()))
((name Virtual__virtual_impl1__)
(impl (_build/default/virtual_impl1/virtual__virtual_impl1__.ml-gen))
(intf ())
(cmt
(_build/default/virtual_impl1/.virtual_impl1.objs/byte/virtual__virtual_impl1__.cmt))
(cmti ()))))
(include_dirs (_build/default/virtual_impl1/.virtual_impl1.objs/byte))))
(library
((name virtual_impl2)
(uid bfe8d16a00ac2473ce3fc5fc99d7c6cb)
(local true)
(requires (f0299ba46dc29b8d4bd2f5d1cf82587c))
(source_dir _build/default/virtual_impl2)
(modules
(((name Virtual)
(impl (_build/default/virtual_impl2/virtual.ml))
(intf ())
(cmt
(_build/default/virtual_impl2/.virtual_impl2.objs/byte/virtual.cmt))
(cmti ()))
((name Virtual__virtual_impl2__)
(impl (_build/default/virtual_impl2/virtual__virtual_impl2__.ml-gen))
(intf ())
(cmt
(_build/default/virtual_impl2/.virtual_impl2.objs/byte/virtual__virtual_impl2__.cmt))
(cmti ()))))
(include_dirs (_build/default/virtual_impl2/.virtual_impl2.objs/byte)))))

$ dune describe workspace --lang 0.1 --with-deps --sanitize-for-tests
((executables
Expand Down Expand Up @@ -1104,7 +1179,73 @@ not stable across different setups.
(requires ())
(source_dir /FINDLIB//stdlib-shims)
(modules ())
(include_dirs (/FINDLIB//stdlib-shims)))))
(include_dirs (/FINDLIB//stdlib-shims))))
(library
((name virtual)
(uid f0299ba46dc29b8d4bd2f5d1cf82587c)
(local true)
(requires ())
(source_dir _build/default/virtual)
(modules
(((name Virtual)
(impl ())
(intf (_build/default/virtual/virtual.mli))
(cmt ())
(cmti (_build/default/virtual/.virtual.objs/byte/virtual.cmti))
(module_deps ((for_intf ()) (for_impl ()))))))
(include_dirs (_build/default/virtual/.virtual.objs/byte))))
(library
((name virtual_impl1)
(uid 243949502d62f27969aff867fdfb0c6a)
(local true)
(requires (f0299ba46dc29b8d4bd2f5d1cf82587c))
(source_dir _build/default/virtual_impl1)
(modules
(((name Virtual)
(impl (_build/default/virtual_impl1/virtual.ml))
(intf ())
(cmt
(_build/default/virtual_impl1/.virtual_impl1.objs/byte/virtual.cmt))
(cmti ())
(module_deps
((for_intf ())
(for_impl ()))))
((name Virtual__virtual_impl1__)
(impl (_build/default/virtual_impl1/virtual__virtual_impl1__.ml-gen))
(intf ())
(cmt
(_build/default/virtual_impl1/.virtual_impl1.objs/byte/virtual__virtual_impl1__.cmt))
(cmti ())
(module_deps
((for_intf ())
(for_impl ()))))))
(include_dirs (_build/default/virtual_impl1/.virtual_impl1.objs/byte))))
(library
((name virtual_impl2)
(uid bfe8d16a00ac2473ce3fc5fc99d7c6cb)
(local true)
(requires (f0299ba46dc29b8d4bd2f5d1cf82587c))
(source_dir _build/default/virtual_impl2)
(modules
(((name Virtual)
(impl (_build/default/virtual_impl2/virtual.ml))
(intf ())
(cmt
(_build/default/virtual_impl2/.virtual_impl2.objs/byte/virtual.cmt))
(cmti ())
(module_deps
((for_intf ())
(for_impl ()))))
((name Virtual__virtual_impl2__)
(impl (_build/default/virtual_impl2/virtual__virtual_impl2__.ml-gen))
(intf ())
(cmt
(_build/default/virtual_impl2/.virtual_impl2.objs/byte/virtual__virtual_impl2__.cmt))
(cmti ())
(module_deps
((for_intf ())
(for_impl ()))))))
(include_dirs (_build/default/virtual_impl2/.virtual_impl2.objs/byte)))))


Test other formats
Expand Down