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

Add test to show lib name collision in contexts #9839

Closed
wants to merge 38 commits into from
Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
51cd326
add test to show lib name collision in workspaces
jchavarri Jan 25, 2024
3143887
lib: pass ctxt to resolve
jchavarri Jan 29, 2024
ea48974
Revert "lib: pass ctxt to resolve"
jchavarri Jan 29, 2024
1b40b42
lib: allow multiple libs in Found
jchavarri Jan 30, 2024
a4d298c
lib collision: update test
jchavarri Feb 2, 2024
822e046
_
rgrinberg Feb 3, 2024
00e26fa
_
rgrinberg Feb 3, 2024
25add02
rules: don't filter libs when only 1 is found
jchavarri Feb 5, 2024
0d6d8e2
update lib collision test to show public libs behavior
jchavarri Feb 5, 2024
df6a7dc
move test to enabled_if folder
jchavarri Feb 5, 2024
43dda0a
add available_library_stanza
jchavarri Feb 5, 2024
f292c15
cleanup
jchavarri Feb 5, 2024
d970da9
add extra test
jchavarri Feb 5, 2024
e53cbdf
lib: attempt to process duplicated lib error
jchavarri Feb 6, 2024
dc82220
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 12, 2024
ee1da7d
lib collision: improve test
jchavarri Feb 12, 2024
28585d6
add dir to get_compiled_info
jchavarri Feb 12, 2024
4f1b4a7
eif: add test for collision when 2 libs are in same folder
jchavarri Feb 13, 2024
40eff23
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 13, 2024
fcf7960
lib: fixes after merging main
jchavarri Feb 13, 2024
0ea5c27
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 13, 2024
7f19d29
eif: don't add any rules if lib is disabled
jchavarri Feb 13, 2024
ff91664
don't add rules if lib stanza is disabled
jchavarri Feb 14, 2024
422b666
lib: remove unused function
jchavarri Feb 14, 2024
cd54d89
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 14, 2024
b38558a
fix tests descriptions
jchavarri Feb 15, 2024
963a201
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 15, 2024
e9ede35
update tests
jchavarri Feb 15, 2024
fa65065
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 26, 2024
c4d7be5
update tests
jchavarri Feb 26, 2024
0252670
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 27, 2024
e0f9775
apply suggestions from code review
jchavarri Feb 28, 2024
a8910d4
Merge branch 'main' into workspaces-lib-name-collision
jchavarri Feb 29, 2024
fb67ba9
test: add tests for library name collisions
jchavarri Feb 29, 2024
11aa1e3
Merge branch 'add-tests-lib-collisions' into workspaces-lib-name-coll…
jchavarri Feb 29, 2024
ff8289d
update tests
jchavarri Feb 29, 2024
70e6ea7
apply suggestions from code review
jchavarri Feb 29, 2024
619ea9d
attempt to deal with public libs
jchavarri Feb 29, 2024
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
23 changes: 13 additions & 10 deletions bin/ocaml/top.ml
Original file line number Diff line number Diff line change
Expand Up @@ -165,16 +165,19 @@ module Module = struct
in
let pps () =
let module Merlin = Dune_rules.Merlin in
let pps = Merlin.pp_config merlin ctx ~expander in
let+ pps, _ = Action_builder.evaluate_and_collect_facts pps in
let pp = Dune_rules.Module_name.Per_item.get pps module_name in
match pp with
| None -> None, None
| Some pp_flags ->
let args = Merlin.Processed.pp_args pp_flags in
(match Merlin.Processed.pp_kind pp_flags with
| Pp -> Some args, None
| Ppx -> None, Some args)
match merlin with
| None -> Memo.return (None, None)
| Some merlin ->
let pps = Merlin.pp_config merlin ctx ~expander in
let+ pps, _ = Action_builder.evaluate_and_collect_facts pps in
let pp = Dune_rules.Module_name.Per_item.get pps module_name in
(match pp with
| None -> None, None
| Some pp_flags ->
let args = Merlin.Processed.pp_args pp_flags in
(match Merlin.Processed.pp_kind pp_flags with
| Pp -> Some args, None
| Ppx -> None, Some args))
in
let+ (pp, ppx), files_to_load = Memo.fork_and_join pps files_to_load in
let code =
Expand Down
13 changes: 5 additions & 8 deletions src/dune_rules/gen_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,11 @@ end = struct
let+ () = toplevel_setup ~sctx ~dir ~toplevel in
empty_none
| Library.T lib ->
let* enabled_if = Lib.DB.available (Scope.libs scope) (Library.best_name lib) in
if_available_buildable
~loc:lib.buildable.loc
(fun () ->
let+ () = Odoc.setup_private_library_doc_alias sctx ~scope ~dir:ctx_dir lib
anmonteiro marked this conversation as resolved.
Show resolved Hide resolved
and+ rules = Lib_rules.rules lib ~sctx ~dir ~scope ~dir_contents ~expander in
rules)
enabled_if
let* available = Lib_rules.rules lib ~sctx ~dir ~scope ~dir_contents ~expander in
(match available with
| None -> Memo.return empty_none
| Some (cctx, merlin) ->
Memo.return { empty_none with merlin; cctx = Some (lib.buildable.loc, cctx) })
| Foreign.Library.T lib ->
Expander.eval_blang expander lib.enabled_if
>>= if_available (fun () ->
Expand Down
156 changes: 104 additions & 52 deletions src/dune_rules/lib.ml
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,18 @@ module Error = struct
]
;;

