test: opaque profile coverage for inter-library dep tracking#14331
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds blackbox cram coverage for how -opaque affects per-module inter-library dependency tracking across dev/release profiles, ensuring future regressions show up as targeted, self-explanatory test failures.
Changes:
- Add a test that pins
.cmxdep glob differences for local libraries between release (tracks*.cmx) and dev (tracks only*.cmi). - Add a test that pins
.cmxdeps for an external library (unix) remain tracked in both profiles. - Add a test that pins
.mlichanges still trigger consumer rebuilds in both profiles (i.e., opaque only affects.cmxpropagation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/blackbox-tests/test-cases/per-module-lib-deps/opaque-cmx-deps-local.t | Verifies local-lib .cmx glob deps are present in release but omitted in dev. |
| test/blackbox-tests/test-cases/per-module-lib-deps/opaque-cmx-deps-external.t | Verifies external-lib (unix) .cmx glob deps remain present in both profiles. |
| test/blackbox-tests/test-cases/per-module-lib-deps/opaque-mli-change.t | Verifies .mli changes force consumer rebuilds under both dev and release profiles. |
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 25, 2026
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 25, 2026
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 25, 2026
The exact-count assertions inherited from ocaml#14331 ([2] under both profiles) now observe [1] under both, because this PR's per-module filter narrows the consumer's dep set enough that one of the post-edit rebuild events the previous (glob-only) deps would have recorded no longer fires. Promote the assertions to [1] so the test continues to pass and documents the tighter behaviour produced by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 26, 2026
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 26, 2026
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 26, 2026
The exact-count assertions inherited from ocaml#14331 ([2] under both profiles) now observe [1] under both, because this PR's per-module filter narrows the consumer's dep set enough that one of the post-edit rebuild events the previous (glob-only) deps would have recorded no longer fires. Promote the assertions to [1] so the test continues to pass and documents the tighter behaviour produced by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
The exact-count assertions inherited from ocaml#14331 ([2] under both profiles) now observe [1] under both, because this PR's per-module filter narrows the consumer's dep set enough that one of the post-edit rebuild events the previous (glob-only) deps would have recorded no longer fires. Promote the assertions to [1] so the test continues to pass and documents the tighter behaviour produced by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Three observational tests pinning the visible consequences of dune's [-opaque] handling in [groups_for_cm_kind]. Together with the existing [opaque.t] (which covers the [.ml]-only-change rebuild behaviour) they document the four directly-observable code paths that depend on the [-opaque] flag. - [opaque-cmx-deps-local.t]: a local lib's [.cmx] is in the consumer's dep set under release; only [.cmi] under dev. - [opaque-cmx-deps-external.t]: an external (findlib-resolved) lib's [.cmx] is in the dep set under both profiles. Pins that the [Lib.is_local lib] guard in [groups_for_cm_kind] is intentional. - [opaque-mli-change.t]: a [.mli] change in a dep rebuilds consumers under both profiles. Pins that opaque mode is purely about [.cmx] propagation, not [.cmi] propagation. Future regressions in any of those four code paths fail a specific test with a self-explanatory name. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
The preamble comments referenced internal names ([groups_for_cm_kind], [lib_file_deps.ml], [Lib.is_local lib]). Replace with descriptions phrased entirely in terms of externally observable behaviour: file extensions, profile names, and consumer/dep relationships. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
force-pushed
the
robinbb-opaque-coverage
branch
from
f45d971 to
3500103
Compare
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
Each of the dep PRs supplying a test in [per-module-lib-deps/] was renamed per Leonidas's review on ocaml#14309: generic [libA]/[modA1]-style names replaced by role-bearing names like [dep_lib]/[spurious_rebuild]. This commit applies the same renames to this PR's local copies of those tests so they stay convergent with the dep PRs' canonical versions. Touches: - single-module-unreferenced-lib.t (ocaml#14308) - sibling-unreferenced-lib.t (ocaml#14309) - unwrapped-tight-deps.t (ocaml#14310) - lib-vs-lib-name-collision.t (ocaml#14324) - opaque-mli-change.t, opaque-cmx-deps-local.t (ocaml#14331) - transitive-unreferenced-lib.t (ocaml#14332) - wrapped-reexport-via-open-flag.t (ocaml#14346) - auto-wrapped-child-reexport.t (ocaml#14347) Pure rename + path/dir adjustments. Count assertions preserved (this PR's promotions to the post-fix counts are unchanged). Test suite passes. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 27, 2026
The exact-count assertions inherited from ocaml#14331 ([2] under both profiles) now observe [1] under both, because this PR's per-module filter narrows the consumer's dep set enough that one of the post-edit rebuild events the previous (glob-only) deps would have recorded no longer fires. Promote the assertions to [1] so the test continues to pass and documents the tighter behaviour produced by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
The exact-count assertions inherited from ocaml#14331 ([2] under both profiles) now observe [1] under both, because this PR's per-module filter narrows the consumer's dep set enough that one of the post-edit rebuild events the previous (glob-only) deps would have recorded no longer fires. Promote the assertions to [1] so the test continues to pass and documents the tighter behaviour produced by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Alizter
approved these changes
Apr 29, 2026
Collaborator
Alizter
left a comment
Alizter
left a comment
There was a problem hiding this comment.
I agree with copilot, printing the list of rebuilt targets would be better.
Replace [| length] with the actual list of [target_files] arrays, matching the pattern used elsewhere in the cluster (e.g. [single-module-lib.t]). The list spells out which build artifacts get rebuilt when a dep's [.mli] changes — currently the [.cmi]/ [.cmti] byte rule and the [.cmx]/[.o] native rule for the consumer [Main] module — so a future regression that adds, removes, or renames a target produces a self-explanatory diff instead of a bare numeric mismatch. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
The [length > 0] assertions were too permissive and would mask a spurious extra rebuild. Replace with the exact count under each profile (which happens to be 2 in both cases). Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
[unix] is a separate library bundled with the OCaml distribution and resolved through findlib; it isn't part of [Stdlib] proper. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
The exact-count assertions inherited from ocaml#14331 ([2] under both profiles) now observe [1] under both, because this PR's per-module filter narrows the consumer's dep set enough that one of the post-edit rebuild events the previous (glob-only) deps would have recorded no longer fires. Promote the assertions to [1] so the test continues to pass and documents the tighter behaviour produced by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
Replace [| length] with the actual list of [target_files] arrays, matching the pattern used elsewhere in the cluster (e.g. [single-module-lib.t]). Under this PR's per-module filter, only the consumer's native [.cmx]/[.o] rule rebuilds when a dep's [.mli] changes — the [.cmi]/[.cmti] byte rule is no longer invalidated. Mirror of the [opaque-mli-change.t] fix on PR ocaml#14331's branch [robinbb-opaque-coverage]; the file is shared between the two PRs since ocaml#14331 is pulled into this branch's base. There the rebuild list shows two rules ([.cmi]/[.cmti] byte + [.cmx]/[.o] native) because that PR doesn't apply the per-module filter; here it shows one because this PR's filter narrows the deps. Addresses Copilot's review feedback: ocaml#14331 (comment) Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
Three cluster tests landed on main as observational baselines (via PRs ocaml#14308, ocaml#14331, ocaml#14350) recording the trunk-today rebuild list. With this PR's per-module filter applied, the [.cmi]/[.cmti] byte rule no longer globs over a dep lib's objdir for unreferenced sibling modules, so: - [single-module-unreferenced-lib.t]: [spurious_rebuild]'s rebuild count drops from 1 to 0. - [opaque-mli-change.t]: only the [.cmx]/[.o] native rule re-runs (release and dev profile both); the byte rule's deps no longer include the changed dep lib's [.cmi]. - [implicit-transitive-deps-false.t]: same — the [.cmx]/[.o] native rule re-runs, the byte rule does not. Promote the asserted [target_files] arrays accordingly. No behavioural surprise here — all three drops match the design of the per-module filter; the upstream baselines exist specifically to be flipped by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
Cluster tests landed on main as observational baselines (via PRs ocaml#14308, ocaml#14331, ocaml#14350, and the transitive-unreferenced-module follow-up) recording the trunk rebuild list. With this PR's per-module filter applied, the [.cmi]/[.cmti] byte rule no longer globs over a dep lib's objdir for unreferenced sibling modules, so: - [single-module-unreferenced-lib.t]: [spurious_rebuild]'s rebuild count drops from 1 to 0. - [opaque-mli-change.t]: only the [.cmx]/[.o] native rule re-runs (release and dev profile both); the byte rule's deps no longer include the changed dep lib's [.cmi]. - [implicit-transitive-deps-false.t]: same — the [.cmx]/[.o] native rule re-runs, the byte rule does not. - [transitive-unreferenced-module.t]: the consumer doesn't even directly reference the transitive lib, so the filter drops it entirely; both rules are skipped, [target_files] becomes [[]]. Promote the asserted [target_files] arrays accordingly. No behavioural surprise here — all four drops match the design of the per-module filter; the upstream baselines exist specifically to be flipped by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
Apr 29, 2026
Cluster tests landed on main as observational baselines (via PRs ocaml#14308, ocaml#14331, ocaml#14350, and the transitive-unreferenced-module follow-up) recording the trunk rebuild list. With this PR's per-module filter applied, the [.cmi]/[.cmti] byte rule no longer globs over a dep lib's objdir for unreferenced sibling modules, so: - [single-module-unreferenced-lib.t]: [spurious_rebuild]'s rebuild count drops from 1 to 0. - [opaque-mli-change.t]: only the [.cmx]/[.o] native rule re-runs (release and dev profile both); the byte rule's deps no longer include the changed dep lib's [.cmi]. - [implicit-transitive-deps-false.t]: same — the [.cmx]/[.o] native rule re-runs, the byte rule does not. - [transitive-unreferenced-module.t]: the consumer doesn't even directly reference the transitive lib, so the filter drops it entirely; both rules are skipped, [target_files] becomes [[]]. Promote the asserted [target_files] arrays accordingly. No behavioural surprise here — all four drops match the design of the per-module filter; the upstream baselines exist specifically to be flipped by this PR. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three new cram tests under
test/blackbox-tests/test-cases/per-module-lib-deps/:opaque-cmx-deps-local.t— a local lib appears in the consumer's.cmxdep set as*.cmionly under dev (opaque=true), and as both*.cmiand*.cmxunder release (opaque=false).opaque-cmx-deps-external.t— an external library (findlib-resolvedunix) appears as both globs under both profiles, pinning that theLib.is_local libguard ingroups_for_cm_kindis the intended behaviour.opaque-mli-change.t— a.mlichange in a dep triggers a consumer rebuild under both profiles; pins that the dev-mode optimisation is about cmx propagation, not cmi propagation.Together with the existing
opaque.t(which covers.ml-only-change rebuild behaviour) these document the four directly-observable consequences of-opaque. Future regressions in any of the four code paths will fail a self-explanatory test rather than slipping through.Test plan
dune runtest test/blackbox-tests/test-cases/per-module-lib-deps/opaque-cmx-deps-local.tpasses.dune runtest test/blackbox-tests/test-cases/per-module-lib-deps/opaque-cmx-deps-external.tpasses.dune runtest test/blackbox-tests/test-cases/per-module-lib-deps/opaque-mli-change.tpasses.