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

fix: lib collision public same folder #10

Merged
merged 4 commits into from
Mar 28, 2024
Merged

fix: lib collision public same folder #10

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f651140
fix: lib collision public same folder
jchavarri Mar 26, 2024
80412ce
Merge branch 'anmonteiro/multi-context-libs' into jchavarri/multi-con…
jchavarri Mar 27, 2024
c1a8628
ml_sources: use obj_dir instead of priv_name
jchavarri Mar 27, 2024
8841c13
refactor: use the library name!
anmonteiro Mar 28, 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
5 changes: 4 additions & 1 deletion src/dune_rules/ml_sources.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ module Modules = struct
libs
~init:(Lib_name.Set.empty, Lib_info.Sentinel.Map.empty)
~f:(fun (lib_set, acc) part ->
let name = Library.best_name part.stanza in
(* we need to check for private name to avoid "multiple rules" errors,
because even for public libraries, the artifacts folder still uses
the private name *)
let name = Library.private_name part.stanza in
Copy link
Owner

@anmonteiro anmonteiro Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we should be checking directly what conflicts. In this case, check the obj_dir path equality rather than the private name equality? we have it in group.obj_dir.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed private_name and used obj_dir instead in c1a8628.

imo, as a user the error message is slightly harder to understand because we can't refer to the lib private name anymore, but I don't have a strong opinion. Whatever you and @rgrinberg prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

I do actually prefer the current version than the previous one. This looks way better IMO

match Lib_name.Set.mem lib_set name with
| true ->
User_error.raise
Expand Down
1 change: 1 addition & 0 deletions src/dune_rules/stanzas/library.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ let best_name t =
| Public p -> snd p.name
;;

let private_name t = Lib_name.of_local t.name
let is_virtual t = Option.is_some t.virtual_modules
let is_impl t = Option.is_some t.implements

Expand Down
1 change: 1 addition & 0 deletions src/dune_rules/stanzas/library.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ val foreign_lib_files
val archive : t -> dir:Path.Build.t -> ext:string -> Path.Build.t

val best_name : t -> Lib_name.t
val private_name : t -> Lib_name.t
val is_virtual : t -> bool
val is_impl : t -> bool
val obj_dir : dir:Path.Build.t -> t -> Path.Build.t Obj_dir.t
Expand Down
22 changes: 10 additions & 12 deletions test/blackbox-tests/test-cases/lib-collision/lib-collision-public-same-folder.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ the same folder.
Without any consumers of the libraries

$ dune build
Error: Multiple rules generated for _build/default/foo.cmxs:
- dune:4
- dune:1
File "dune", line 4, characters 0-44:
4 | (library
5 | (name foo)
6 | (public_name baz.foo))
Error: Library "foo" appears for the second time in this directory
[1]

With some consumer
Expand All @@ -43,13 +45,9 @@ With some consumer
> EOF

$ dune build
File "dune", line 1, characters 0-0:
Error: Module "Main" is used in several stanzas:
- dune:1
- dune:4
- dune:7
To fix this error, you must specify an explicit "modules" field in every
library, executable, and executables stanzas in this dune file. Note that
each module cannot appear in more than one "modules" field - it must belong
to a single library or executable.
File "dune", line 4, characters 0-44:
4 | (library
5 | (name foo)
6 | (public_name baz.foo))
Error: Library "foo" appears for the second time in this directory
[1]
Loading