let duplicated ~loc ~name ~dir_a ~dir_b =
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this only seems to be used once. I'd probably inline it in its usage site.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll need it for public libs as well (see failed attempt in 619ea9d).

User_error.make
~loc
[ Pp.textf
"A library with name %S is defined in two folders: %s and %s. Either change \
one of the names, or enable them conditionally using the 'enabled_if' field."
(Lib_name.to_string name)
(Path.to_string_maybe_quoted dir_a)
(Path.to_string_maybe_quoted dir_b)
]
;;

(* diml: it is not very clear what a "default implementation cycle" is *)
let default_implementation_cycle cycle =
make
Expand Down Expand Up @@ -406,7 +418,7 @@ type db =

and resolve_result =
| Not_found
| Found of Lib_info.external_
| Found of Lib_info.external_ list
| Hidden of Lib_info.external_ Hidden.t
| Invalid of User_message.t
| Ignore
Expand Down Expand Up @@ -1125,19 +1137,53 @@ end = struct
| Hidden h -> Hidden.error h ~loc ~name >>| Option.some
;;

let find_in_parent ~db ~name =
let open Memo.O in
let+ res =
match db.parent with
| None -> Memo.return Status.Not_found
| Some db -> find_internal db name
in
res
;;

let to_status ~db ~name = function
| [] -> find_in_parent ~db ~name
| info :: [] -> instantiate db name info ~hidden:None
| a :: b :: _ ->
let loc = Lib_info.loc b in
let dir_a = Lib_info.src_dir a in
let dir_b = Lib_info.src_dir b in
Memo.return (Status.Invalid (Error.duplicated ~loc ~name ~dir_a ~dir_b))
;;

