Skip to content

fix(melange + include_qualified): track correct .cmj dependencies in emit #12531

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+39 −8
Open

fix(melange + include_qualified): track correct .cmj dependencies in emit #12531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/dune_rules/dep_rules.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,7 @@ let rules ~obj_dir ~modules ~sandbox ~impl ~sctx ~dir =
in
Dep_graph.make ~dir ~per_module)
;;

let deps_of ~obj_dir ~modules ~sandbox ~impl ~dir ~sctx module_ =
deps_of ~obj_dir ~modules ~sandbox ~impl ~dir ~sctx (Normal module_)
;;
11 changes: 11 additions & 0 deletions src/dune_rules/dep_rules.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,14 @@ val rules
-> sctx:Super_context.t
-> dir:Path.Build.t
-> Dep_graph.Ml_kind.t Memo.t

val deps_of
: obj_dir:Path.Build.t Obj_dir.t
-> modules:Modules.With_vlib.t
-> sandbox:Sandbox_config.t
-> impl:Virtual_rules.t
-> dir:Path.Build.t
-> sctx:Super_context.t
-> Module.t
-> ml_kind:Ml_kind.t
-> Module.t list Action_builder.t Memo.t
30 changes: 23 additions & 7 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 @@ -320,13 +320,29 @@ let build_js
; Dep src
]
in
With_targets.map_build command ~f:(fun command ->
let open Action_builder.O in
match local_modules_and_obj_dir with
| Some (modules, obj_dir) ->
match local_modules_and_obj_dir with
| Some (modules, obj_dir) ->
With_targets.map_build command ~f:(fun command ->
let open Action_builder.O in
let paths =
let+ module_deps =
Dep_rules.immediate_deps_of m modules ~obj_dir ~ml_kind:Impl
let deps =
let open Memo.O in
let+ deps, _ =
Memo.Implicit_output.collect Rules.implicit_output (fun () ->
Dep_rules.deps_of
~obj_dir
~modules
~sandbox:Sandbox_config.default
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit sucky to generate all these rules just to drop them. It would be better to factor out deps_of in a way so that we don't have to do 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.

what do you suggest doing here? I don't fully understand what rules are being generated.

Happy to do it in a follow-up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually it sounds like we'll have to do it here anyway. my current plan is to thread the rules as a return value of build_js back to gen_rules

Copy link
Member

Choose a reason for hiding this comment

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

what do you suggest doing here? I don't fully understand what rules are being generated.

Sure, I can clarify. I'm suggesting an almost mechanical transformation that is equivalent to what this PR is doing:

  1. Take deps_of and copy paste it to a new function
  2. In this new function, identify all the calls to add_rule and delete them
  3. For all the functions being called in your new deps_of, identify the ones with add_rule and repeat step 1.
  4. Once this process is done and you have deps_of that creates no rules, rename the original function to be gen_deps_of_rules
  5. Factor out all the copy pasting.

Hopefully the end result will give us two functions that share as much work as possible.

my current plan is to thread the rules as a return value of build_js back to gen_rules

That's possible as well, but there's a reason why we avoid doing this when possible. It leads to a massive amount of boilerplate managing this list of rules at every step.

~impl:Virtual_rules.no_implements
~dir
~sctx
~ml_kind:Impl
m)
in
deps
in
Action_builder.of_memo_join deps
in
List.fold_left module_deps ~init:[] ~f:(fun acc dep_m ->
if Module.has dep_m ~ml_kind:Impl
Expand All @@ -338,8 +354,8 @@ let build_js
cmj_file :: acc)
else acc)
in
Action_builder.dyn_paths_unit paths >>> command
| None -> command)
Action_builder.dyn_paths_unit paths >>> command)
| None -> command
in
Super_context.add_rule sctx ~dir ~loc ~mode build)
;;
Expand Down
2 changes: 1 addition & 1 deletion test/blackbox-tests/test-cases/melange/empty-aliases-file.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ file, and it's not present
> let x = "foo"
> EOF

$ DUNE_SANDBOX=none dune build @melange
$ dune build @melange

`foo.js` was manually written, therefore it's present. `sub.js` is an alias
file, and it's not present
Expand Down
Loading