From f9c3dc4117067960e75e0eb6e2e9a52c023cc048 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Tue, 19 Mar 2024 17:07:05 -0700 Subject: [PATCH 1/6] refactor: add labeled arguments to `Dep_rules.immediate_deps_of` Signed-off-by: Antonio Nuno Monteiro --- bin/describe/describe_workspace.ml | 4 +++- src/dune_rules/dep_rules.ml | 2 +- src/dune_rules/dep_rules.mli | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bin/describe/describe_workspace.ml b/bin/describe/describe_workspace.ml index 8f93a6ef79f..27ff861aedd 100644 --- a/bin/describe/describe_workspace.ml +++ b/bin/describe/describe_workspace.ml @@ -337,7 +337,9 @@ module Crawl = struct | { with_deps = false; _ } -> Action_builder.return { Ocaml.Ml_kind.Dict.intf = []; impl = [] } | { with_deps = true; _ } -> - let deps = Dune_rules.Dep_rules.immediate_deps_of unit modules obj_dir in + let deps ml_kind = + Dune_rules.Dep_rules.immediate_deps_of unit modules ~obj_dir ~ml_kind + in let open Action_builder.O in let+ intf, impl = Action_builder.both (deps Intf) (deps Impl) in { Ocaml.Ml_kind.Dict.intf; impl } diff --git a/src/dune_rules/dep_rules.ml b/src/dune_rules/dep_rules.ml index e21bc5cce99..774c06e9af7 100644 --- a/src/dune_rules/dep_rules.ml +++ b/src/dune_rules/dep_rules.ml @@ -129,7 +129,7 @@ let rec deps_of md ~ml_kind (m : Modules.Sourced_module.t) = (** Tests whether a set of modules is a singleton *) let has_single_file modules = Option.is_some @@ Modules.as_singleton modules -let immediate_deps_of unit modules obj_dir ml_kind = +let immediate_deps_of unit modules ~obj_dir ~ml_kind = match Module.kind unit with | Alias _ -> Action_builder.return [] | Wrapped_compat -> diff --git a/src/dune_rules/dep_rules.mli b/src/dune_rules/dep_rules.mli index d5315b4456d..2f5b6bbc432 100644 --- a/src/dune_rules/dep_rules.mli +++ b/src/dune_rules/dep_rules.mli @@ -10,8 +10,8 @@ val for_module val immediate_deps_of : Module.t -> Modules.t - -> Path.Build.t Obj_dir.t - -> Ml_kind.t + -> obj_dir:Path.Build.t Obj_dir.t + -> ml_kind:Ml_kind.t -> Module.t list Action_builder.t val rules : Ocamldep.Modules_data.t -> Dep_graph.t Ml_kind.Dict.t Memo.t From ccf34cabd9aff6d2f5d9ee7ea790d977bd038478 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Tue, 19 Mar 2024 17:11:29 -0700 Subject: [PATCH 2/6] fix(melange): track immediate `.cmj` dependents as dependencies of JS rules Signed-off-by: Antonio Nuno Monteiro --- src/dune_rules/melange/melange_rules.ml | 106 ++++++++++++------ .../melange/library-include-subdirs.t | 17 +-- 2 files changed, 80 insertions(+), 43 deletions(-) diff --git a/src/dune_rules/melange/melange_rules.ml b/src/dune_rules/melange/melange_rules.ml index c9e29670093..1a34701017e 100644 --- a/src/dune_rules/melange/melange_rules.ml +++ b/src/dune_rules/melange/melange_rules.ml @@ -152,37 +152,62 @@ let js_targets_of_libs sctx libs ~module_systems ~target_dir = List.rev_append for_vlib base)) ;; -let build_js ~loc ~dir ~pkg_name ~mode ~module_systems ~output ~obj_dir ~sctx ~includes m = +let build_js + ~loc + ~dir + ~pkg_name + ~mode + ~module_systems + ~output + ~obj_dir + ~sctx + ~includes + ~local_modules + m + = let open Memo.O in let* compiler = Melange_binary.melc sctx ~loc:(Some loc) ~dir in Memo.parallel_iter module_systems ~f:(fun (module_system, js_ext) -> - let src = Obj_dir.Module.cm_file_exn obj_dir m ~kind:(Melange Cmj) in - let output = make_js_name ~output ~js_ext m in - let obj_dir = [ Command.Args.A "-I"; Path (Obj_dir.melange_dir obj_dir) ] in - let melange_package_args = - let pkg_name_args = - match pkg_name with - | None -> [] - | Some pkg_name -> [ "--bs-package-name"; Package.Name.to_string pkg_name ] + let build = + let command = + let src = Obj_dir.Module.cm_file_exn obj_dir m ~kind:(Melange Cmj) in + let output = make_js_name ~output ~js_ext m in + let obj_dir = [ Command.Args.A "-I"; Path (Obj_dir.melange_dir obj_dir) ] in + let melange_package_args = + let pkg_name_args = + match pkg_name with + | None -> [] + | Some pkg_name -> [ "--bs-package-name"; Package.Name.to_string pkg_name ] + in + let js_modules_str = Melange.Module_system.to_string module_system in + "--bs-module-type" :: js_modules_str :: pkg_name_args + in + Command.run + ~dir:(Super_context.context sctx |> Context.build_dir |> Path.build) + compiler + [ Command.Args.S obj_dir + ; Command.Args.as_any includes + ; As melange_package_args + ; A "-o" + ; Target output + ; Dep src + ] in - let js_modules_str = Melange.Module_system.to_string module_system in - "--bs-module-type" :: js_modules_str :: pkg_name_args + With_targets.map_build command ~f:(fun command -> + let open Action_builder.O in + let local_library_paths = + match local_modules with + | Some (modules, obj_dir) -> + let+ module_deps = + Dep_rules.immediate_deps_of m modules ~obj_dir ~ml_kind:Impl + in + List.map module_deps ~f:(fun dep_m -> + Obj_dir.Module.cm_file_exn obj_dir dep_m ~kind:(Melange Cmj) |> Path.build) + | None -> Action_builder.return [] + in + Action_builder.dyn_paths_unit local_library_paths >>> command) in - Super_context.add_rule - sctx - ~dir - ~loc - ~mode - (Command.run - ~dir:(Super_context.context sctx |> Context.build_dir |> Path.build) - compiler - [ Command.Args.S obj_dir - ; Command.Args.as_any includes - ; As melange_package_args - ; A "-o" - ; Target output - ; Dep src - ])) + Super_context.add_rule sctx ~dir ~loc ~mode build) ;; (* attach [deps] to the specified [alias] AND the (dune default) [all] alias. @@ -423,7 +448,18 @@ let setup_entries_js setup_runtime_assets_rules sctx ~dir ~target_dir ~mode ~output ~for_:`Emit mel in Memo.parallel_iter modules_for_js ~f:(fun m -> - build_js ~dir ~loc ~pkg_name ~mode ~module_systems ~output ~obj_dir ~sctx ~includes m) + build_js + ~dir + ~loc + ~pkg_name + ~mode + ~module_systems + ~output + ~obj_dir + ~sctx + ~includes + ~local_modules:None + m) ;; let setup_js_rules_libraries @@ -451,12 +487,18 @@ let setup_js_rules_libraries let pkg_name = Lib_info.package info in build_js ~loc ~pkg_name ~obj_dir in + let output = output_of_lib ~target_dir lib in let* includes = let+ requires_link = Memo.Lazy.force (Lib.Compile.requires_link lib_compile_info) in cmj_includes ~requires_link ~scope - in - let output = output_of_lib ~target_dir lib in - let* () = + and* local_modules = + match Lib.Local.of_lib lib with + | Some lib -> + let+ modules = Dir_contents.modules_of_local_lib sctx lib in + let obj_dir = Lib.Local.obj_dir lib in + Some (modules, obj_dir) + | None -> Memo.return None + and* () = setup_runtime_assets_rules sctx ~dir @@ -498,10 +540,10 @@ let setup_js_rules_libraries cmj_includes ~requires_link ~scope in impl_only_modules_defined_in_this_lib sctx vlib - >>= Memo.parallel_iter ~f:(build_js ~dir ~output ~includes) + >>= Memo.parallel_iter ~f:(build_js ~dir ~output ~includes ~local_modules) in let* source_modules = impl_only_modules_defined_in_this_lib sctx lib in - Memo.parallel_iter source_modules ~f:(build_js ~dir ~output ~includes)) + Memo.parallel_iter source_modules ~f:(build_js ~dir ~output ~local_modules ~includes)) ;; let setup_js_rules_libraries_and_entries diff --git a/test/blackbox-tests/test-cases/melange/library-include-subdirs.t b/test/blackbox-tests/test-cases/melange/library-include-subdirs.t index 3fb7d6ad96f..61cf49d50af 100644 --- a/test/blackbox-tests/test-cases/melange/library-include-subdirs.t +++ b/test/blackbox-tests/test-cases/melange/library-include-subdirs.t @@ -38,24 +38,19 @@ Melange shows the proper path to `bar.js` let Foo__Bar = require("./init/bar.js"); $ mv lib/init lib/end - $ dune build @mel -The import in `foo.js` still shows the initial path to `bar.js`, but the file is not there anymore +The import in `foo.js` has been updated to the new bar.js target $ cat _build/default/output/lib/foo.js | grep bar.js - let Foo__Bar = require("./init/bar.js"); + let Foo__Bar = require("./end/bar.js"); + +The initial file is not there anymore $ test -f _build/default/output/lib/init/bar.js [1] - $ test -f _build/default/output/lib/end/bar.js - -After removal of the js artifact, the path in `bar.js` import is correct +But the new one is - $ rm _build/default/output/lib/foo.js - - $ dune build @mel + $ test -f _build/default/output/lib/end/bar.js - $ cat _build/default/output/lib/foo.js | grep bar.js - let Foo__Bar = require("./end/bar.js"); From bbe3a3610b88b27ffdb036e97fde03bd9e417c3a Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Tue, 19 Mar 2024 17:12:51 -0700 Subject: [PATCH 3/6] test(melange): show outdated paths for include_subdirs and melange.emit Signed-off-by: Antonio Nuno Monteiro --- .../melange/library-include-subdirs.t | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/blackbox-tests/test-cases/melange/library-include-subdirs.t b/test/blackbox-tests/test-cases/melange/library-include-subdirs.t index 61cf49d50af..4f69b84ef21 100644 --- a/test/blackbox-tests/test-cases/melange/library-include-subdirs.t +++ b/test/blackbox-tests/test-cases/melange/library-include-subdirs.t @@ -54,3 +54,46 @@ But the new one is $ test -f _build/default/output/lib/end/bar.js +Now try the same thing with `melange.emit` + + $ rm -rf lib + $ cat > dune < (include_subdirs unqualified) + > (melange.emit + > (target output) + > (alias mel) + > (emit_stdlib false) + > (preprocess (pps melange.ppx))) + > EOF + + $ mkdir init + $ cat > init/bar.ml < let name = "Zoe" + > EOF + $ cat > foo.ml < let name = Bar.name + > EOF + + $ dune build @mel + +Melange shows the proper path to `bar.js` + + $ cat _build/default/output/foo.js | grep bar.js + let Melange__Bar = require("./init/bar.js"); + + $ mv init end + $ dune build @mel + +The import in `foo.js` has been updated to the new bar.js target + + $ cat _build/default/output/foo.js | grep bar.js + let Melange__Bar = require("./init/bar.js"); + +The initial file is not there anymore + + $ test -f _build/default/output/init/bar.js + [1] + +But the new one is + + $ test -f _build/default/output/end/bar.js From 387fe04e5b597d7501af94357b5f1eab9337a869 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Tue, 19 Mar 2024 17:14:33 -0700 Subject: [PATCH 4/6] fix(melange): track immediate `.cmj` dependents in `melange.emit` too Signed-off-by: Antonio Nuno Monteiro --- src/dune_rules/melange/melange_rules.ml | 11 ++++++----- .../test-cases/melange/library-include-subdirs.t | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/dune_rules/melange/melange_rules.ml b/src/dune_rules/melange/melange_rules.ml index 1a34701017e..1037baaded4 100644 --- a/src/dune_rules/melange/melange_rules.ml +++ b/src/dune_rules/melange/melange_rules.ml @@ -419,7 +419,7 @@ let modules_for_js_and_obj_dir ~sctx ~dir_contents ~scope (mel : Melange_stanzas Modules.fold_no_vlib modules ~init:[] ~f:(fun x acc -> if Module.has x ~ml_kind:Impl then x :: acc else acc) in - modules_for_js, obj_dir + modules, modules_for_js, obj_dir ;; let setup_entries_js @@ -433,7 +433,7 @@ let setup_entries_js (mel : Melange_stanzas.Emit.t) = let open Memo.O in - let* modules_for_js, obj_dir = + let* local_modules, modules_for_js, local_obj_dir = modules_for_js_and_obj_dir ~sctx ~dir_contents ~scope mel in let requires_link = Lib.Compile.requires_link compile_info in @@ -443,11 +443,12 @@ let setup_entries_js let* requires_link = Memo.Lazy.force requires_link in let includes = cmj_includes ~requires_link ~scope in let output = `Private_library_or_emit target_dir in - let obj_dir = Obj_dir.of_local obj_dir in + let obj_dir = Obj_dir.of_local local_obj_dir in let* () = setup_runtime_assets_rules sctx ~dir ~target_dir ~mode ~output ~for_:`Emit mel in Memo.parallel_iter modules_for_js ~f:(fun m -> + let local_modules = Some (local_modules, local_obj_dir) in build_js ~dir ~loc @@ -458,7 +459,7 @@ let setup_entries_js ~obj_dir ~sctx ~includes - ~local_modules:None + ~local_modules m) ;; @@ -599,7 +600,7 @@ let setup_emit_js_rules ~dir_contents ~dir ~scope ~sctx mel = package). When resolution fails, we replace the JS entries with the resolution error inside [Action_builder.fail] to give Dune a chance to fail if any of the targets end up attached to a package installation. *) - let* modules_for_js, _obj_dir = + let* _local_modules, modules_for_js, _obj_dir = modules_for_js_and_obj_dir ~sctx ~dir_contents ~scope mel in let module_systems = mel.module_systems in diff --git a/test/blackbox-tests/test-cases/melange/library-include-subdirs.t b/test/blackbox-tests/test-cases/melange/library-include-subdirs.t index 4f69b84ef21..d95c3794cf3 100644 --- a/test/blackbox-tests/test-cases/melange/library-include-subdirs.t +++ b/test/blackbox-tests/test-cases/melange/library-include-subdirs.t @@ -87,7 +87,7 @@ Melange shows the proper path to `bar.js` The import in `foo.js` has been updated to the new bar.js target $ cat _build/default/output/foo.js | grep bar.js - let Melange__Bar = require("./init/bar.js"); + let Melange__Bar = require("./end/bar.js"); The initial file is not there anymore From 34ade5fbc0e9696ca9de48f849f40b384572e1ac Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Tue, 19 Mar 2024 17:49:29 -0700 Subject: [PATCH 5/6] chore: add a changelog entry Signed-off-by: Antonio Nuno Monteiro --- doc/changes/10286.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 doc/changes/10286.md diff --git a/doc/changes/10286.md b/doc/changes/10286.md new file mode 100644 index 00000000000..e2e6d56e6cf --- /dev/null +++ b/doc/changes/10286.md @@ -0,0 +1,3 @@ +- melange: fix a bug that would cause stale `import` paths to be emitted when + moving source files within `(include_subdirs ..)` + (#10286, fixes #9190, @anmonteiro) From 58dd3ba78f3b26d8566ca258b2907270942d5868 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Tue, 19 Mar 2024 19:55:29 -0700 Subject: [PATCH 6/6] fix: symlink `.impl.d` files for virtual library modules too Signed-off-by: Antonio Nuno Monteiro --- src/dune_rules/virtual_rules.ml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/dune_rules/virtual_rules.ml b/src/dune_rules/virtual_rules.ml index 866e4be43ca..f6f4011c0b6 100644 --- a/src/dune_rules/virtual_rules.ml +++ b/src/dune_rules/virtual_rules.ml @@ -23,6 +23,14 @@ let setup_copy_rules_for_impl ~sctx ~dir vimpl = let dst = Obj_dir.Module.cm_file_exn impl_obj_dir m ~kind in copy_to_obj_dir ~src ~dst in + let copy_ocamldep_file m = + match Obj_dir.to_local vlib_obj_dir with + | None -> Memo.return () + | Some vlib_obj_dir -> + let src = Obj_dir.Module.dep vlib_obj_dir (Immediate (m, Impl)) |> Path.build in + let dst = Obj_dir.Module.dep impl_obj_dir (Immediate (m, Impl)) in + copy_to_obj_dir ~src ~dst + in let copy_interface_to_impl ~src kind () = let dst = Obj_dir.Module.cm_public_file_exn impl_obj_dir src ~kind in let src = Obj_dir.Module.cm_public_file_exn vlib_obj_dir src ~kind in @@ -39,7 +47,8 @@ let setup_copy_rules_for_impl ~sctx ~dir vimpl = >>> Memo.when_ melange (copy_interface_to_impl ~src (Melange Cmi))) >>> Memo.when_ (Module.has src ~ml_kind:Impl) (fun () -> Memo.when_ byte (fun () -> copy_obj_file src (Ocaml Cmo)) - >>> Memo.when_ melange (fun () -> copy_obj_file src (Melange Cmj)) + >>> Memo.when_ melange (fun () -> + copy_obj_file src (Melange Cmj) >>> copy_ocamldep_file src) >>> Memo.when_ native (fun () -> copy_obj_file src (Ocaml Cmx) >>>