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

eif: fix name collision in same folder for exes and melange emits #10220

Merged
merged 24 commits into from
Mar 13, 2024
Merged

eif: fix name collision in same folder for exes and melange emits #10220

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
286a3c0
eif: error on dups in ml_sources if both are enabled
jchavarri Mar 5, 2024
9216dad
eif: not add modules in disabled libs
jchavarri Mar 5, 2024
e02387a
eif: use Toggle.t instead of a custom type
jchavarri Mar 6, 2024
3524130
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 6, 2024
bd7b0f9
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 7, 2024
bdbdeff
simplify implementation
jchavarri Mar 7, 2024
50acad1
fix melange emit case
jchavarri Mar 8, 2024
50d5bee
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 8, 2024
5189cc4
add explanatory comment for usage of enabled in melange_rules
jchavarri Mar 8, 2024
673ed9a
update tests
jchavarri Mar 8, 2024
ca2909c
remove unnecessary changes in setup_emit_js_rules
jchavarri Mar 8, 2024
2c98f8f
code review suggested changes
jchavarri Mar 10, 2024
ecbad4f
update src/dune_rules/ml_sources.ml
jchavarri Mar 10, 2024
ef6c146
fmt
jchavarri Mar 10, 2024
74bca3f
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 10, 2024
050aed8
fix change in enabled_if behavior for test stanzas
jchavarri Mar 10, 2024
623ada2
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 10, 2024
16471fe
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 12, 2024
20e02e3
_
rgrinberg Mar 12, 2024
1f17f12
_
rgrinberg Mar 12, 2024
e2b665f
_
rgrinberg Mar 12, 2024
a07ee5f
Merge branch 'main' into improve-ml-sources-same-folder-collision
jchavarri Mar 13, 2024
b22ae6f
update tests
jchavarri Mar 13, 2024
ebfe399
+ changelog
jchavarri Mar 13, 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
6 changes: 3 additions & 3 deletions src/dune_rules/artifacts_obj.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ let lookup_library { libraries; modules = _ } = Lib_name.Map.find libraries

