fix(lib-collision): tolerate public_name collisions in different contexts
#10549
Conversation
…ntexts Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
anmonteiro
force-pushed
the
anmonteiro/allow-public-name-collision
branch
from
2e21bd1
to
bacf324
Compare
| @@ -23,13 +23,6 @@ different folders. | |||
| Without any consumers of the libraries | |||
|
|
|||
| $ dune build | |||
| File "b/dune", line 3, characters 14-21: | |||
There was a problem hiding this comment.
This behavior is consistent with other cases I think (should be ok)
| @@ -51,15 +46,9 @@ With some consumer | |||
| > EOF | |||
|
|
|||
| $ cat > main.ml <<EOF | |||
| > let () = Foo.x | |||
| > let () = print_endline Foo.x | |||
There was a problem hiding this comment.
Did this modification change anything in the test behavior?
There was a problem hiding this comment.
It fixed the type errors in compilation
| @@ -473,15 +460,22 @@ module DB = struct | |||
| | Some lib -> | |||
| let name = Package.name pkg in | |||
| (name, Lib_entry.Library (Lib.Local.of_lib_exn lib)) :: acc) | |||
| | Library.T { visibility = Public pub; _ } -> | |||
| let+ lib = Lib.DB.find public_libs (Public_lib.name pub) in | |||
| | Library.T { visibility = Public pub; enabled_if; _ } -> | |||
There was a problem hiding this comment.
Why don't private libraries have an enabled_if check?
There was a problem hiding this comment.
that's a good point. I'll make these changes shortly.
| | Library.T { visibility = Public pub; _ } -> | ||
| let+ lib = Lib.DB.find public_libs (Public_lib.name pub) in | ||
| | Library.T { visibility = Public pub; enabled_if; _ } -> | ||
| let* lib = Lib.DB.find public_libs (Public_lib.name pub) in |
There was a problem hiding this comment.
You should use the id rather than the name for this check.
There was a problem hiding this comment.
I tried find_by_lib_id but I got a crash about adding a duplicate name in https://github.com/ocaml/dune/blob/main/src/dune_rules/install_rules.ml#L697
There was a problem hiding this comment.
I'm not sure what that means yet.
There was a problem hiding this comment.
Ah, it's likely because it's finding the right lib now.
…ntexts (ocaml#10549) Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
scope.ml: we store andenabledToggle.t Memo.tthat we evaluate when resolving a public library by its name.