-
Notifications
You must be signed in to change notification settings - Fork 413
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
eif: fix name collision in same folder for exes and melange emits #10220
Changes from 15 commits
286a3c0
9216dad
e02387a
3524130
bd7b0f9
bdbdeff
50acad1
50d5bee
5189cc4
673ed9a
ca2909c
2c98f8f
ecbad4f
ef6c146
74bca3f
050aed8
623ada2
16471fe
20e02e3
1f17f12
e2b665f
a07ee5f
b22ae6f
ebfe399
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,107 +45,118 @@ module Modules = struct | |
* (Loc.t * Module.Source.t) Module_trie.t | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it time we make this a record instead? that'd be a whole lot easier to follow what each member of this group_part is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I can do so in a follow-up PR. |
||
* Modules_group.t | ||
* Path.Build.t Obj_dir.t | ||
* Toggle.t | ||
|
||
type groups = | ||
{ libraries : Library.t group_part list | ||
; executables : Executables.t group_part list | ||
; melange_emits : Melange_stanzas.Emit.t group_part list | ||
} | ||
|
||
let make { libraries = libs; executables = exes; melange_emits = emits } = | ||
let libraries = | ||
match | ||
Lib_name.Map.of_list_map libs ~f:(fun (lib, _, m, obj_dir) -> | ||
Library.best_name lib, (m, obj_dir)) | ||
with | ||
| Ok x -> x | ||
| Error (name, _, (lib2, _, _, _)) -> | ||
User_error.raise | ||
~loc:lib2.buildable.loc | ||
[ Pp.textf | ||
"Library %S appears for the second time in this directory" | ||
(Lib_name.to_string name) | ||
] | ||
let make = | ||
let keep_enabled t = | ||
List.filter t ~f:(fun (_, _, _, _, enabled) -> Toggle.enabled enabled) | ||
in | ||
let executables = | ||
match | ||
String.Map.of_list_map exes ~f:(fun ((exes : Executables.t), _, m, obj_dir) -> | ||
snd (List.hd exes.names), (m, obj_dir)) | ||
with | ||
| Ok x -> x | ||
| Error (name, _, (exes2, _, _, _)) -> | ||
User_error.raise | ||
~loc:exes2.buildable.loc | ||
[ Pp.textf "Executable %S appears for the second time in this directory" name ] | ||
in | ||
let melange_emits = | ||
match | ||
String.Map.of_list_map emits ~f:(fun (mel, _, m, obj_dir) -> | ||
mel.target, (m, obj_dir)) | ||
with | ||
| Ok x -> x | ||
| Error (name, _, (mel, _, _, _)) -> | ||
User_error.raise | ||
~loc:mel.loc | ||
[ Pp.textf "Target %S appears for the second time in this directory" name ] | ||
in | ||
let rev_map = | ||
let modules = | ||
let by_path (origin : Origin.t) trie = | ||
Module_trie.fold trie ~init:[] ~f:(fun (_loc, m) acc -> | ||
(Module.Source.path m, origin) :: acc) | ||
in | ||
List.concat | ||
[ List.concat_map libs ~f:(fun (l, m, _, _) -> by_path (Library l) m) | ||
; List.concat_map exes ~f:(fun (e, m, _, _) -> by_path (Executables e) m) | ||
; List.concat_map emits ~f:(fun (l, m, _, _) -> by_path (Melange l) m) | ||
] | ||
fun { libraries = libs; executables = exes; melange_emits = emits } -> | ||
let libs = keep_enabled libs in | ||
let exes = keep_enabled exes in | ||
let emits = keep_enabled emits in | ||
let libraries = | ||
match | ||
Lib_name.Map.of_list_map libs ~f:(fun (lib, _, m, obj_dir, _) -> | ||
Library.best_name lib, (m, obj_dir)) | ||
with | ||
| Ok x -> x | ||
| Error (name, _, (lib2, _, _, _, _)) -> | ||
User_error.raise | ||
~loc:lib2.buildable.loc | ||
[ Pp.textf | ||
"Library %S appears for the second time in this directory" | ||
(Lib_name.to_string name) | ||
] | ||
in | ||
match Module_name.Path.Map.of_list modules with | ||
| Ok x -> x | ||
| Error (path, _, _) -> | ||
let locs = | ||
List.filter_map modules ~f:(fun (n, origin) -> | ||
Option.some_if | ||
(Ordering.is_eq (Module_name.Path.compare n path)) | ||
(Origin.loc origin)) | ||
|> List.sort ~compare:Loc.compare | ||
in | ||
let main_message = | ||
Pp.textf | ||
"Module %S is used in several stanzas:" | ||
(Module_name.Path.to_string path) | ||
in | ||
let loc, related_locs = | ||
match locs with | ||
| [] -> | ||
(* duplicates imply at least at one module with this location *) | ||
assert false | ||
| loc :: related_locs -> loc, related_locs | ||
in | ||
let annots = | ||
let main = User_message.make ~loc [ main_message ] in | ||
let related = | ||
List.map related_locs ~f:(fun loc -> | ||
User_message.make ~loc [ Pp.text "Used in this stanza" ]) | ||
let executables = | ||
match | ||
String.Map.of_list_map | ||
exes | ||
~f:(fun ((exes : Executables.t), _, m, obj_dir, _) -> | ||
snd (List.hd exes.names), (m, obj_dir)) | ||
with | ||
| Ok x -> x | ||
| Error (name, _, (exes2, _, _, _, _)) -> | ||
User_error.raise | ||
~loc:exes2.buildable.loc | ||
[ Pp.textf "Executable %S appears for the second time in this directory" name | ||
] | ||
in | ||
let melange_emits = | ||
match | ||
String.Map.of_list_map emits ~f:(fun (mel, _, m, obj_dir, _) -> | ||
mel.target, (m, obj_dir)) | ||
with | ||
| Ok x -> x | ||
| Error (name, _, (mel, _, _, _, _)) -> | ||
User_error.raise | ||
~loc:mel.loc | ||
[ Pp.textf "Target %S appears for the second time in this directory" name ] | ||
in | ||
let rev_map = | ||
let modules = | ||
let by_path (origin : Origin.t) trie = | ||
Module_trie.fold trie ~init:[] ~f:(fun (_loc, m) acc -> | ||
(Module.Source.path m, origin) :: acc) | ||
in | ||
User_message.Annots.singleton | ||
Compound_user_error.annot | ||
[ Compound_user_error.make ~main ~related ] | ||
List.concat | ||
[ List.concat_map libs ~f:(fun (l, m, _, _, _) -> by_path (Library l) m) | ||
; List.concat_map exes ~f:(fun (e, m, _, _, _) -> by_path (Executables e) m) | ||
; List.concat_map emits ~f:(fun (l, m, _, _, _) -> by_path (Melange l) m) | ||
] | ||
in | ||
User_error.raise | ||
~annots | ||
~loc:(Loc.drop_position loc) | ||
[ main_message | ||
; Pp.enumerate locs ~f:(fun loc -> Pp.verbatim (Loc.to_file_colon_line loc)) | ||
; Pp.text | ||
"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." | ||
] | ||
in | ||
{ libraries; executables; melange_emits; rev_map } | ||
match Module_name.Path.Map.of_list modules with | ||
| Ok x -> x | ||
| Error (path, _, _) -> | ||
let locs = | ||
List.filter_map modules ~f:(fun (n, origin) -> | ||
Option.some_if | ||
(Ordering.is_eq (Module_name.Path.compare n path)) | ||
(Origin.loc origin)) | ||
|> List.sort ~compare:Loc.compare | ||
in | ||
let main_message = | ||
Pp.textf | ||
"Module %S is used in several stanzas:" | ||
(Module_name.Path.to_string path) | ||
in | ||
let loc, related_locs = | ||
match locs with | ||
| [] -> | ||
(* duplicates imply at least at one module with this location *) | ||
assert false | ||
| loc :: related_locs -> loc, related_locs | ||
in | ||
let annots = | ||
let main = User_message.make ~loc [ main_message ] in | ||
let related = | ||
List.map related_locs ~f:(fun loc -> | ||
User_message.make ~loc [ Pp.text "Used in this stanza" ]) | ||
in | ||
User_message.Annots.singleton | ||
Compound_user_error.annot | ||
[ Compound_user_error.make ~main ~related ] | ||
in | ||
User_error.raise | ||
~annots | ||
~loc:(Loc.drop_position loc) | ||
[ main_message | ||
; Pp.enumerate locs ~f:(fun loc -> Pp.verbatim (Loc.to_file_colon_line loc)) | ||
; Pp.text | ||
"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." | ||
] | ||
in | ||
{ libraries; executables; melange_emits; rev_map } | ||
;; | ||
end | ||
|
||
|
@@ -393,6 +404,7 @@ let modules_of_stanzas = | |
Memo.parallel_map stanzas ~f:(fun stanza -> | ||
match Stanza.repr stanza with | ||
| Library.T lib -> | ||
let* enabled = Expander.eval_blang expander lib.enabled_if >>| Toggle.of_bool in | ||
(* jeremiedimino: this [Resolve.get] means that if the user writes an | ||
invalid [implements] field, we will get an error immediately even if | ||
the library is not built. We should change this to carry the | ||
|
@@ -411,9 +423,10 @@ let modules_of_stanzas = | |
>>= Resolve.read_memo | ||
in | ||
let obj_dir = Library.obj_dir lib ~dir in | ||
`Library (lib, sources, modules, obj_dir) | ||
`Library (lib, sources, modules, obj_dir, enabled) | ||
| Executables.T exes | Tests.T { exes; _ } -> | ||
let obj_dir = Executables.obj_dir ~dir exes in | ||
let* enabled = Expander.eval_blang expander exes.enabled_if >>| Toggle.of_bool in | ||
let+ sources, modules = | ||
let { Buildable.loc = stanza_loc; modules = modules_settings; _ } = | ||
exes.buildable | ||
|
@@ -434,9 +447,10 @@ let modules_of_stanzas = | |
then Modules_group.make_wrapped ~obj_dir ~modules `Exe | ||
else Modules_group.exe_unwrapped modules ~obj_dir | ||
in | ||
`Executables (exes, sources, modules, obj_dir) | ||
`Executables (exes, sources, modules, obj_dir, enabled) | ||
| Melange_stanzas.Emit.T mel -> | ||
let obj_dir = Obj_dir.make_melange_emit ~dir ~name:mel.target in | ||
let* enabled = Expander.eval_blang expander mel.enabled_if >>| Toggle.of_bool in | ||
let+ sources, modules = | ||
Modules_field_evaluator.eval | ||
~expander | ||
|
@@ -451,7 +465,7 @@ let modules_of_stanzas = | |
let modules = | ||
Modules_group.make_wrapped ~obj_dir:(Obj_dir.obj_dir obj_dir) ~modules `Melange | ||
in | ||
`Melange_emit (mel, sources, modules, obj_dir) | ||
`Melange_emit (mel, sources, modules, obj_dir, enabled) | ||
| _ -> Memo.return `Skip) | ||
>>| filter_partition_map | ||
;; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,13 @@ | |
> EOF | ||
|
||
$ dune build @melange --display short | ||
Error: Alias "melange" specified on the command line is empty. | ||
It is not defined in . or any of its descendants. | ||
[1] | ||
Comment on lines
+37
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These errors might be more meaningful to users than the previous behavior (not building anything because the alias is "empty", but not erroring either). |
||
|
||
No rules attached to the alias | ||
|
||
$ dune rules @melange | ||
Error: Alias "melange" specified on the command line is empty. | ||
It is not defined in . or any of its descendants. | ||
[1] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
enabled_if has a limitation: it attempts building even if enabled_if evaluates to false. | ||
enabled_if and build_if have similar behavior | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old behavior was in fact correct and this PR breaks it. The description of this test might be the source of confusion. There's no limitation anywhere, it's just that the different semantics were chosen for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be fixed in 050aed8. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just as a side note, if I'm understanding the current behavior properly, it is quite confusing from a user perspective.
I found this comment that explains the use case for it: #7899 (comment). It's probably too late, but wonder if it would have been possible to leave the behavior of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in hindsight it would be better to do it your way. We could revisit it for 4.0 |
||
$ cat > dune-project << EOF | ||
> (lang dune 3.9) | ||
|
@@ -39,7 +39,7 @@ We test the various combinations: | |
|
||
$ test_all | ||
When building @all with ENABLED=unset: | ||
build was done: YES | ||
build was done: NO | ||
test did run: NO | ||
When building @runtest with ENABLED=unset: | ||
build was done: NO | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you pass the toggle to artifacts_obj but you don't do anything with it, any reason for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used it originally, but I guess I can remove it now and just pass the right list of libs, exes and emits directly from the caller, if that's the better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's the better approach, so I'm looking at the code right to find out. I'll list my thought process below. You'll have to repeat in a few different places, so it might be useful.
First, I need to check where are the callers of
Artifacts_obj
that use a library name. Looks like there's only one caller of:That lives in Expander:
So now I think about what's going to happen if we pretend that a disabled library does not exist. Since this code is part of the expander, to reproduce this behavior I'd run the following command
dune build %{cma:dir/disabled-lib}
.From the perspective of the user, dune telling me this does not exist would be rather undesirable. So we need to improve the situation somehow. To do so, we need
Artifacts_obj.lookup_library
to propagate the library being disabled.I make the conclusion that your decision is indeed correct and we need to pass enabled/disabled to
Artifacts_obj
Hope that helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand, this process would help on the eventual case that in some future Dune sets "fake" rules for all the disabled libraries in the current context right? As we confirmed the other day that there's no need to set rules for disabled libs right now.
I also wonder, in this hypothetic future where there are fake rules for disabled libraries (for better user errors), how would we deal with the case where two libraries are defined in the same folder with the same name, but one defined on each context? I assume we would skip the creation of fake rules in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we discussed using fake rules to make the error messages better. And indeed the decision was to forego them for now, as it's quite complex to implement. However, there's other cases where we can improve the error messages without much complexity. Expanding percent forms like we have here is an obvious candidate.
In your hypothetical case we would skip the fake rules. As they would overlap with actual rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test case for this in #10245
Looks like we don't handle disabled libraries correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change requests didn't specify if this is something that should be fixed in this PR. Is that the case? In any case, the PR doesn't seem to change the current behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there's no need to fix it here.