fix: only fill lower bound on existing menhir deps; do not auto-add by robinbb · Pull Request #14434 · ocaml/dune

robinbb · 2026-05-05T22:46:00Z

Summary

The 3.23 menhir auto-injection (#14168) was triggered by the project-wide (using menhir ...) extension and added menhir as a new dependency to every package's generated opam file, regardless of whether the package's own (depends ...) field declared it. In multi-package projects where only some packages used menhir, every package gained a spurious build-time dependency.

The original ask in #10707 was narrower: when a generated opam file already lists menhir, fill in the lower bound {>= "20180523"} that dune's menhir rules require. This PR restricts the behaviour to that.

Behaviour

A package that does not declare (depends menhir) no longer has menhir added by dune.
A package that declares bare (depends menhir) has the lower bound filled in.
A package whose (depends ...) already specifies a constraint (version bound, {with-test}, etc.) is preserved verbatim.
Gated on (using menhir ...) being enabled — without the extension, no upgrade.

The convenience of auto-adding menhir for users who use a (menhir ...) stanza but forget to declare (depends menhir) is deliberately removed: the dependency-inference logic that supported it was the source of #14428 and out of scope for #10707.

@robinbb

Bump the cram tests from `(lang dune 3.23)` to `(lang dune 3.24)` and rename the changelog entry to the PR number to match repo convention (`(ocaml#14434, fixes ocaml#14428, @robinbb)`). Signed-off-by: Robin Bate Boerop <me@robinbb.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes #14428 by limiting Dune’s auto-injected menhir (>= 20180523) opam dependency to only those packages that actually use a (menhir ...) stanza, instead of injecting it into every package when (using menhir ...) is enabled project-wide.

Changes:

Compute a per-project packages_using_menhir set by scanning dune files for (menhir ...) stanzas and mapping co-located installable stanzas back to packages.
Thread packages_using_menhir through Opam_create.generate/opam_fields and update callers.
Add/adjust blackbox tests (including a new multi-package regression test) and add a changelog entry.

Reviewed changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/blackbox-tests/test-cases/menhir/opam-menhir-dep.t	Updates existing test setup to ensure the package actually contains a `(menhir ...)` stanza.
test/blackbox-tests/test-cases/menhir/opam-menhir-dep-multi-package.t	Adds regression coverage for multi-package projects where only some packages use menhir.
src/dune_rules/opam_create.mli	Exposes `packages_using_menhir` and updates `generate` API to accept the computed set.
src/dune_rules/opam_create.ml	Implements `packages_using_menhir` and gates menhir dep injection on membership in that set.
bin/dune_init.ml	Updates call site to new `generate` signature.
bin/describe/describe_opam_files.ml	Computes `packages_using_menhir` before generating opam contents.
doc/changes/fixed/14434.md	Documents the fix in the changelog.

@Opam

Two of the four Copilot comments are still applicable; the third was already resolved by a later commit on this branch (the [(mli)] doc no longer claims context-independence — see e7bdede which restored the [Context_name.t] parameter). - `add_rules`: skip the [packages_using_menhir] computation in [`Inside_opam_directory] mode, where [add_opam_file_rule] is invoked from [add_opam_file_rules] instead and the value here is unused. Eliminates the duplicate scan flagged by Copilot. - Cram tests: replace the `[1]`-exit-code expectations on `dune build @Opam --auto-promote` with `|| true`. The exit code (non-zero on first promotion, zero thereafter) is incidental to what the tests prove; the file-content assertions are what matter. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.

Mirrors the existing `has_standard`. Callers that need to summarise an `Unexpanded.t` syntactically — without reading the file an `(:include FN)` directive points to — must now check `has_include` and widen their answer accordingly, since `fold_strings` skips `Include` AST nodes entirely. Used by the menhir auto-injection in opam-create (ocaml#14434, fixes ocaml#14428): a `(modules (:include FN))` library would otherwise be mis-summarised as compiling no modules. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 3 comments.

Alizter · 2026-05-06T01:53:04Z

This fix has grown quite large. It might be worth considering if we want to revert the original feature and reopen that issue until a better implementation can be found. Otherwise this will be painful to backport.

Mirrors the existing `has_standard`. Callers that need to summarise an `Unexpanded.t` syntactically — without reading the file an `(:include FN)` directive points to — must now check `has_include` and widen their answer accordingly, since `fold_strings` skips `Include` AST nodes entirely. Used by the menhir auto-injection in opam-create (ocaml#14434, fixes ocaml#14428): a `(modules (:include FN))` library would otherwise be mis-summarised as compiling no modules. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

robinbb · 2026-05-06T02:08:05Z

This fix has grown quite large. It might be worth considering if we want to revert the original feature and reopen that issue until a better implementation can be found. Otherwise this will be painful to backport.

I can simplify....

robinbb · 2026-05-06T02:19:13Z

@Alizter The PR is now small, and easily reviewed. Thanks for pointing out that it was getting out of control. :-)

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Fixes ocaml#14428. The 3.23 menhir auto-injection (ocaml#14168) was triggered by the project-wide `(using menhir ...)` extension and added `menhir` as a new dependency to every package's generated opam file, regardless of whether the package's own `(depends ...)` field declared it. In multi-package projects where only some packages used menhir, every package gained a spurious build-time dependency. The original ask in ocaml#10707 was narrower: when a generated opam file *already* lists `menhir`, fill in the lower bound `{>= "20180523"}` that dune's menhir rules require. Restrict the behaviour to that. - A package that does not declare `(depends menhir)` no longer has `menhir` added by dune. - A package that declares bare `(depends menhir)` has the lower bound filled in, as before. - A package whose `(depends ...)` already specifies a constraint (version bound, `{with-test}`, etc.) is preserved verbatim. The convenience of auto-adding `menhir` for users who use a `(menhir ...)` stanza but forget to declare `(depends menhir)` is deliberately removed: the dependency-inference logic that supported it was the source of ocaml#14428 and out of scope for ocaml#10707. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

Copilot

Pull request overview

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

Leonidas-from-XIV

I'm ok with how it is, just wondering if it wouldn't be better to inject the constraint always.

Edit: Originally had a comment about testing multiple packages but I missed that the test includes a case for this.

Leonidas-from-XIV · 2026-05-06T08:03:21Z

+    if Package.Name.equal dep.name menhir_name
+    then
+      { dep with
+        constraint_ = Some (Option.value dep.constraint_ ~default:menhir_constraint)


Would it not make more sense to add to the constraint instead of passing on when there's with-test or a potentially lower bound? Adding a >= 2000 constraint when the existing constraint is >= 3000 seems safe to me. It might look a bit strange but not an issue.

I believe we do (or at least did) something like this in for dune dependencies where we would add a >= dune-lang-ver constraint, even if the user already had a constraint defined.

Good point — merge_dune_constraints does exactly this for the dune dep (AND with the user's constraint, take the higher of two >= literals, warn on too-low). The same shape for menhir would catch users who wrote a too-low bound and would tighten Case 5's :with-test to {>= "20180523" & with-test}. I'd prefer to do that as a follow-up though — this PR's scope is the over-injection fix from #14428, and merge-tightening is a separable semantic improvement that needs its own warning text and a couple of new test cases. Happy to file the follow-up.

Leonidas-from-XIV · 2026-05-06T08:07:05Z

+constraint and is preserved verbatim — the lower bound is not
+combined with it.
+
+  $ rm -f bar.opam


Is this necessary?

It is — Case 4 generates a bar.opam, and Case 5's dune-project has no bar package stanza, so without the rm -f dune errors out on the orphan opam file before regenerating foo.opam. Added a comment in the test explaining this.

Case 4 leaves a [bar.opam] behind. Case 5's dune-project has no [bar] package stanza, so dune errors on the orphan opam file before regenerating [foo.opam] unless [bar.opam] is removed first. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

@Opam

Each case ran [dune build @Opam --auto-promote] (allowed to exit [1] when a promotion happened), then [dune build @Opam] again to confirm a fixed point, then [grep] on the promoted file. The fixed-point check is generic dune behaviour, not specific to what this test exercises (the content of the [menhir] line in the generated opam file). Drop it; the [grep] assertions still cover the intended behaviour. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

robinbb · 2026-05-06T17:36:12Z

I'm ok with how it is, just wondering if it wouldn't be better to inject the constraint always.

Yes. I went down that path initially, and that's what caused the PR to become too big - the logic for figuring out where to inject it. Perhaps I was making a simple mistake, but I did not have a simple solutions. So, I fell back to this, because this is all that is required to both solve the open bug (#14428) and the original issue (#10707).

Copilot

Pull request overview

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

robinbb self-assigned this May 5, 2026

robinbb requested a review from Leonidas-from-XIV May 5, 2026 22:59

robinbb requested a review from Copilot May 5, 2026 23:08

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread src/dune_rules/opam_create.ml Outdated

Comment thread src/dune_rules/opam_create.ml Outdated

Comment thread test/blackbox-tests/test-cases/menhir/opam-menhir-dep-multi-package.t Outdated

Comment thread src/dune_rules/opam_create.mli Outdated

robinbb requested a review from Copilot May 6, 2026 00:03

Copilot started reviewing on behalf of robinbb May 6, 2026 00:04 View session

robinbb added the menhir Related to the internal menhir plugin label May 6, 2026

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread src/dune_rules/opam_create.ml Outdated

robinbb force-pushed the robinbb-fix-14428-menhir-per-package branch from 67bd319 to 7ac6714 Compare May 6, 2026 01:21

robinbb requested a review from Copilot May 6, 2026 01:22

Copilot started reviewing on behalf of robinbb May 6, 2026 01:23 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread src/dune_rules/opam_create.ml Outdated

Comment thread src/dune_rules/opam_create.ml Outdated

Comment thread src/dune_rules/opam_create.ml Outdated

robinbb force-pushed the robinbb-fix-14428-menhir-per-package branch from 7ac6714 to 114d281 Compare May 6, 2026 01:57

robinbb force-pushed the robinbb-fix-14428-menhir-per-package branch 2 times, most recently from a43da16 to 2e68c3f Compare May 6, 2026 02:16

robinbb requested a review from Copilot May 6, 2026 02:17

Copilot started reviewing on behalf of robinbb May 6, 2026 02:17 View session

robinbb assigned Leonidas-from-XIV May 6, 2026

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread src/dune_rules/opam_create.ml

Comment thread test/blackbox-tests/test-cases/menhir/opam-menhir-dep.t

robinbb changed the title ~~fix: only inject menhir opam dep into packages that use menhir~~ fix: only fill lower bound on existing menhir deps; do not auto-add May 6, 2026

robinbb force-pushed the robinbb-fix-14428-menhir-per-package branch from 2e68c3f to 071e2ec Compare May 6, 2026 02:27

robinbb requested a review from Copilot May 6, 2026 02:28

Copilot started reviewing on behalf of robinbb May 6, 2026 02:29 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread src/dune_rules/opam_create.ml

robinbb force-pushed the robinbb-fix-14428-menhir-per-package branch from 071e2ec to 949d78f Compare May 6, 2026 02:36

robinbb requested a review from Copilot May 6, 2026 02:37

Copilot started reviewing on behalf of robinbb May 6, 2026 02:37 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

robinbb marked this pull request as ready for review May 6, 2026 02:42

Leonidas-from-XIV requested changes May 6, 2026

View reviewed changes

robinbb added 2 commits May 6, 2026 10:24

robinbb requested a review from Copilot May 6, 2026 17:36

Copilot started reviewing on behalf of robinbb May 6, 2026 17:37 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

robinbb requested a review from Leonidas-from-XIV May 6, 2026 18:02

shonfeder mentioned this pull request May 6, 2026

3.23.1 release tracker #14441

Open

10 tasks

Conversation

robinbb commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behaviour

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alizter commented May 6, 2026

Uh oh!

robinbb commented May 6, 2026

Uh oh!

robinbb commented May 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Leonidas-from-XIV left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leonidas-from-XIV May 6, 2026

Choose a reason for hiding this comment

Uh oh!

robinbb May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Leonidas-from-XIV May 6, 2026

Choose a reason for hiding this comment

Uh oh!

robinbb May 6, 2026

Choose a reason for hiding this comment

Uh oh!

robinbb commented May 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robinbb commented May 5, 2026 •

edited

Loading

Leonidas-from-XIV left a comment •

edited

Loading