feat: finer dependency analysis between libraries (#4572)

[robinbb]

Implements per-module library dependency filtering as proposed in #4572. When
library A depends on library B, Dune currently assumes every module in A
depends on every module in B — causing full recompilation of A whenever any
module in B changes. This PR uses ocamldep's per-module output to declare
cross-library file dependencies only for libraries a module actually
references.

This PR builds on prerequisite PRs:

• test: document inter-library recompilation behavior (#4572) #14017 — baseline tests documenting current recompilation behavior (merged)
• test: document module name shadowing between stanza and library (#4572) #14031 — test documenting module name shadowing between stanzas and libraries
• test: verify library file deps in compilation rules and sandboxed builds (#4572) #14100 — test verifying library file deps in compilation rules and sandboxed builds
• test: verify transparent alias incremental build safety (#4572) #14101 — safety test for incremental builds with transparent aliases
• test: verify incremental builds with alias re-exported libraries (#4572) #14129 — test verifying incremental builds with alias re-exported libraries
• refactor: move library file deps from Includes to per-module in build_cm (#4572) #14030 — refactor moving Hidden_deps from Includes.make to per-module in build_cm

Depends on #14030 and #14129.

The changes in this PR (on top of the above):

• A new Lib_index (in lib_file_deps.ml) maps library entry module names
  back to their libraries. It is computed once per stanza and stored in
  Compilation_context.t.
• read_immediate_deps_raw_of reads raw ocamldep output (including
  cross-library references that were previously silently dropped), filters
  out stanza-internal names, and looks up the remainder in the index to
  determine which libraries are actually needed.
• Conservative fallbacks ensure correctness: special module kinds, link-time
  generated modules, and unresolvable references fall back to all-library
  deps.
• Baseline test expectations are updated to show the improved behaviour.

Resolves #4572

[rgrinberg]

Thanks for working on this. I expect users are going to be quite pleased to have this feature. Some preliminary questions:

Includes.make no longer bundles Hidden_deps with -I flags. Include
flags remain shared across all modules in a stanza; file dependencies are
now declared per-module in build_cm.

Why are the include flags are needed if we don't expect the library to be used by the module?

For each module, read_immediate_deps_raw_of reads the raw ocamldep output
(including cross-library references that were previously silently dropped),
filters out stanza-internal names, and looks up the remainder in the index
to determine which libraries are actually needed.

Do we have tests for when an internal module name is shadowing a library?

Conservative fallbacks ensure correctness: alias and wrapped-compat modules
skip library deps entirely; special module kinds, link-time generated
modules, and unresolvable references fall back to all-library deps.

Is it possible to move these changes to a separate PR and rebase this work on top of that?

[robinbb]

Is it possible to move these changes to a separate PR and rebase this work on top of that?

@rgrinberg I have tried to do so with this PR #14030, depending on what you really meant by "these changes". :-)

[robinbb]

Thanks for working on this. I expect users are going to be quite pleased to have this feature.

You're very welcome, and thank you for your prompt attention to it!

[robinbb]

Do we have tests for when an internal module name is shadowing a library?

I guess not. I have made #14031 for this. Please have a look.

[nojb]

Thanks for working on this patch @robinbb!

I tried this in our codebase by adding a new (unused) module to a library which is close to the root of the dependency tree, and doing dune build. Then I modified the unused module and redid dune build. At this point I expected for there not to be any recompilation at all, since the modified module is not used anywhere in the codebase. However, I observe some recompilation of modules (in other libraries) which do not depend on it. The libraries involved are all unwrapped.

Do you have any insight as to what may be causing the extra recompilation?

[nojb]

Do you have any insight as to what may be causing the extra recompilation?

I think one case where unexpected extra recompilation is triggered is when there a library consisting of a single module. If memory serves there is an optimization that avoids calling ocamldep in this case, but that may be defeating the optimization in this patch.

[Alizter]

Transparent module aliases hide library references from ocamldep when a module re-exports a library wrapper via module M = Mylib, ocamldep will miss the dependency on Mylib.

Here is the regression:

  $ cat > dune-project << EOF
  > (lang dune 3.0)
  > EOF
  $ mkdir lib && cat > lib/dune << EOF
  > (library (name mylib))
  > EOF
  $ cat > lib/mylib.ml << EOF
  > let v = 42
  > EOF
  $ cat > lib/mylib.mli << EOF
  > val v : int
  > EOF
  $ cat > dune << EOF
  > (executable (name main) (libraries mylib))
  > EOF
  $ cat > re.ml << EOF
  > module M = Mylib
  > EOF
  $ cat > re.mli << EOF
  > module M = Mylib
  > EOF
  $ cat > main.ml << EOF
  > let () = print_int Re.M.v
  > EOF

  $ dune build ./main.exe

Change mylib's interface:

  $ cat > lib/mylib.mli << EOF
  > val v : int
  > val w : string
  > EOF
  $ cat > lib/mylib.ml << EOF
  > let v = 42
  > let w = ""
  > EOF

The incremental build must succeed:

  $ dune build ./main.exe
  File "_none_", line 1:
  Error: Files .main.eobjs/native/dune__exe__Main.cmx and lib/mylib.cmxa
         make inconsistent assumptions over interface Mylib
  [1]

This probably also occurs for any module from Mylib not just the wrapper, so affects unwrapped, wrapped and singleton buildable stanzas. Before this change, the coarse glob would handle this unknowingly.

[nojb]

Do you have any insight as to what may be causing the extra recompilation?

I think one case where unexpected extra recompilation is triggered is when there a library consisting of a single module. If memory serves there is an optimization that avoids calling ocamldep in this case, but that may be defeating the optimization in this patch.

Continuing to investigate this, an .mli in a library A containing only

val foo: Buffer.t

triggers the recompilation of the .cmi every time when a module in a library on which A depends on is changed. From a distance this looks like some missing logic to handle dependencies on stdlib modules.

[art-w]

Very cool work indeed!

Is you new PR #14116 superseding this one?

Either way, I think it's worth spending more time to improve the test suite, because it's easy to accidentally rely on subtle behaviors that are more permissive than the general case. For example, the -opaque flag makes it possible to skip some recompilations on benign changes, even though these recompilations would normally be required.

Here's a small reproduction which should help debug the issue where a library re-exports one of its dependencies with a module alias. This cram test also shows the subtleties in the setup that could make it pass (when it shouldn't!)

---

First we need a library where we'll perform the changes:

$ mkdir impl
$ cat > impl/dune <<EOF
> (library (name impl))
> EOF
$ cat > impl/impl.ml <<EOF
> let foo = "initial build"
> EOF

Then another library, which exposes an alias to impl:

$ mkdir alias
$ cat > alias/dune <<EOF
> (library (name alias) (libraries impl))
> EOF
$ cat > alias/alias.ml <<EOF
> module Impl = Impl
> EOF

And a binary which depends on Alias to access Impl:

$ mkdir bin
$ cat > bin/dune <<EOF
> (executable (name main) (libraries alias))
> EOF
$ cat > bin/main.ml <<EOF
> let () = print_endline Alias.Impl.foo
> EOF

Compilation is different when the executable is defined in a single file, so the bug doesn't trigger unless we also add an empty unused file: (!!)

$ touch bin/unused.ml

The first build succeeds:

$ dune exec ./bin/main.exe
initial build

Now we introduce a "soft" update, such that impl.cmi isn't modified:

$ cat > impl/impl.ml <<EOF
> let foo = "second build, no change to cmi"
> EOF

In that specific case, we are allowed to not recompile bin/main.ml because the -opaque flag guarantees we are not dependent on the implementation of Impl: (the linker checks that the cmi digest hasn't changed)

$ dune exec ./bin/main.exe
second build, no change to cmi

But if the cmi of Impl changed, then skipping the recompilation of main.ml is wrong: (the linker check fails)

$ cat > impl/impl.ml <<EOF
> let new_value = 42
> let foo = "third build, forced a cmi update"
> EOF

$ dune exec ./bin/main.exe
File "_none_", line 1:
Error: Files bin/.main.eobjs/native/dune__exe__Main.cmx and impl/impl.cmxa
       make inconsistent assumptions over interface Impl
[1]

---

Regarding revdeps testing, one issue to keep in mind is that testing a clean build (and not an incremental build) is likely to hide the failures:

$ dune clean
$ dune exec ./bin/main.exe
third build, forced a cmi update

[robinbb]

Is you new PR #14116 superseding this one?

No, not necessarily. I have 2 PRs because I need to test more than one thing at a time, and I need access to the CI invocations. (Very difficult to replicate the CI corner cases that are failing, now, for this effort. Non-corner cases are passing.)

[robinbb]

Either way, I think it's worth spending more time to improve the test suite, because it's easy to accidentally rely on subtle behaviors that are more permissive than the general case. For example, the -opaque flag makes it possible to skip some recompilations on benign changes, even though these recompilations would normally be required.

Yes, agreed - we need more tests. The absence of test coverage is what is making it so difficult to get this right. It is difficult to see just how many different scenarios are actually working (but non-optimal), in the status quo. I have submitted many augmentations already to the test suite in dependency PRs to this one (14017, 14031, 14100, 14101, 14103) in an effort to reduce the risk, already. More still are required.

I see your advice for a suggested new test(s). I'll try to recreate that in a cram test. Possibly in a different PR (onto which I will rebase this branch).

[robinbb]

Is you new PR #14116 superseding this one?

No, not necessarily. I have 2 PRs because I need to test more than one thing at a time, and I need access to the CI invocations. (Very difficult to replicate the CI corner cases that are failing, now, for this effort. Non-corner cases are passing.)

In the end, yes, it is.

[robinbb]

Deprecated in favour of #14116