let resolve_name db name =
let open Memo.O in
db.resolve name
>>= function
| Ignore -> Memo.return Status.Ignore
| Redirect_in_the_same_db (_, name') -> find_internal db name'
| Redirect (db', (_, name')) -> find_internal db' name'
| Found info -> instantiate db name info ~hidden:None
| Found libs ->
(match libs with
| [] | _ :: [] ->
(* In case we have 0 or 1 results found, convert to [Status.t] directly.
This allows to provide better errors later on,
e.g. `Library "foo" in _build/default is hidden (unsatisfied 'enabled_if') *)
to_status ~db ~name libs
| _ :: _ :: _ ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like this is what does the heavy lifting in this PR:

  1. we find all libraries with the name foo
  2. if we only find one, proceed as before
  3. if there are 2 or more, proceed with the ones that aren't disabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what I tried to reflect in the inline comments. Would you change / improve anything to make it clearer?

(* If there are multiple results found, we optimistically pre-filter to
remove those that are disabled *)
let* filtered_libs =
Memo.List.filter libs ~f:(fun lib ->
let* enabled = Lib_info.enabled lib in
match enabled with
| Disabled_because_of_enabled_if -> Memo.return false
| Normal | Optional -> Memo.return true)
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
in
to_status ~db ~name filtered_libs)
| Invalid e -> Memo.return (Status.Invalid e)
| Not_found ->
(match db.parent with
| None -> Memo.return Status.Not_found
| Some db -> find_internal db name)
| Not_found -> find_in_parent ~db ~name
| Hidden { lib = info; reason = hidden; path = _ } ->
(match db.parent with
| None -> Memo.return Status.Not_found
Expand Down Expand Up @@ -1763,13 +1809,49 @@ module Compile = struct
;;
end

module Local : sig
type t = private lib

val of_lib : lib -> t option
val of_lib_exn : lib -> t
val to_lib : t -> lib
val obj_dir : t -> Path.Build.t Obj_dir.t
val info : t -> Path.Build.t Lib_info.t
val to_dyn : t -> Dyn.t
val equal : t -> t -> bool
val hash : t -> int

include Comparable_intf.S with type key := t
end = struct
type nonrec t = t

let to_lib t = t
let of_lib (t : lib) = Option.some_if (is_local t) t

let of_lib_exn t =
match of_lib t with
| Some l -> l
| None -> Code_error.raise "Lib.Local.of_lib_exn" [ "l", to_dyn t ]
;;

let obj_dir t = Obj_dir.as_local_exn (Lib_info.obj_dir t.info)
let info t = Lib_info.as_local_exn t.info

module Set = Set
module Map = Map

let to_dyn = to_dyn
let equal = equal
let hash = hash
end

(* Databases *)

module DB = struct
module Resolve_result = struct
type t = resolve_result =
| Not_found
| Found of Lib_info.external_
| Found of Lib_info.external_ list
| Hidden of Lib_info.external_ Hidden.t
| Invalid of User_message.t
| Ignore
Expand All @@ -1786,7 +1868,7 @@ module DB = struct
match x with
| Not_found -> variant "Not_found" []
| Invalid e -> variant "Invalid" [ Dyn.string (User_message.to_string e) ]
| Found lib -> variant "Found" [ Lib_info.to_dyn Path.to_dyn lib ]
| Found libs -> variant "Found" [ (Dyn.list (Lib_info.to_dyn Path.to_dyn)) libs ]
| Hidden h -> variant "Hidden" [ Hidden.to_dyn (Lib_info.to_dyn Path.to_dyn) h ]
| Ignore -> variant "Ignore" []
| Redirect (_, (_, name)) -> variant "Redirect" [ Lib_name.to_dyn name ]
Expand Down Expand Up @@ -1825,7 +1907,7 @@ module DB = struct
let open Memo.O in
Findlib.find findlib name
>>| function
| Ok (Library pkg) -> Found (Dune_package.Lib.info pkg)
| Ok (Library pkg) -> Found [ Dune_package.Lib.info pkg ]
| Ok (Deprecated_library_name d) ->
Redirect_in_the_same_db (d.loc, d.new_public_name)
| Ok (Hidden_library pkg) -> Hidden (Hidden.unsatisfied_exist_if pkg)
Expand Down Expand Up @@ -1894,15 +1976,21 @@ module DB = struct

let available t name = Resolve_names.available_internal t name

let get_compile_info t ~allow_overlaps name =
let get_compile_info t ~allow_overlaps ~dir name =
let open Memo.O in
find_even_when_hidden t name
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we excluding hidden libraries now?

Copy link
Collaborator Author

@jchavarri jchavarri Feb 29, 2024

Choose a reason for hiding this comment

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

If we don't skip hidden libs, then a bunch of tests will fail with this error:

  Internal error, please report upstream including the contents of _build/log.
  Description:
    ("modules_and_obj_dir: failed lookup", { keys = []; for_ = Library "foo" })

The errors seem to come from the usage in Ml_sources.modules, I have started a separate PR to discuss this change: #10171

find t name
>>| function
| Some lib -> lib, Compile.for_lib ~allow_overlaps t lib
| None ->
Code_error.raise
"Lib.DB.get_compile_info got library that doesn't exist"
[ "name", Lib_name.to_dyn name ]
| Some lib ->
(match Local.of_lib lib with
| None ->
Code_error.raise
"Lib.DB.get_compile_info got library that is not local"
[ "name", Lib_name.to_dyn name ]
| Some info ->
(match Path.Build.equal dir (Lib_info.src_dir (Local.info info)) with
| true -> Some (lib, Compile.for_lib ~allow_overlaps t lib)
| false -> None))
| None -> None
;;

let resolve_user_written_deps
Expand Down Expand Up @@ -2092,39 +2180,3 @@ let to_dune_lib
in
Dune_package.Lib.of_dune_lib ~info ~main_module_name
;;

module Local : sig
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This module was just moved up to make Local available in scope for get_compile_info.

type t = private lib

val of_lib : lib -> t option
val of_lib_exn : lib -> t
val to_lib : t -> lib
val obj_dir : t -> Path.Build.t Obj_dir.t
val info : t -> Path.Build.t Lib_info.t
val to_dyn : t -> Dyn.t
val equal : t -> t -> bool
val hash : t -> int

include Comparable_intf.S with type key := t
end = struct
type nonrec t = t

let to_lib t = t
let of_lib (t : lib) = Option.some_if (is_local t) t

let of_lib_exn t =
match of_lib t with
| Some l -> l
| None -> Code_error.raise "Lib.Local.of_lib_exn" [ "l", to_dyn t ]
;;

let obj_dir t = Obj_dir.as_local_exn (Lib_info.obj_dir t.info)
let info t = Lib_info.as_local_exn t.info

module Set = Set
module Map = Map

let to_dyn = to_dyn
let equal = equal
let hash = hash
end
12 changes: 8 additions & 4 deletions src/dune_rules/lib.mli
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ module DB : sig
type t

val not_found : t
val found : Lib_info.external_ -> t
val found : Lib_info.external_ list -> t
val to_dyn : t Dyn.builder
val redirect : db -> Loc.t * Lib_name.t -> t
val redirect_in_the_same_db : Loc.t * Lib_name.t -> t
Expand All @@ -124,15 +124,19 @@ module DB : sig

val find : t -> Lib_name.t -> lib option Memo.t
val find_even_when_hidden : t -> Lib_name.t -> lib option Memo.t

(** [available db lib_name] returns [true] if there is any library available
in the database with name [lib_name], regardless if it is enabled or not. *)
val available : t -> Lib_name.t -> bool Memo.t

(** Retrieve the compile information for the given library. Works for
libraries that are optional and not available as well. *)
(** [get_compile_info db ~allow_overlaps ~dir lib_name] retrieves the compile
information for the given library, if it exists. Otherwise returns [None]. *)
val get_compile_info
: t
-> allow_overlaps:bool
-> dir:Path.Build.t
-> Lib_name.t
-> (lib * Compile.t) Memo.t
-> (lib * Compile.t) option Memo.t

val resolve : t -> Loc.t * Lib_name.t -> lib Resolve.Memo.t

Expand Down
Loading
Loading