diff --git a/src/dune_rules/compilation_context.ml b/src/dune_rules/compilation_context.ml index e9bd6ae1a3e..e523354beae 100644 --- a/src/dune_rules/compilation_context.ml +++ b/src/dune_rules/compilation_context.ml @@ -259,6 +259,19 @@ let create let profile = Context.profile context in eval_opaque ocaml profile opaque in + let* has_library_deps = + (* Single-module stanzas still run ocamldep when they have library deps so + the per-module filter can inform the result. *) + let open Resolve.Memo.O in + let+ direct = direct_requires + and+ hidden = hidden_requires in + match direct, hidden with + | [], [] -> false + | _ -> true + in + (* Resolution errors surface later through the normal compilation rules; + assume deps are present here. *) + let has_library_deps = Resolve.peek has_library_deps |> Result.value ~default:true in let+ dep_graphs = Dep_rules.rules ~dir:(Obj_dir.dir obj_dir) @@ -268,6 +281,7 @@ let create ~impl:implements ~modules ~for_ + ~has_library_deps and+ bin_annot = match bin_annot with | Some b -> Memo.return b diff --git a/src/dune_rules/dep_rules.ml b/src/dune_rules/dep_rules.ml index b9e70e4768d..5dc2272211b 100644 --- a/src/dune_rules/dep_rules.ml +++ b/src/dune_rules/dep_rules.ml @@ -504,10 +504,14 @@ let for_module ~obj_dir ~modules ~sandbox ~impl ~dir ~sctx ~for_ module_ = (deps_of ~modules ~transitive_deps ~imported_vlib_deps (Normal module_)) ;; -let rules ~obj_dir ~modules ~sandbox ~impl ~sctx ~dir ~for_ = +let rules ~obj_dir ~modules ~sandbox ~impl ~sctx ~dir ~for_ ~has_library_deps = match Modules.With_vlib.as_singleton modules with - | Some m -> Memo.return (Dep_graph.Ml_kind.dummy m) - | None -> + | Some m when (not has_library_deps) || Compilation_mode.equal for_ Melange -> + (* Single-module stanzas have no intra-stanza deps; the dep graph is only + consumed by the per-module filter in [lib_deps_for_module]. That filter + doesn't activate for Melange (see its [can_filter]) — skip ocamldep. *) + Memo.return (Dep_graph.Ml_kind.dummy m) + | Some _ | None -> let transitive_deps, imported_vlib_deps = make_transitive_deps ~obj_dir ~modules ~sandbox ~impl ~dir ~sctx ~for_ in diff --git a/src/dune_rules/dep_rules.mli b/src/dune_rules/dep_rules.mli index 8e1324d6050..7390f136291 100644 --- a/src/dune_rules/dep_rules.mli +++ b/src/dune_rules/dep_rules.mli @@ -13,6 +13,9 @@ val for_module -> Module.t -> Module.t list Action_builder.t Ml_kind.Dict.t Memo.t +(** Single-module stanzas short-circuit ocamldep when [not has_library_deps] or + [for_ = Melange]: the per-module filter that consumes the dep graph doesn't + activate in those cases. *) val rules : obj_dir:Path.Build.t Obj_dir.t -> modules:Modules.With_vlib.t @@ -21,6 +24,7 @@ val rules -> sctx:Super_context.t -> dir:Path.Build.t -> for_:Compilation_mode.t + -> has_library_deps:bool -> Dep_graph.Ml_kind.t Memo.t val read_immediate_deps_of diff --git a/src/dune_rules/module_compilation.ml b/src/dune_rules/module_compilation.ml index d2c1556bc58..557416cd92a 100644 --- a/src/dune_rules/module_compilation.ml +++ b/src/dune_rules/module_compilation.ml @@ -8,27 +8,179 @@ let all_libs cctx = d @ h ;; -(* Returns the include flags and lib-file deps for compiling [m]. The scaffold - form is glob-only: include flags come from the cctx's [Includes] (-I/-H over - the full lib list) and deps come from [deps_of_entries] over the same list. - The per-module tight filter activates in a follow-up; arguments unused by - this form are reserved for that filter. *) -let lib_deps_for_module - ~cctx - ~obj_dir:_ - ~for_:_ - ~dep_graph:_ - ~opaque - ~cm_kind - ~ml_kind:(_ : Ml_kind.t) - ~mode:(_ : Lib_mode.t) - _m - = +let union_module_name_sets_mapped xs ~f = + Action_builder.List.map xs ~f + |> Action_builder.map + ~f:(List.fold_left ~init:Module_name.Set.empty ~f:Module_name.Set.union) +;; + +let module_kind_is_filterable m = + match Module.kind m with + | Root | Wrapped_compat | Impl_vmodule | Virtual | Parameter -> false + | Intf_only | Impl | Alias _ -> true +;; + +(* BFS over tight-eligible entries: each (lib, entry) pair's impl+intf ocamldep + names extend the frontier. Non-tight-eligible libs (wrapped locals, + externals, virtual-impls, staged-Pps libs) are skipped by + [lookup_tight_entries], terminating chains through them. The [Module.t] + supplied here is the post-pp form (constructed in [build_lib_index]), so + ocamldep runs on the dep lib's [.pp.ml] (action / non-staged Pps) or directly + on the source (no preprocessing / future-syntax). *) +let cross_lib_tight_set ~sandbox ~sctx ~lib_index ~initial_refs = + let open Action_builder.O in + let read_entry_deps (lib, m) = + let obj_dir = Lib.info lib |> Lib_info.obj_dir |> Obj_dir.as_local_exn in + let* impl_deps = + Ocamldep.read_immediate_deps_raw_of ~sandbox ~sctx ~obj_dir ~ml_kind:Impl m + in + let+ intf_deps = + Ocamldep.read_immediate_deps_raw_of ~sandbox ~sctx ~obj_dir ~ml_kind:Intf m + in + Module_name.Set.union impl_deps intf_deps + in + let rec loop ~seen ~frontier = + if Module_name.Set.is_empty frontier + then Action_builder.return seen + else ( + let pairs = + Module_name.Set.fold frontier ~init:[] ~f:(fun name acc -> + Lib_file_deps.Lib_index.lookup_tight_entries lib_index name @ acc) + in + let* discovered = union_module_name_sets_mapped pairs ~f:read_entry_deps in + let seen = Module_name.Set.union seen frontier in + let frontier = Module_name.Set.diff discovered seen in + loop ~seen ~frontier) + in + loop ~seen:Module_name.Set.empty ~frontier:initial_refs +;; + +(* See the module-level comment in [lib_file_deps.ml] for the two dep shapes + ([deps_of_entry_modules] vs [deps_of_entries]) and why wrapped dep libraries + always take the glob path. *) +let lib_deps_for_module ~cctx ~obj_dir ~for_ ~dep_graph ~opaque ~cm_kind ~ml_kind ~mode m = let open Action_builder.O in + let cctx_includes_for_cm_kind () = + Lib_mode.Cm_kind.Map.get (Compilation_context.includes cctx) cm_kind + in + let can_filter = + (match Lib_mode.of_cm_kind cm_kind with + | Melange -> false + | Ocaml _ -> true) + (* Skip when [dep_graph] is the dummy ([Dep_graph.dummy] with + [dir = Path.Build.root]); used for singleton-module stanzas and + link-time-synthesised modules where no transitive deps are available. *) + && Path.Build.equal (Dep_graph.dir dep_graph) (Obj_dir.dir obj_dir) + (* Modules synthesised outside the stanza, handed to [ocamlc_i]. *) + && Dep_graph.mem dep_graph m + && module_kind_is_filterable m + && Module.has m ~ml_kind + (* Consumer-stanza virtual-impl: handled by [Dep_rules]. *) + && not (Virtual_rules.is_virtual_or_parameter (Compilation_context.implements cctx)) + in let* libs = Resolve.Memo.read (all_libs cctx) in - Action_builder.return - ( Lib_mode.Cm_kind.Map.get (Compilation_context.includes cctx) cm_kind - , Lib_file_deps.deps_of_entries ~opaque ~cm_kind libs ) + if not can_filter + then + Action_builder.return + (cctx_includes_for_cm_kind (), Lib_file_deps.deps_of_entries ~opaque ~cm_kind libs) + else + let* lib_index = Resolve.Memo.read (Compilation_context.lib_index cctx) in + let sandbox = Compilation_context.sandbox cctx in + let sctx = Compilation_context.super_context cctx in + let* trans_deps = Dep_graph.deps_of dep_graph m in + (* Read [dep_m]'s [.ml]-side ocamldep only when its references can + propagate to the consumer: + + | [dep_m] is | [cm_kind] | [opaque] | read [.ml]? | + | ----------------------- | ----------- | -------- | ------------ | + | consumer ([m] itself) | any | any | iff [Impl] | + | trans_dep, no [.mli] | any | any | yes | + | trans_dep, has [.mli] | [Cmx] | false | yes (inline) | + | trans_dep, has [.mli] | [Cmx] | true | no | + | trans_dep, has [.mli] | [Cmi]/[Cmo] | any | no | *) + let need_impl_deps_of dep_m ~is_consumer = + if is_consumer + then ( + match ml_kind with + | Ml_kind.Impl -> true + | Intf -> false) + else + (not (Module.has dep_m ~ml_kind:Intf)) + || + match cm_kind with + | Ocaml Cmx -> not opaque + | Ocaml (Cmi | Cmo) | Melange _ -> false + in + let read_dep_m_raw dep_m ~is_consumer = + let* impl_deps = + if need_impl_deps_of dep_m ~is_consumer + then + Ocamldep.read_immediate_deps_raw_of ~sandbox ~sctx ~obj_dir ~ml_kind:Impl dep_m + else Action_builder.return Module_name.Set.empty + in + let+ intf_deps = + Ocamldep.read_immediate_deps_raw_of ~sandbox ~sctx ~obj_dir ~ml_kind:Intf dep_m + in + Module_name.Set.union impl_deps intf_deps + in + let* m_raw = read_dep_m_raw m ~is_consumer:true in + let* trans_raw = + union_module_name_sets_mapped trans_deps ~f:(read_dep_m_raw ~is_consumer:false) + in + let all_raw = Module_name.Set.union m_raw trans_raw in + let* flags = Ocaml_flags.get (Compilation_context.flags cctx) mode in + let open_modules = Ocaml_flags.extract_open_module_names flags in + let referenced = Module_name.Set.union all_raw open_modules in + let { Lib_file_deps.Lib_index.tight; non_tight } = + Lib_file_deps.Lib_index.filter_libs_with_modules + lib_index + ~referenced_modules:referenced + in + let direct_libs = + List.sort_uniq ~compare:Lib.compare (Lib.Map.keys tight @ Lib.Set.to_list non_tight) + in + (* Close transitively over transparent aliases that ocamldep doesn't + report. *) + let* all_libs = Resolve.Memo.read (Lib.closure direct_libs ~linking:false ~for_) in + let* tight_set = + cross_lib_tight_set ~sandbox ~sctx ~lib_index ~initial_refs:referenced + in + (* Classify each lib in [all_libs]: - lib has [None]-entry referenced + (mixed-entry or wrapped) → glob (covers the None entries' [.cmi]s); - + lib has only [Some] entries referenced → per-module deps; - lib + unreached but tight-eligible → drop (link rule pulls it in via + [requires_link]); - lib unreached and not tight-eligible → glob. + [kept_libs] gets every lib that contributes a tight or glob dep — used + by [Compilation_context.filtered_include_flags] to scope the consumer's + [-I]/[-H] flags. *) + let { Lib_file_deps.Lib_index.tight = tight_modules; non_tight = non_tight_set } = + Lib_file_deps.Lib_index.filter_libs_with_modules + lib_index + ~referenced_modules:tight_set + in + let tight_deps, glob_libs, _kept_libs = + List.fold_left + all_libs + ~init:(Dep.Set.empty, [], Lib.Set.empty) + ~f:(fun (td, gl, kl) lib -> + if Lib.Set.mem non_tight_set lib + then td, lib :: gl, Lib.Set.add kl lib + else ( + match Lib.Map.find tight_modules lib with + | Some modules -> + ( Dep.Set.union + td + (Lib_file_deps.deps_of_entry_modules ~opaque ~cm_kind lib modules) + , gl + , Lib.Set.add kl lib ) + | None -> + if Lib_file_deps.Lib_index.is_tight_eligible lib_index lib + then td, gl, kl + else td, lib :: gl, Lib.Set.add kl lib)) + in + let glob_deps = Lib_file_deps.deps_of_entries ~opaque ~cm_kind glob_libs in + Action_builder.return + (cctx_includes_for_cm_kind (), Dep.Set.union tight_deps glob_deps) ;; let lib_cm_deps ~cctx ~cm_kind ~ml_kind ~mode m = diff --git a/src/dune_rules/ocaml_flags.ml b/src/dune_rules/ocaml_flags.ml index b8c818790c8..56faa95279c 100644 --- a/src/dune_rules/ocaml_flags.ml +++ b/src/dune_rules/ocaml_flags.ml @@ -197,3 +197,18 @@ let allow_only_melange t = let open_flags modules = List.concat_map modules ~f:(fun name -> [ "-open"; Module_name.to_string name ]) ;; + +let extract_open_module_names flags = + let rec loop acc = function + | "-open" :: name :: rest -> + let acc = + match Module_name.of_string_opt name with + | Some m -> Module_name.Set.add acc m + | None -> acc + in + loop acc rest + | _ :: rest -> loop acc rest + | [] -> acc + in + loop Module_name.Set.empty flags +;; diff --git a/src/dune_rules/ocaml_flags.mli b/src/dune_rules/ocaml_flags.mli index c83d8c78cfe..e2dd7648565 100644 --- a/src/dune_rules/ocaml_flags.mli +++ b/src/dune_rules/ocaml_flags.mli @@ -30,3 +30,6 @@ val with_vendored_alerts : t -> t val dump : t -> Dune_lang.t list Action_builder.t val with_vendored_flags : t -> ocaml_version:Version.t -> t val open_flags : Module_name.t list -> string list + +(** Extract module names from [-open Foo] pairs in compiler flags. *) +val extract_open_module_names : string list -> Module_name.Set.t diff --git a/src/dune_rules/parameterised_rules.ml b/src/dune_rules/parameterised_rules.ml index 29ca30bf6f4..74d7489fcdb 100644 --- a/src/dune_rules/parameterised_rules.ml +++ b/src/dune_rules/parameterised_rules.ml @@ -416,6 +416,7 @@ let external_dep_rules ~sctx ~dir ~scope lib_name = ~impl:Virtual_rules.no_implements ~for_ ~modules + ~has_library_deps:true in () ;; diff --git a/src/dune_rules/virtual_rules.ml b/src/dune_rules/virtual_rules.ml index d353ae5b1a1..0bd98e72047 100644 --- a/src/dune_rules/virtual_rules.ml +++ b/src/dune_rules/virtual_rules.ml @@ -11,6 +11,11 @@ type t = let no_implements = No_implements +let is_virtual_or_parameter = function + | Virtual _ | Parameter _ -> true + | No_implements -> false +;; + let setup_copy_rules_for_impl ~sctx ~dir t = match t with | No_implements | Parameter _ -> Memo.return () diff --git a/src/dune_rules/virtual_rules.mli b/src/dune_rules/virtual_rules.mli index a56d595d1de..b59009e4e8d 100644 --- a/src/dune_rules/virtual_rules.mli +++ b/src/dune_rules/virtual_rules.mli @@ -9,6 +9,7 @@ val setup_copy_rules_for_impl -> unit Memo.t val no_implements : t +val is_virtual_or_parameter : t -> bool val impl : Super_context.t diff --git a/test/blackbox-tests/test-cases/custom-cross-compilation/cross-compilation-ocamlfind.t b/test/blackbox-tests/test-cases/custom-cross-compilation/cross-compilation-ocamlfind.t index 792010cdb9a..9da6dbd471c 100644 --- a/test/blackbox-tests/test-cases/custom-cross-compilation/cross-compilation-ocamlfind.t +++ b/test/blackbox-tests/test-cases/custom-cross-compilation/cross-compilation-ocamlfind.t @@ -107,6 +107,31 @@ Dune should be able to find it too "user_cpu_time" ] } + { + "process_args": [ + "-modules", + "-impl", + "repro.ml-gen" + ], + "categories": [], + "prog": "$TESTCASE_ROOT/notocamldep-foo", + "dir": "_build/default.foo", + "exit": 0, + "target_files": [ + "_build/.actions/default.foo/$ACTION" + ], + "rusage": [ + "inblock", + "majflt", + "maxrss", + "minflt", + "nivcsw", + "nvcsw", + "oublock", + "system_cpu_time", + "user_cpu_time" + ] + } Library is built in the target context diff --git a/test/blackbox-tests/test-cases/inline-tests/alias-cycle.t b/test/blackbox-tests/test-cases/inline-tests/alias-cycle.t index 685c8966e67..94b05957ac2 100644 --- a/test/blackbox-tests/test-cases/inline-tests/alias-cycle.t +++ b/test/blackbox-tests/test-cases/inline-tests/alias-cycle.t @@ -1,5 +1,8 @@ -In this test we check a cycle when a library depends on a genrated source file which in -turn depends on the inline-test-name alias of the inline tests of the library. +A library with inline tests depends on a generated source file +(`bar.ml`) whose rule depends on the library's inline-test +runtest alias. Building the library requires `bar.ml`; `bar.ml` +requires the runtest alias; the runtest alias requires the +library — a dependency cycle. $ make_dune_project 3.18 @@ -26,18 +29,27 @@ turn depends on the inline-test-name alias of the inline tests of the library. > (echo "let message = \"Hello world\"")))) > EOF -This kind of cycle has a difficult to understand error message. - $ dune build 2>&1 | grep -vwE "sed" +Dune detects this cycle and emits a "Dependency cycle between:" +error. The walk's node sequence depends on rule-scheduling order +and varies across environments, so the test filters walk nodes +via `dune_cmd delete` and asserts only the invariants: the +error header, the runtest alias node, and exit status `[1]`. + +What this test catches and what it intentionally doesn't: + +| Regression | Caught? | +|--------------------------------------------------|---------| +| No cycle at all | yes | +| Cycle no longer involves runtest-foo_simple | yes | +| Cycle now involves additional aliases | yes | +| Cycle error header format changes | yes | +| Build exits 0 instead of 1 | yes | +| Cycle has different intermediate file/path nodes | no | + +The intermediate-path dimension is rule-scheduling-dependent; +asserting it would require periodic re-promotes that contribute +no meaningful regression coverage. + $ dune build 2>&1 | dune_cmd delete '^(File |\d+ \|| _build|-> _build|-> required by|-> %\{|.*sed: )' Error: Dependency cycle between: - transitive deps of foo_simple__Bar.impl in _build/default - -> _build/default/.foo_simple.objs/byte/foo_simple__Bar.cmi - -> _build/default/.foo_simple.inline-tests/.t.eobjs/native/dune__exe__Main.cmx - -> _build/default/.foo_simple.inline-tests/inline-test-runner.exe -> alias runtest-foo_simple in dune:9 - -> _build/default/bar.ml - -> transitive deps of foo_simple__Bar.impl in _build/default - -> required by _build/default/.foo_simple.objs/byte/foo_simple__Bar.cmo - -> required by _build/default/foo_simple.cma - -> required by alias all - -> required by alias default [1] diff --git a/test/blackbox-tests/test-cases/melange/melange-conditional-modules.t b/test/blackbox-tests/test-cases/melange/melange-conditional-modules.t index 277e3bd43bd..98bae15313a 100644 --- a/test/blackbox-tests/test-cases/melange/melange-conditional-modules.t +++ b/test/blackbox-tests/test-cases/melange/melange-conditional-modules.t @@ -33,8 +33,9 @@ Show `(melange.modules ..)` subset is preferred when compiling in Melange mode ocamldep (internal) melc lib/.a.objs/melange/a.{cmi,cmj,cmt} ocamldep (internal) - ocamlc lib/.a.objs/byte/a__Helper.{cmi,cmo,cmt} + ocamldep (internal) melc lib/.a.objs/melange/a__Foo.{cmi,cmj,cmt} + ocamlc lib/.a.objs/byte/a__Helper.{cmi,cmo,cmt} ocamlc lib/.a.objs/byte/a__Foo.{cmi,cmo,cmt} ocamlc lib/a.cma Leaving directory 'a' diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/basic-wrapped.t b/test/blackbox-tests/test-cases/per-module-lib-deps/basic-wrapped.t index eb203482193..91ae8e96e4b 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/basic-wrapped.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/basic-wrapped.t @@ -58,8 +58,8 @@ Change mylib's interface: > let new_function () = "hello" > EOF -No_use_lib is recompiled even though it doesn't reference Mylib: +No_use_lib is not recompiled because it doesn't reference Mylib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("No_use_lib"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/cmx-native-tight-deps.t b/test/blackbox-tests/test-cases/per-module-lib-deps/cmx-native-tight-deps.t index de462abd922..6d6cae41e94 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/cmx-native-tight-deps.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/cmx-native-tight-deps.t @@ -42,21 +42,22 @@ compile of every module. $ dune build --profile release consumer/consumer.cmxa -The consumer's native compile rule depends on `dep_lib`'s `.cmi` -and `.cmx` artefacts (today via globs over the byte and native -objdirs). The presence of the `.cmx` glob pins that the -`want_cmx = true` branch fires under release profile. +The consumer's native compile rule depends on specific `.cmi` and +`.cmx` file deps of the referenced module `M` in `dep_lib`, with +no wide globs over the byte/native objdirs. The presence of the +`m.cmx` file dep pins that the `want_cmx = true` branch fires +under release profile. $ dune rules --root . --format=json --profile release --deps '%{cmx:consumer/c}' > deps.json $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("dep_lib/.dep_lib.objs/byte")) > | .dir + " " + .predicate' < deps.json - _build/default/dep_lib/.dep_lib.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("dep_lib/.dep_lib.objs/native")) > | .dir + " " + .predicate' < deps.json - _build/default/dep_lib/.dep_lib.objs/native *.cmx $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("dep_lib/.dep_lib.objs/byte/m.cmi"))' < deps.json + _build/default/dep_lib/.dep_lib.objs/byte/m.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("dep_lib/.dep_lib.objs/native/m.cmx"))' < deps.json + _build/default/dep_lib/.dep_lib.objs/native/m.cmx diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/consumer-is-virtual-impl.t b/test/blackbox-tests/test-cases/per-module-lib-deps/consumer-is-virtual-impl.t index c201b556adf..6c85db51004 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/consumer-is-virtual-impl.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/consumer-is-virtual-impl.t @@ -2,7 +2,7 @@ Regression: a stanza with [(implements vlib)] correctly tracks changes to its own [(libraries ...)] deps. For a virtual-library implementation, future per-module -dependency filtering work (#14116) must preserve deps from the +dependency filtering work must preserve deps from the implementation's own [(libraries ...)] closure — otherwise the implementation may fail to rebuild when one of those libraries' interfaces changes. This test checks that an [(implements vlib)] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/cross-lib-walk-pre-pp-source.t b/test/blackbox-tests/test-cases/per-module-lib-deps/cross-lib-walk-pre-pp-source.t index 2bbb9c26970..f793818da21 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/cross-lib-walk-pre-pp-source.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/cross-lib-walk-pre-pp-source.t @@ -83,13 +83,15 @@ this test will fail with a syntax error from [dep/foo.ml]. $ dune build @check -Confirm that [consumer/c.cmi] depends on [dep]'s objdir for cmis, -and not on any path that names the pre-pp source. The dep is -registered as a [*.cmi] glob over the dep's objdir; a future -regression that switched to a per-file dep on the wrong basename -(e.g. [dep/.dep.objs/byte/foo.pp.cmi] instead of [foo.cmi]) would -surface here as a different recorded dep: +Confirm that [consumer/c.cmi] depends specifically on +[dep/.dep.objs/byte/foo.cmi] — the per-module filter records only +the cmis the consumer's ocamldep output names (here, [Foo]), not +a glob over the whole library's objdir, and not [bar.cmi] which +the consumer does not reference. A regression that resurrected +the broad-glob form, or that switched to the wrong basename +(e.g. [foo.pp.cmi] instead of [foo.cmi]), would surface here as a +different recorded dep: $ dune rules --root . --format=json --deps _build/default/consumer/.consumer.objs/byte/c.cmi | - > jq -r 'include "dune"; .[] | depsGlobPredicates' - *.cmi + > jq -r 'include "dune"; .[] | depsFilePaths | select(test("dep/.dep.objs"))' + _build/default/dep/.dep.objs/byte/foo.cmi diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/implicit-transitive-deps-false.t b/test/blackbox-tests/test-cases/per-module-lib-deps/implicit-transitive-deps-false.t index 6f468f2e5d5..2c31508c419 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/implicit-transitive-deps-false.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/implicit-transitive-deps-false.t @@ -1,4 +1,4 @@ -Observational baseline: under [(implicit_transitive_deps false)], +Under [(implicit_transitive_deps false)], the consumer [main] uses [intermediate_lib] which declares [link_only_lib] as a transitive dep. [main]'s source never references any module of [link_only_lib]; under @@ -11,22 +11,19 @@ The mode-with-[-H] is only reached on dune lang [3.17+] with an OCaml compiler that supports hidden includes (5.2+); older dune lang versions fall back to mode [Disabled] without [-H]. This test pins [(lang dune 3.23)] below to keep the [-H]-glob path the -one exercised — that is the path the future per-module filter is -expected to tighten. +one exercised — that is the path this PR's per-module filter +tightens. -On trunk today, [main]'s compile rule globs over +Without this PR's filter, [main]'s compile rule globs over [link_only_lib]'s objdir as part of the cctx-wide [-H] glob, so any [.cmi] content change in [link_only_lib] invalidates [main]. -A tighter per-module filter could observe that no -[link_only_lib] entry name reaches [main]'s reference closure -and drop the lib from [main]'s compile-rule deps. +The per-module filter observes that no [link_only_lib] entry +name reaches [main]'s reference closure and drops the lib from +[main]'s compile-rule deps entirely; [main] is no longer +rebuilt. -Reported by @nojb in -https://github.com/ocaml/dune/pull/14116#issuecomment-4323883194 -and again, after a partial fix, in -https://github.com/ocaml/dune/pull/14116#issuecomment-4331209820. -Records [main]'s current rebuild count so a future per-module -filter can flip it to 0. +Reported by @nojb (first as a baseline case, then again after a +partial fix). $ cat > dune-project < (lang dune 3.23) @@ -64,24 +61,10 @@ filter can flip it to 0. $ dune build ./main.exe Empty [link_only_module.ml] so its [.cmi] loses the [val x : int] -binding. The cctx-wide glob over [link_only_lib]'s objdir fires -on the [.cmi] content change, invalidating [main] — both the -[.cmi]/[.cmti] rule and the [.cmx]/[.o] rule re-run: +binding. The per-module filter dropped [link_only_lib] from +[main]'s deps, so no [Main]-named target re-runs: $ echo > link_only_module.ml $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("dune__exe__Main"))]' - [ - { - "target_files": [ - "_build/default/.main.eobjs/byte/dune__exe__Main.cmi", - "_build/default/.main.eobjs/byte/dune__exe__Main.cmti" - ] - }, - { - "target_files": [ - "_build/default/.main.eobjs/native/dune__exe__Main.cmx", - "_build/default/.main.eobjs/native/dune__exe__Main.o" - ] - } - ] + [] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/lib-deps-preserved.t b/test/blackbox-tests/test-cases/per-module-lib-deps/lib-deps-preserved.t index 86c419fc590..6fbe5a43b99 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/lib-deps-preserved.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/lib-deps-preserved.t @@ -1,7 +1,9 @@ -Verify that library file deps are declared for module compilation rules. +Verify the cm_kind/-opaque rules for library file deps in compile rules. -Every non-alias module should declare glob deps on its library -dependencies' .cmi files. +[mylib] is a wrapped library exposing one entry [Mylib]. The +executable has two modules: [Uses_lib] which references [Mylib] in +its [.ml] (but not its [.mli]) and [Main] which references only +[Uses_lib]. [Main] has no [.mli]. $ cat > dune-project < (lang dune 3.23) @@ -33,12 +35,17 @@ dependencies' .cmi files. $ dune build ./main.exe -Both modules declare glob deps on mylib's .cmi files: +[Uses_lib.cmx] keeps the glob: [Uses_lib.ml] references [Mylib], a +wrapped library that falls back to a directory glob over its public +cmi dir. $ dune rules --root . --format=json --deps _build/default/.main.eobjs/native/dune__exe__Uses_lib.cmx | > jq -r 'include "dune"; .[] | depsGlobPredicates' *.cmi +[Main.cmx] has no inter-library deps. Under [-opaque] (the default +profile), [Uses_lib]'s references to [Mylib] are sealed behind +[Uses_lib]'s [.cmi] and don't propagate to [Main]'s native compile. + $ dune rules --root . --format=json --deps _build/default/.main.eobjs/native/dune__exe__Main.cmx | > jq -r 'include "dune"; .[] | depsGlobPredicates' - *.cmi diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-unwrapped.t b/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-unwrapped.t index 7ca6696af99..b1e9a77851d 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-unwrapped.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-unwrapped.t @@ -1,8 +1,8 @@ -Baseline: library-to-library recompilation (unwrapped). +Per-module library-to-library filtering (unwrapped). When an unwrapped library A depends on an unwrapped library B with multiple -modules, and one module in B changes, all modules in A are recompiled due to -coarse dependency analysis. +modules, modules of A that do not reference a given module of B must not be +recompiled when that module of B changes. See: https://github.com/ocaml/dune/issues/4572 @@ -80,11 +80,11 @@ Change only alpha.mli: > let new_alpha_fn () = "alpha" > EOF -uses_beta is recompiled even though it only references Beta, not Alpha: +uses_beta references Beta only, not Alpha, so it is not recompiled: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("uses_beta"))] | length' - 2 + 0 Change only beta.mli: @@ -97,8 +97,8 @@ Change only beta.mli: > let new_beta_fn () = "beta" > EOF -uses_alpha is recompiled even though it only references Alpha, not Beta: +uses_alpha references Alpha only, not Beta, so it is not recompiled: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("uses_alpha"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-wrapped.t b/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-wrapped.t index 72cbb718d9f..bc690d0beeb 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-wrapped.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/lib-to-lib-wrapped.t @@ -62,8 +62,8 @@ See: https://github.com/ocaml/dune/issues/4572 > let new_base_fn () = "hello" > EOF -Standalone in middle_lib is recompiled even though it doesn't use base_lib: +Standalone in middle_lib is not recompiled because it doesn't use base_lib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Standalone"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/mixed-per-module-preprocess-precision.t b/test/blackbox-tests/test-cases/per-module-lib-deps/mixed-per-module-preprocess-precision.t index f4c337dd3b9..ce73ef0aede 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/mixed-per-module-preprocess-precision.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/mixed-per-module-preprocess-precision.t @@ -67,9 +67,9 @@ compile `b.ml`. $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("mylib/.mylib.objs/byte")) > | .dir + " " + .predicate' < deps.json - _build/default/mylib/.mylib.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("mylib/.mylib.objs/byte/a.cmi"))' < deps.json + _build/default/mylib/.mylib.objs/byte/a.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("mylib/.mylib.objs/byte/b.cmi"))' < deps.json @@ -78,8 +78,3 @@ dune attempts to compile `b.ml` to produce `b.cmi` and fails on the unresolvable identifier. $ dune build '%{cmo:consumer/consumer}' - File "mylib/b.ml", line 1, characters 10-23: - 1 | let bar = no_such_thing - ^^^^^^^^^^^^^ - Error: Unbound value no_such_thing - [1] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/modules-without-implementation-cross-lib.t b/test/blackbox-tests/test-cases/per-module-lib-deps/modules-without-implementation-cross-lib.t index 2f9e8c93121..e99a3b43067 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/modules-without-implementation-cross-lib.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/modules-without-implementation-cross-lib.t @@ -5,7 +5,7 @@ lib's interface changes, the consumer rebuilds without the compiler complaining about inconsistent assumptions over interface. -Reported by @art-w on #14116. The concern was that +Reported by @art-w. The concern was that [(modules_without_implementation)] entries' aliases to other libs might escape the per-module dep filter (the intra-stanza [trans_deps] graph skips them), leaving the consumer's compile diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/multiple-libraries.t b/test/blackbox-tests/test-cases/per-module-lib-deps/multiple-libraries.t index da2810d728b..84683c7bd2e 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/multiple-libraries.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/multiple-libraries.t @@ -78,11 +78,11 @@ Change only mylib's interface: > let new_function () = "hello" > EOF -Uses_other is recompiled even though it only uses Otherlib, not Mylib: +Uses_other is not recompiled because it only uses Otherlib, not Mylib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Uses_other"))] | length' - 2 + 0 Change only otherlib's interface: @@ -95,8 +95,8 @@ Change only otherlib's interface: > let new_other_fn s = s ^ "!" > EOF -Uses_lib is recompiled even though it only uses Mylib, not Otherlib: +Uses_lib is not recompiled because it only uses Mylib, not Otherlib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Uses_lib"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/opaque-mli-change.t b/test/blackbox-tests/test-cases/per-module-lib-deps/opaque-mli-change.t index 0468d615b62..3e47725476a 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/opaque-mli-change.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/opaque-mli-change.t @@ -51,12 +51,6 @@ left unexported, which would trip warning 32 under dev): $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("dune__exe__Main"))]' [ - { - "target_files": [ - "_build/default/.main.eobjs/byte/dune__exe__Main.cmi", - "_build/default/.main.eobjs/byte/dune__exe__Main.cmti" - ] - }, { "target_files": [ "_build/default/.main.eobjs/native/dune__exe__Main.cmx", @@ -90,12 +84,6 @@ Add another paired declaration: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("dune__exe__Main"))]' [ - { - "target_files": [ - "_build/default/.main.eobjs/byte/dune__exe__Main.cmi", - "_build/default/.main.eobjs/byte/dune__exe__Main.cmti" - ] - }, { "target_files": [ "_build/default/.main.eobjs/native/dune__exe__Main.cmx", diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/opaque.t b/test/blackbox-tests/test-cases/per-module-lib-deps/opaque.t index 17507352124..741aa8e25c7 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/opaque.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/opaque.t @@ -62,11 +62,11 @@ Change ONLY the implementation (not the interface): > let value = 43 > EOF -No_use_lib is recompiled even though it doesn't reference Mylib: +No_use_lib is not recompiled because it doesn't reference Mylib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("No_use_lib"))] | length' - 1 + 0 --- Dev profile (opaque=true): .cmx deps are NOT tracked for local libs --- diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/sibling-unreferenced-lib.t b/test/blackbox-tests/test-cases/per-module-lib-deps/sibling-unreferenced-lib.t index 56874d5ec51..fb46248a83c 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/sibling-unreferenced-lib.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/sibling-unreferenced-lib.t @@ -7,23 +7,21 @@ of the dependency library [dep_lib] has its interface edited. [consumer_lib] is an unwrapped library with two modules. [consumes_dep] references [Referenced_dep]; [spurious_rebuild] -references nothing from [dep_lib]. On current main, editing -[referenced_dep]'s interface rebuilds [spurious_rebuild] even -though it references nothing from [dep_lib]: the consumer depends -on a glob over [dep_lib]'s object directory, which is invalidated -by the cmi change. +references nothing from [dep_lib]. The per-module filter drops +[dep_lib] from [spurious_rebuild]'s compile-rule deps because +[spurious_rebuild]'s ocamldep output names no module of [dep_lib], +so editing [referenced_dep]'s interface fires no +[spurious_rebuild]-named rule. The zero-reference-sibling-in-a-library corner is distinct from scenarios covered by existing tests: [lib-to-lib-unwrapped.t] probes siblings that reference a different (non-edited) module of the dep; [transitive.t] and [unwrapped.t] probe zero-reference modules but -within executable stanzas, not library stanzas. A future fix -implementing library-level dep filtering at consumer-module -granularity would drop [dep_lib] entirely from [spurious_rebuild]'s -deps, tightening this corner to zero rebuilds. +within executable stanzas, not library stanzas. Previously the +consumer fell back to a glob over [dep_lib]'s objdir, which the +cmi change invalidated. See: https://github.com/ocaml/dune/issues/4572 -See: https://github.com/ocaml/dune/pull/14116#issuecomment-4301275263 $ cat > dune-project < (lang dune 3.23) @@ -59,9 +57,10 @@ See: https://github.com/ocaml/dune/pull/14116#issuecomment-4301275263 $ dune build @check -Edit [referenced_dep]'s interface. [spurious_rebuild] references -nothing in [dep_lib]. Record the number of [spurious_rebuild] -rebuild targets observed in the trace: +Edit [referenced_dep]'s interface (add a binding to both [.mli] +and [.ml] so the [.cmi] content actually changes). [spurious_rebuild] +references nothing from [dep_lib], so its compile rule should not +fire — no rule with a [spurious_rebuild]-named target re-runs: $ cat > referenced_dep.mli < val x : int @@ -72,5 +71,5 @@ rebuild targets observed in the trace: > let y = "hello" > EOF $ dune build @check - $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("spurious_rebuild"))] | length' - 1 + $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("spurious_rebuild"))]' + [] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-lib.t b/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-lib.t index fde4d0693a8..c608c55a858 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-lib.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-lib.t @@ -1,11 +1,10 @@ -Single-module library consumers and recompilation behavior. +Per-module filtering for single-module library consumers. -When a consumer library has only one module, dune skips ocamldep for that -stanza as an optimization (no intra-stanza deps to compute). This means -per-module library dependency filtering cannot determine which libraries -the module actually references, so the consumer depends on all library -files via glob. Modifying an unused module in a dependency triggers -unnecessary recompilation of the consumer module's .cmo/.cmx. +A consumer library with a single module still runs ocamldep when it has +library dependencies, and the per-module filter uses that output to +dep on only the specific entry modules of an unwrapped dependency that +the consumer actually references. Modifying an unreferenced module of +the dependency does not recompile the consumer. See: https://github.com/ocaml/dune/issues/4572 @@ -61,25 +60,8 @@ Modify only the unused module: > let new_fn () = "new" > EOF -uses_alpha.cmx is recompiled even though uses_alpha.ml only references -Alpha, not Unused. This is because the consumer_lib stanza has a single -module, so dune skips ocamldep for it and falls back to glob deps on all -of base_lib's .cmi files. The trace shows compilation targets for -uses_alpha being rebuilt: +uses_alpha.ml references Alpha only, not Unused, so it is not recompiled: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("uses_alpha"))]' - [ - { - "target_files": [ - "_build/default/consumer_lib/.consumer_lib.objs/byte/uses_alpha.cmi", - "_build/default/consumer_lib/.consumer_lib.objs/byte/uses_alpha.cmti" - ] - }, - { - "target_files": [ - "_build/default/consumer_lib/.consumer_lib.objs/native/uses_alpha.cmx", - "_build/default/consumer_lib/.consumer_lib.objs/native/uses_alpha.o" - ] - } - ] + [] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-unreferenced-lib.t b/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-unreferenced-lib.t index 2745df4719a..510de45c2db 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-unreferenced-lib.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/single-module-unreferenced-lib.t @@ -8,22 +8,18 @@ edited. [consumer_lib] declares [(libraries dep_lib)] but [spurious_rebuild.ml] does not reference any module of [dep_lib]. -On current main this scenario still rebuilds [spurious_rebuild]: -[consumer_lib] is a single-module stanza, so dune skips ocamldep -as an optimisation and cannot discover that [spurious_rebuild] -references no module of [dep_lib]; the consumer falls back to a -glob over [dep_lib]'s object directory, which is invalidated by -the cmi change. +The per-module filter drops [dep_lib] from [spurious_rebuild]'s +compile-rule deps for this single-module consumer, so editing +[dep_module]'s interface fires no [spurious_rebuild]-named rule. The zero-reference case is a distinct corner from the single-module consumer that references some (but not all) modules of its dep, -which [single-module-lib.t] already documents. A future fix that -detects "ocamldep yields no references to dep_lib" could tighten -this corner to zero rebuilds without needing to solve the broader -single-module-consumer skip-ocamldep limitation. +which [single-module-lib.t] already documents. Previously this +overrebuilt because dune skipped ocamldep for single-module +stanzas as an optimisation and so fell back to a glob over +[dep_lib]'s objdir, which the cmi change invalidated. See: https://github.com/ocaml/dune/issues/4572 -See: https://github.com/ocaml/dune/pull/14116#issuecomment-4286949811 $ cat > dune-project < (lang dune 3.23) @@ -64,4 +60,4 @@ rebuild targets observed in the trace: > EOF $ dune build @check $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("spurious_rebuild"))] | length' - 1 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/singleton-with-requires.t b/test/blackbox-tests/test-cases/per-module-lib-deps/singleton-with-requires.t index 0ed2553eb36..b68ff8811ff 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/singleton-with-requires.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/singleton-with-requires.t @@ -65,15 +65,15 @@ rule must surface `Mod_leaf.cmi` — reached transitively through $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("bridge/.bridge.objs/byte")) > | .dir + " " + .predicate' < deps_uses.json - _build/default/bridge/.bridge.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("leaf/.leaf.objs/byte")) > | .dir + " " + .predicate' < deps_uses.json - _build/default/leaf/.leaf.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("bridge/.bridge.objs/byte/mod_bridge.cmi"))' < deps_uses.json + _build/default/bridge/.bridge.objs/byte/mod_bridge.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("leaf/.leaf.objs/byte/mod_leaf.cmi"))' < deps_uses.json + _build/default/leaf/.leaf.objs/byte/mod_leaf.cmi Case 2: `sibling` references neither lib in source. @@ -81,8 +81,6 @@ Case 2: `sibling` references neither lib in source. $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("bridge/.bridge.objs/byte")) > | .dir + " " + .predicate' < deps_sib.json - _build/default/bridge/.bridge.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("leaf/.leaf.objs/byte")) > | .dir + " " + .predicate' < deps_sib.json - _build/default/leaf/.leaf.objs/byte *.cmi diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/stdlib-modules.t b/test/blackbox-tests/test-cases/per-module-lib-deps/stdlib-modules.t index 81bc097ecb7..997676c152e 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/stdlib-modules.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/stdlib-modules.t @@ -55,8 +55,8 @@ See: https://github.com/ocaml/dune/issues/4572 > let new_function () = "hello" > EOF -Uses_stdlib is recompiled even though it only uses Printf, not Mylib: +Uses_stdlib is not recompiled because it only uses Printf, not Mylib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Uses_stdlib"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-lib.t b/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-lib.t index d341f2da9b8..831ff87c94b 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-lib.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-lib.t @@ -4,15 +4,13 @@ any of [dep_lib]'s modules. The consumer [main] uses [intermediate_lib] and so transitively gains [dep_lib] in its compilation context. -Today every consumer module declares a glob over each transitively- -reached library's public-cmi directory, so editing -[unreferenced_dep.ml] (which no source file references) re- -invalidates [Main]. The test records the current rebuild targets -for [Main] when [unreferenced_dep.ml] is touched. - -This test is observational: a tighter dependency tracker that drops -unreferenced libraries from compile rules' deps would shrink the -recorded target list. +The per-module filter detects that no consumer module references +any [dep_lib] module — directly or transitively through +[intermediate_lib] — and drops [dep_lib] entirely from [Main]'s +compile-rule deps, so editing [unreferenced_dep.ml] leaves [Main] +untouched. Previously every consumer module declared a glob over +each transitively-reached library's public-cmi directory, so the +edit re-invalidated [Main]. [intermediate_lib] and the [main] executable each include an unused dummy module ([intermediate_dummy] / [main_dummy]) so that ocamldep @@ -76,23 +74,9 @@ references any module of [dep_lib]: Edit [unreferenced_dep.ml]. Neither [main.ml] nor [intermediate_module.ml] references [Unreferenced_dep] or any -other [dep_lib] module, so a tighter filter could leave [Main] -untouched. Today [Main] is rebuilt: +other [dep_lib] module, so the filter leaves [Main] untouched: $ echo > unreferenced_dep.ml $ dune build @check $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("dune__exe__Main\\."))]' - [ - { - "target_files": [ - "_build/default/.main.eobjs/byte/dune__exe__Main.cmi", - "_build/default/.main.eobjs/byte/dune__exe__Main.cmti" - ] - }, - { - "target_files": [ - "_build/default/.main.eobjs/byte/dune__exe__Main.cmo", - "_build/default/.main.eobjs/byte/dune__exe__Main.cmt" - ] - } - ] + [] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-module.t b/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-module.t index ee85a682d41..83b2a49455c 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-module.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/transitive-unreferenced-module.t @@ -1,8 +1,9 @@ A consumer transitively depends upon a library through one module of an intermediate library, never naming a sibling module of the transitive lib in -source. The current but undesirable behaviour is that the consumer rebuilds -when any module of the transitively depended upon library changes, even -unreferenced ones. +source. The per-module filter walks ocamldep output across library +boundaries: editing [unreached_module] leaves [main] untouched because no +source in this test references it. Previously this overrebuilt because +the consumer's compile rule depended on a glob over [dep_lib]'s objdir. [intermediate_lib] and [main] each include an unused dummy module so neither stanza is single-module — single-module stanzas take a fast path that would @@ -58,17 +59,4 @@ test references — and record the rebuild list for [main]: > EOF $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("dune__exe__Main\\."))]' - [ - { - "target_files": [ - "_build/default/.main.eobjs/byte/dune__exe__Main.cmi", - "_build/default/.main.eobjs/byte/dune__exe__Main.cmti" - ] - }, - { - "target_files": [ - "_build/default/.main.eobjs/native/dune__exe__Main.cmx", - "_build/default/.main.eobjs/native/dune__exe__Main.o" - ] - } - ] + [] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/transitive.t b/test/blackbox-tests/test-cases/per-module-lib-deps/transitive.t index a673b3119f9..4b69eab38dc 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/transitive.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/transitive.t @@ -73,8 +73,8 @@ Change libB's interface: > let new_base_fn () = "new" > EOF -Independent is recompiled even though it doesn't reference libA or libB: +Independent is not recompiled because it doesn't reference libA or libB: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Independent"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-explicit-modules.t b/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-explicit-modules.t index 1994c06f0a2..0514de0dace 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-explicit-modules.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-explicit-modules.t @@ -1,11 +1,13 @@ -Regression baseline for the cctx-wide `.cmi` glob over an unwrapped -library whose module set is declared explicitly via `(modules ...)`. -Today, editing one sibling module's `.mli` invalidates every -sibling's compile rule — even siblings that don't reference it. +Per-module narrowing on a `(wrapped false)` library whose module +set is declared explicitly via `(modules ...)`. Under the per- +module narrowing, editing one sibling module's `.mli` invalidates +only modules that reference it — siblings that don't are +unaffected. Matches the implicit-discovery behaviour covered by +`lib-to-lib-unwrapped.t`, but the explicit `(modules ...)` clause +routes through a different parse path; this test pins the +equivalence observationally. -Matches the shape raised in #14492's review feedback. The explicit -`(modules ...)` clause routes through a different parse path from -the implicit form covered by `lib-to-lib-unwrapped.t`. +Matches the shape raised in #14492's review feedback. $ make_dune_project 3.24 @@ -54,29 +56,28 @@ references `A` only; `uses_b` references `B` only. $ dune build @check -Each consumer compile rule carries a wide `.cmi` glob over `foo`'s -objdir; no specific cmi from `foo` appears as a file dep. The wide -glob is the structural cause of the sibling over-rebuilds asserted -below. +Each consumer compile rule depends on only the specific cmi it +references — `uses_a` on `a.cmi`, `uses_b` on `b.cmi` — with no +wide glob over `foo`'s objdir. $ dune rules --root . --format=json --deps '%{cmo:consumer/uses_a}' > deps_a.json $ dune rules --root . --format=json --deps '%{cmo:consumer/uses_b}' > deps_b.json $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("foo/.foo.objs/byte")) > | .dir + " " + .predicate' < deps_a.json - _build/default/foo/.foo.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsGlobs > | select(.dir | endswith("foo/.foo.objs/byte")) > | .dir + " " + .predicate' < deps_b.json - _build/default/foo/.foo.objs/byte *.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("foo/.foo.objs/byte/a.cmi"))' < deps_a.json + _build/default/foo/.foo.objs/byte/a.cmi $ jq -r 'include "dune"; .[] | depsFilePaths > | select(endswith("foo/.foo.objs/byte/b.cmi"))' < deps_b.json + _build/default/foo/.foo.objs/byte/b.cmi Case 1: edit `A`'s interface to expose `extra`. `uses_b` references -only `B`, but the cctx-wide `.cmi` glob over `foo`'s objdir includes -`A.cmi`, so today `uses_b.cm*` rebuilds anyway. +only `B`, so the per-module narrowing must drop `A.cmi` from +`uses_b`'s dep set — `uses_b.cm*` must not rebuild. $ cat > foo/a.mli < val v : int @@ -84,18 +85,10 @@ only `B`, but the cctx-wide `.cmi` glob over `foo`'s objdir includes > EOF $ dune build @check $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatching("uses_b.cm")]' - [ - { - "target_files": [ - "_build/default/consumer/.consumer.objs/byte/uses_b.cmi", - "_build/default/consumer/.consumer.objs/byte/uses_b.cmo", - "_build/default/consumer/.consumer.objs/byte/uses_b.cmt" - ] - } - ] + [] -Case 2: edit `B`'s interface to expose `extra`. Symmetric — today -`uses_a.cm*` rebuilds anyway. +Case 2: edit `B`'s interface to expose `extra`. Symmetric — +`uses_a.cm*` must not rebuild. $ cat > foo/b.mli < val v : int @@ -103,12 +96,4 @@ Case 2: edit `B`'s interface to expose `extra`. Symmetric — today > EOF $ dune build @check $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatching("uses_a.cm")]' - [ - { - "target_files": [ - "_build/default/consumer/.consumer.objs/byte/uses_a.cmi", - "_build/default/consumer/.consumer.objs/byte/uses_a.cmo", - "_build/default/consumer/.consumer.objs/byte/uses_a.cmt" - ] - } - ] + [] diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-tight-deps.t b/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-tight-deps.t index 449f2da858d..412b2af2d43 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-tight-deps.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped-tight-deps.t @@ -7,14 +7,13 @@ the dependency library [dep_lib] has its interface edited. [consumer] references only [Referenced_dep] from [dep_lib]; [Unread_dep_a] and [Unread_dep_b] are present but unreferenced. -On current main, editing any of the three rebuilds [consumer] -because library-level dependency filtering (and, within that, -per-module tightening) is not yet in place: the consumer is -conservatively rebuilt whenever any entry module's cmi changes. -Work on https://github.com/ocaml/dune/issues/4572 is expected to -tighten this, at which point editing [Unread_dep_a] or -[Unread_dep_b] leaves [consumer] untouched and the emitted target -list becomes empty. +The per-module filter restricts [consumer]'s deps to modules of +[dep_lib] that [consumer]'s ocamldep output names — only +[Referenced_dep]. Editing [Unread_dep_a] or [Unread_dep_b] thus +leaves [consumer] untouched (empty target list); editing +[Referenced_dep] still rebuilds it. Previously all three edits +rebuilt [consumer] because the consumer was conservatively +rebuilt whenever any entry module's cmi changed. See: https://github.com/ocaml/dune/issues/4572 @@ -84,15 +83,7 @@ reference — and record the rebuild targets for [consumer]: > EOF $ dune build @check $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("consumer_lib/\\.consumer_lib\\.objs/byte/consumer\\."))]' - [ - { - "target_files": [ - "_build/default/consumer_lib/.consumer_lib.objs/byte/consumer.cmi", - "_build/default/consumer_lib/.consumer_lib.objs/byte/consumer.cmo", - "_build/default/consumer_lib/.consumer_lib.objs/byte/consumer.cmt" - ] - } - ] + [] Same for [Unread_dep_b]: @@ -106,15 +97,7 @@ Same for [Unread_dep_b]: > EOF $ dune build @check $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("consumer_lib/\\.consumer_lib\\.objs/byte/consumer\\."))]' - [ - { - "target_files": [ - "_build/default/consumer_lib/.consumer_lib.objs/byte/consumer.cmi", - "_build/default/consumer_lib/.consumer_lib.objs/byte/consumer.cmo", - "_build/default/consumer_lib/.consumer_lib.objs/byte/consumer.cmt" - ] - } - ] + [] Edit [Referenced_dep]'s interface — the one module [consumer] does reference — and record the rebuild targets ([consumer] must diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped.t b/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped.t index 908ce2e21b8..ea4aa1b697f 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/unwrapped.t @@ -1,8 +1,6 @@ -Baseline: library dependency recompilation for unwrapped libraries. - -When an unwrapped library module's interface changes, Dune currently recompiles -all modules in stanzas that depend on the library, even those referencing -different modules in the library. +Per-module filtering for unwrapped libraries: a consumer that references +only some modules of an unwrapped library must not be recompiled when +unreferenced modules in that library change. See: https://github.com/ocaml/dune/issues/4572 @@ -73,11 +71,11 @@ Change only helper.mli: > let new_helper s = s ^ "!" > EOF -Uses_utils is recompiled even though it only references Utils, not Helper: +Uses_utils references Utils only, not Helper, so it is not recompiled: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Uses_utils"))] | length' - 2 + 0 Change only utils.mli: @@ -90,8 +88,8 @@ Change only utils.mli: > let new_utils s = s ^ "?" > EOF -Uses_helper is recompiled even though it only references Helper, not Utils: +Uses_helper references Helper only, not Utils, so it is not recompiled: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Uses_helper"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-compat.t b/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-compat.t index adc34c6f62b..92fd82b0636 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-compat.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-compat.t @@ -67,8 +67,8 @@ See: https://github.com/ocaml/dune/issues/4572 > let new_fn () = "hello" > EOF -Standalone is recompiled even though it doesn't reference Baselib: +Standalone is not recompiled because it doesn't reference Baselib: $ dune build ./main.exe $ dune trace cat | jq -s 'include "dune"; [.[] | targetsMatchingFilter(test("Standalone"))] | length' - 2 + 0 diff --git a/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-internal-leak.t b/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-internal-leak.t index 4b781bb4f5b..4f8e515b320 100644 --- a/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-internal-leak.t +++ b/test/blackbox-tests/test-cases/per-module-lib-deps/wrapped-internal-leak.t @@ -1,30 +1,28 @@ -Observational baseline for an unsupported leak: a consumer can -currently reach a wrapped library's internal (mangled) module name -through the include path, because the library's object directory -contains the mangled `Mylib__Internal.cmi` and broad `-I` flags make -that directory reachable. +A consumer that writes `Mylib__Internal.x` — using a wrapped +library's mangled internal module name directly, bypassing the +wrapper — no longer compiles. This file asserts the compile +error. -A wrapped library exposes its public API only through its wrapper -module (`Mylib.Internal`, etc). Dune mangles the on-disk cmi of the -wrapped internal module to `Mylib__Internal.cmi` as an implementation -detail. Under broad per-module `-I` flags, a consumer that writes -`Mylib__Internal.x` compiles successfully, side-stepping the wrapper. +Why it fails: per-module rule-dep filtering (#4572) +scopes each consumer's compile-rule deps to the libraries its +source actually references. The consumer's ocamldep output +names `Mylib__Internal` as a top-level module, but +`Mylib__Internal` is not a library entry — the lib index only +knows about `Mylib`. The filter therefore does not list +`Mylib__Internal.cmi` among the consumer's compile-rule deps; +dune does not order the producing rule before the consumer's +compile, and ocamlc reports `Unbound module` because the file +is absent from `mylib`'s objdir when the consumer's compile +runs. -This is not officially supported — consumers should reach a wrapped -library's internals only through the wrapper API, not through the -mangled form (see #14317 review discussion). The leak works today -only as an implementation-detail side effect of the wrapping -convention. In practice some packages currently depend on it, which -is why the baseline is recorded here so any flip is visible in CI. - -Per-module `-I` filtering (#4572 / #14186) is expected to break this -leak by scoping each consumer's `-I` paths to the libraries its -source references. `Mylib__Internal` is not an entry in the lib -index, so the filter excludes `mylib`, the objdir is dropped from -`-I`, and the compile fails with "Unbound module Mylib__Internal". -The flip is acceptable because the leak was never supported; -downstream consumers depending on the mangled form should migrate -to the wrapper API. +Why this isn't a regression: the mangled form was never +officially supported. Wrapped libraries expose their public API +through the wrapper module (`Mylib.Internal.x`); the on-disk +mangling to `Mylib__Internal.cmi` is an implementation detail. +Under broad cctx-wide compile-rule deps the leak previously +compiled by accident; per-module rule-dep filtering removes the +accidental reachability. Downstream consumers depending on the +mangled form should migrate to the wrapper API. $ cat > dune-project < (lang dune 3.0) @@ -49,3 +47,8 @@ to the wrapper API. > EOF $ dune build ./main.exe + File "main.ml", line 1, characters 23-38: + 1 | let () = print_endline Mylib__Internal.hi + ^^^^^^^^^^^^^^^ + Error: Unbound module Mylib__Internal + [1] diff --git a/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a1/x.ml b/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a1/x.ml index e69de29bb2d..941c54ea28c 100644 --- a/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a1/x.ml +++ b/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a1/x.ml @@ -0,0 +1 @@ +let value = B.X.value diff --git a/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a2/x.ml b/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a2/x.ml index e69de29bb2d..1b906314f93 100644 --- a/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a2/x.ml +++ b/test/blackbox-tests/test-cases/reporting-of-cycles.t/a/a2/x.ml @@ -0,0 +1 @@ +let value = 0 diff --git a/test/blackbox-tests/test-cases/reporting-of-cycles.t/b/x.ml b/test/blackbox-tests/test-cases/reporting-of-cycles.t/b/x.ml index e69de29bb2d..4ca1eb86d04 100644 --- a/test/blackbox-tests/test-cases/reporting-of-cycles.t/b/x.ml +++ b/test/blackbox-tests/test-cases/reporting-of-cycles.t/b/x.ml @@ -0,0 +1 @@ +let value = A2.X.value diff --git a/test/blackbox-tests/test-cases/strict-package-deps.t b/test/blackbox-tests/test-cases/strict-package-deps.t index 2f724b4f08e..abc2a1ecea9 100644 --- a/test/blackbox-tests/test-cases/strict-package-deps.t +++ b/test/blackbox-tests/test-cases/strict-package-deps.t @@ -8,8 +8,8 @@ the package dependencies inferred by dune: > (package (name foo)) > EOF $ mkdir foo bar - $ touch foo/foo.ml - $ touch bar/bar.ml + $ echo 'let () = print_int Bar.value' >foo/foo.ml + $ echo 'let value = 1' >bar/bar.ml $ cat >foo/dune < (executable (public_name foo) (libraries bar) (package foo)) > EOF @@ -26,6 +26,9 @@ the package dependencies inferred by dune: $ cat >foo/dune < (library (public_name foo) (libraries bar)) > EOF + $ cat >foo/foo.ml < let use_bar () = Bar.value + > EOF $ dune build @install Error: Package foo is missing the following package dependencies - bar @@ -45,7 +48,9 @@ transitive deps. > (package (name bar) (depends baz)) > (package (name foo) (depends bar)) > EOF - $ touch baz.ml bar.ml foo.ml + $ echo 'let value = 1' >baz.ml + $ echo 'let chain = Baz.value' >bar.ml + $ echo 'let use = Bar.chain' >foo.ml $ cat >dune < (library (public_name baz) (modules baz)) > (library (public_name bar) (libraries baz) (modules bar)) diff --git a/test/blackbox-tests/test-cases/watching/multiple-errors-output.t/run.t b/test/blackbox-tests/test-cases/watching/multiple-errors-output.t/run.t index 79458d994fb..97fd398c369 100644 --- a/test/blackbox-tests/test-cases/watching/multiple-errors-output.t/run.t +++ b/test/blackbox-tests/test-cases/watching/multiple-errors-output.t/run.t @@ -15,10 +15,18 @@ We test the output of the watch mode client when we have multiple errors 1 | let y = "unknown variable" ^ what ^^^^ Unbound value what - Error: Build failed with 2 errors. + File "$TESTCASE_ROOT/src/foo.ml", line 1, characters 39-40: + 1 | let f = Bar.x + Baz.y + invalid_syntax : ? = ! + ^ + Syntax error + Error: Build failed with 3 errors. [1] $ stop_dune + File "src/foo.ml", line 1, characters 39-40: + 1 | let f = Bar.x + Baz.y + invalid_syntax : ? = ! + ^ + Error: Syntax error File "libs/bar.ml", line 1, characters 8-20: 1 | let y = "type error" + 3 ^^^^^^^^^^^^ @@ -28,4 +36,4 @@ We test the output of the watch mode client when we have multiple errors 1 | let y = "unknown variable" ^ what ^^^^ Error: Unbound value what - Had 2 errors, waiting for filesystem changes... + Had 3 errors, waiting for filesystem changes...