let make ~dir ~lib_config ~libs ~exes =
let+ libraries =
Memo.List.map libs ~f:(fun ((lib : Library.t), _, _, _) ->
Memo.List.map libs ~f:(fun ((lib : Library.t), _, _, _, _) ->
let+ lib_config = lib_config in
let name = Lib_name.of_local lib.name in
let info = Library.to_lib_info lib ~dir ~lib_config in
Expand All @@ -28,9 +28,9 @@ let make ~dir ~lib_config ~libs ~exes =
List.fold_left
exes
~init:Module_name.Map.empty
~f:(fun modules (_, _, m, obj_dir) -> by_name modules obj_dir m)
~f:(fun modules (_, _, m, obj_dir, _) -> by_name modules obj_dir m)
in
List.fold_left libs ~init ~f:(fun modules (_, _, m, obj_dir) ->
List.fold_left libs ~init ~f:(fun modules (_, _, m, obj_dir, _) ->
by_name modules obj_dir m)
in
{ libraries; modules }
Expand Down
4 changes: 2 additions & 2 deletions src/dune_rules/artifacts_obj.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ val empty : t
val make
: dir:Path.Build.t
-> lib_config:Lib_config.t Memo.t
-> libs:(Library.t * _ * Modules.t * Path.Build.t Obj_dir.t) list
-> exes:(_ * _ * Modules.t * Path.Build.t Obj_dir.t) list
-> libs:(Library.t * _ * Modules.t * Path.Build.t Obj_dir.t * Toggle.t) list
-> exes:(_ * _ * Modules.t * Path.Build.t Obj_dir.t * Toggle.t) list
Copy link
Member

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?

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 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.

Copy link
Member

@rgrinberg rgrinberg Mar 8, 2024 •
edited
Loading

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:

val lookup_library : t -> Lib_name.t -> Lib_info.local option

That lives in Expander:

    (match Artifacts_obj.lookup_library artifacts name with
     | None -> does_not_exist ~what:"Library" (Lib_name.to_string name)
     | Some lib ->
       Mode.Dict.get (Lib_info.archives lib) mode
       |> Action_builder.List.map ~f:(fun fn ->
         let fn = Path.build fn in
         let+ () = Action_builder.path fn in
         Value.Path fn))

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.

Copy link
Collaborator Author

@jchavarri jchavarri Mar 8, 2024 •
edited
Loading

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?

Copy link
Member

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.

Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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.

-> t Memo.t

val lookup_module : t -> Module_name.t -> (Path.Build.t Obj_dir.t * Module.t) option
Expand Down
42 changes: 31 additions & 11 deletions src/dune_rules/melange/melange_rules.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,28 +581,38 @@ let emit_rules sctx { stanza_dir; stanza } =
;;

(* Detect if [dir] is under the target directory of a melange.emit stanza. *)
let rec under_melange_emit_target ~dir =
let rec under_melange_emit_target ~sctx ~dir =
match Path.Build.parent dir with
| None -> Memo.return None
| Some parent ->
Dune_load.stanzas_in_dir parent
>>= (function
| None -> under_melange_emit_target ~dir:parent
| None -> under_melange_emit_target ~sctx ~dir:parent
| Some stanzas ->
Dune_file.find_stanzas stanzas Melange_stanzas.Emit.key
>>| List.find_map ~f:(fun mel ->
>>= Memo.List.find_map ~f:(fun (mel : Melange_stanzas.Emit.t) ->
(* In the case where we have two melange.emit stanzas in the same folder,
with one enabled in the current context and one disabled, we want to
make sure that we pick the enabled one *)
let+ enabled =
let* expander =
let* sctx = sctx in
Super_context.expander sctx ~dir
in
Expander.eval_blang expander mel.enabled_if
in
let target_dir = Melange_stanzas.Emit.target_dir ~dir:parent mel in
Option.some_if (Path.Build.equal target_dir dir) mel)
Option.some_if (enabled && Path.Build.equal target_dir dir) mel)
>>= (function
| None -> under_melange_emit_target ~dir:parent
| None -> under_melange_emit_target ~sctx ~dir:parent
| Some stanza -> Memo.return @@ Some { stanza_dir = parent; stanza }))
;;

let gen_emit_rules sctx ~dir ({ stanza_dir; stanza } as for_melange) =
match Path.Build.equal dir (Melange_stanzas.Emit.target_dir ~dir:stanza_dir stanza) with
| false -> Memo.return None
| true ->
under_melange_emit_target ~dir:stanza_dir
under_melange_emit_target ~sctx ~dir:stanza_dir
>>| (function
| None -> Some (emit_rules sctx for_melange)
| Some { stanza_dir = parent_melange_emit_dir; stanza = parent_stanza } ->
Expand Down Expand Up @@ -640,13 +650,23 @@ let gen_emit_rules sctx ~dir ({ stanza_dir; stanza } as for_melange) =
module Gen_rules = Import.Build_config.Gen_rules

let setup_emit_js_rules sctx ~dir =
under_melange_emit_target ~dir
under_melange_emit_target ~sctx ~dir
>>= function
| Some melange ->
gen_emit_rules sctx ~dir melange
>>| (function
| None -> Gen_rules.redirect_to_parent Gen_rules.Rules.empty
| Some melange -> Gen_rules.make melange)
let* enabled =
let* expander =
let* sctx = sctx in
Super_context.expander sctx ~dir
in
Expander.eval_blang expander melange.stanza.enabled_if
in
(match enabled with
| true ->
gen_emit_rules sctx ~dir melange
>>| (function
| None -> Gen_rules.redirect_to_parent Gen_rules.Rules.empty
| Some melange -> Gen_rules.make melange)
| false -> Memo.return (Gen_rules.redirect_to_parent Gen_rules.Rules.empty))
| None ->
(* this should probably be handled by [Dir_status] *)
Dune_load.stanzas_in_dir dir
Expand Down
37 changes: 25 additions & 12 deletions src/dune_rules/ml_sources.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ module Modules = struct
* (Loc.t * Module.Source.t) Module_trie.t
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

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 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
Expand All @@ -53,13 +54,19 @@ module Modules = struct
}

let make { libraries = libs; executables = exes; melange_emits = emits } =
let keep_enabled t =
List.filter t ~f:(fun (_, _, _, _, enabled) -> Toggle.enabled enabled)
in
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
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) ->
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, _, _, _)) ->
| Error (name, _, (lib2, _, _, _, _)) ->
User_error.raise
~loc:lib2.buildable.loc
[ Pp.textf
Expand All @@ -69,22 +76,22 @@ module Modules = struct
in
let executables =
match
String.Map.of_list_map exes ~f:(fun ((exes : Executables.t), _, m, obj_dir) ->
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, _, _, _)) ->
| 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) ->
String.Map.of_list_map emits ~f:(fun (mel, _, m, obj_dir, _) ->
mel.target, (m, obj_dir))
with
| Ok x -> x
| Error (name, _, (mel, _, _, _)) ->
| Error (name, _, (mel, _, _, _, _)) ->
User_error.raise
~loc:mel.loc
[ Pp.textf "Target %S appears for the second time in this directory" name ]
Expand All @@ -96,9 +103,9 @@ module Modules = struct
(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)
[ 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
match Module_name.Path.Map.of_list modules with
Expand Down Expand Up @@ -393,6 +400,8 @@ 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 in
let enabled = Toggle.of_bool enabled in
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
(* 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
Expand All @@ -411,9 +420,11 @@ 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 in
let enabled = Toggle.of_bool enabled in
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
let+ sources, modules =
let { Buildable.loc = stanza_loc; modules = modules_settings; _ } =
exes.buildable
Expand All @@ -434,9 +445,11 @@ 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 in
let enabled = Toggle.of_bool enabled in
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
let+ sources, modules =
Modules_field_evaluator.eval
~expander
Expand All @@ -451,7 +464,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
;;
Expand Down
6 changes: 0 additions & 6 deletions test/blackbox-tests/test-cases/enabled_if/eif-exe-name-collision-same-folder.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,3 @@ in the same dune file
> EOF

$ dune build
File "dune", line 4, characters 0-72:
4 | (executable
5 | (name foo)
6 | (enabled_if (= %{context_name} "alt-context")))
Error: Executable "foo" appears for the second time in this directory
[1]
6 changes: 0 additions & 6 deletions test/blackbox-tests/test-cases/enabled_if/eif-melange-emit-name-collision-same-folder.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,3 @@ in the same dune file
> EOF

$ dune build
File "dune", line 4, characters 0-76:
4 | (melange.emit
5 | (target foo)
6 | (enabled_if (= %{context_name} "alt-context")))
Error: Target "foo" appears for the second time in this directory
[1]
6 changes: 6 additions & 0 deletions test/blackbox-tests/test-cases/melange/enabled_if.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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]
4 changes: 2 additions & 2 deletions test/blackbox-tests/test-cases/test-build-if/feature.t
View file
Open in desktop
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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @emillon. This PR seems to fix the case found in #7899.

Copy link
Member

Choose a reason for hiding this comment

The 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 enabled_if on tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed in 050aed8.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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. enabled_if semantics are now different depending on where it's used:

  • In executable stanzas, the field means the executable is not built if it evaluates to false
  • In test stanzas:
    • when all alias is used, the field means the test executable is always built, regardless its evaluated value
    • but for runtest alias it means the executable will not be built if it evaluates to false (like executable stanzas)

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 enabled_if consistent, and allow advanced use cases to use a combination of rule and executable, or add a new field test_if.

Copy link
Member

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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
Expand Down
Loading