Skip to content

test: pps runtime libs reach melange compile rule#14608

Merged
robinbb merged 1 commit into
ocaml:mainfrom
robinbb:robinbb-test-melange-pps-runtime-libs
May 19, 2026
Merged

test: pps runtime libs reach melange compile rule#14608
robinbb merged 1 commit into
ocaml:mainfrom
robinbb:robinbb-test-melange-pps-runtime-libs

Conversation

@robinbb
Copy link
Copy Markdown
Collaborator

@robinbb robinbb commented May 19, 2026

Pins that a (melange.emit ...) stanza preprocessing with a ppx whose (ppx_runtime_libraries ...) is non-empty includes globs over the runtime lib's .cmi/.cmj melange objdir in the compile-rule dep set — even though no source names the runtime lib syntactically.

Melange compile rules bypass per-module narrowing today (can_filter = false in module_compilation.ml); the property holds via the wide cctx glob. Forward-looking guard if narrowing ever extends to melange.

@robinbb robinbb self-assigned this May 19, 2026
@robinbb robinbb requested a review from Copilot May 19, 2026 19:26
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a black-box cram test ensuring that a melange.emit stanza preprocessed with a ppx whose ppx_runtime_libraries includes a melange library results in the melange compile rule depending (via globs) on the runtime library's .cmi/.cmj melange objdir, even when no melange source syntactically references that runtime library.

Changes:

  • New cram test melange-pps-runtime-libs.t that sets up a melange runtime library, a ppx rewriter declaring it as a ppx_runtime_libraries, and a melange.emit consumer, then asserts via dune rules --deps + jq that the compile rule has the expected globs on the runtime lib's melange objdir.

@robinbb robinbb requested a review from Alizter May 19, 2026 19:40
@robinbb robinbb marked this pull request as ready for review May 19, 2026 19:40
@robinbb robinbb force-pushed the robinbb-test-melange-pps-runtime-libs branch from 466f6d2 to 7d7f9a2 Compare May 19, 2026 20:02
A `(melange.emit ...)` stanza that preprocesses with a ppx
declaring `(ppx_runtime_libraries hello)` must pull `hello` into
the melange compile rule's dep set as globs over its `.cmi`/`.cmj`
melange objdir, even though no source names `Hello` syntactically.

Melange compile rules bypass per-module narrowing today
(`can_filter = false` in `module_compilation.ml`), so the wide
`cctx` glob still applies; this fixture pins the property as a
forward-looking guard.

Signed-off-by: Robin Bate Boerop <me@robinbb.com>
@robinbb robinbb force-pushed the robinbb-test-melange-pps-runtime-libs branch from 7d7f9a2 to b40a6a5 Compare May 19, 2026 20:08
@robinbb robinbb requested a review from Copilot May 19, 2026 20:15
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@robinbb robinbb merged commit b8c6c74 into ocaml:main May 19, 2026
34 checks passed
@robinbb robinbb deleted the robinbb-test-melange-pps-runtime-libs branch May 19, 2026 23:20
@anmonteiro
Copy link
Copy Markdown
Collaborator

I'm curious why the filtering doesn't apply to Melange? can i read about the limitations somewhere?

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 20, 2026

@anmonteiro I don't think there was a technical reason other than being unfamiliar with the melange rules. @robinbb would it simplify the filtering if we extended it to melange?

@robinbb
Copy link
Copy Markdown
Collaborator Author

robinbb commented May 20, 2026

@anmonteiro @Alizter Yes, that's right. I would prefer not to have these special-case code paths for Melange, but since I do know of a test suite against which I can assert that I have not broken anything, and because I do not know the Melange special cases (or absence of special cases) myself, I am trying to carefully preserve the Melange behaviour. Also, there was already some Melange special-casing in the code prior to this work.

I would love to partner with you, @anmonteiro, if you have some time for it, to improve the handling of Melange. (However, it can come after the 9-layer PR #14492, as we need not block on it, IMO.)

@anmonteiro
Copy link
Copy Markdown
Collaborator

@robinbb that sounds great!

There might be some stuff around the .js emission in what they require, but melange compilation should rely only on ocamldep -- my mental model is that dependency filtering can work for it.

@robinbb
Copy link
Copy Markdown
Collaborator Author

robinbb commented May 21, 2026

@anmonteiro Okay, good start. What might break if we get it wrong? Is there a body of source code that we can build with the old/new Dune to check to see if we have the logic right?

@anmonteiro
Copy link
Copy Markdown
Collaborator

usually the melange repository itself can be pretty representative, since it builds its own runtime libs with cross-module optimizations and the entire runtime testsuite.

Usually if i want to get more confidence, I'll run it through the entire package set in nix-overlays, which should have most publicly available packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants