fix: AND user-written menhir constraint with dune's required lower bound#14453
fix: AND user-written menhir constraint with dune's required lower bound#14453robinbb wants to merge 13 commits into
Conversation
e4d1605 to
5e505dc
Compare
There was a problem hiding this comment.
Pull request overview
Tightens Dune’s opam file generation for menhir dependencies so that Dune’s required minimum menhir version is correctly combined with user-written constraints, aligning behavior with the existing merge_dune_constraints logic (follow-up to #14434 / #14428).
Changes:
- Update
upgrade_menhir_constraintto merge user constraints with Dune’s required lower bound, warning and clamping when the user’s lower bound is too low. - Ensure non-version constraints (e.g.
:with-test) are ANDed with the required lower bound instead of replacing it. - Extend blackbox tests and add a changelog entry documenting the behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/dune_rules/opam_create.ml |
Implements merge_menhir_constraint and uses it when upgrading menhir deps in generated opam files (including warning + clamping behavior). |
test/blackbox-tests/test-cases/menhir/opam-menhir-dep.t |
Updates/extends coverage for filter ANDing and warns+clamps too-low user bounds. |
doc/changes/changed/menhir-merge-bound.md |
Documents the new menhir constraint-merging behavior in the changelog. |
5e505dc to
d512b53
Compare
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
I think its a great improvement, however we need to be a bit careful about not messing with existing dune projects that depend on the old behavior. We currently generate different OPAM files between Dune 3.22 and Dune 3.24 even if the users dune-lang is 3.22.
Follows up ocaml#14434, which fixed the over-injection bug from ocaml#14428 by filling [>= "20180523"] only on packages that already declare a [menhir] dep. That fix only handled the *bare* declaration case; user-written constraints were preserved verbatim, including bounds below dune's required minimum and non-version filters that didn't combine with the bound. Tighten [upgrade_menhir_constraint] in [opam_create.ml] to mirror [merge_dune_constraints]: - Two [>= "v"] literals collapse to the higher of the two. When the user's bound is below dune's required minimum, emit a [User_warning.emit] and use [>= "20180523"]. - Any other shape (e.g. [{with-test}]) is ANDed with the lower bound: [(menhir :with-test)] now generates [{>= "20180523" & with-test}], not just [{with-test}]. Gate the entire menhir upgrade step on [(lang dune 3.23)] or newer. Earlier lang versions get the user's constraint verbatim (no fill, no warn), parallelling ocaml#14466 / the fix for ocaml#14436. The 3.23 gate also covers the bare-[(menhir)] fill from ocaml#14434, which previously applied unversioned. Effects on the generated opam file (under [(lang dune 3.23)] or newer): - [(menhir (>= 20100101))] -> warn, [{>= "20180523"}]. - [(menhir :with-test)] -> [{>= "20180523" & with-test}]. - [(menhir (>= 20211128))] -> [{>= "20211128"}] (unchanged; user's bound already at or above dune's). - bare [(menhir)] -> [{>= "20180523"}] (was already the behaviour from ocaml#14434, now gated). Suggestion from @Leonidas-from-XIV in ocaml#14434 (comment) and agreed direction in ocaml#14434 (comment). Lang-version gate per @Leonidas-from-XIV's review of ocaml#14453. Updates [opam-menhir-dep.t] Case 5 expected output, adds Case 6 covering the override-low warning path, and adds Case 7 covering the [(lang dune 3.22)] gate-off behaviour. Signed-off-by: Robin Bate Boerop <[email protected]>
d512b53 to
34125d3
Compare
34125d3 to
c4aba61
Compare
Follows up ocaml#14434, which fixed the over-injection bug from ocaml#14428 by filling [>= "20180523"] only on packages that already declare a [menhir] dep. That fix only handled the *bare* declaration case; user-written constraints were preserved verbatim, including bounds below dune's required minimum and non-version filters that didn't combine with the bound. Tighten [upgrade_menhir_constraint] in [opam_create.ml] to mirror [merge_dune_constraints]: - Two [>= "v"] literals collapse to the higher of the two. When the user's bound is below dune's required minimum, emit a [User_warning.emit] and use [>= "20180523"]. - Any other shape (e.g. [{with-test}]) is ANDed with the lower bound: [(menhir :with-test)] now generates [{>= "20180523" & with-test}], not just [{with-test}]. Gate the entire menhir upgrade step on [(lang dune 3.23)] or newer. Earlier lang versions get the user's constraint verbatim (no fill, no warn), parallelling ocaml#14466 / the fix for ocaml#14436. The 3.23 gate also covers the bare-[(menhir)] fill from ocaml#14434, which previously applied unversioned. Effects on the generated opam file (under [(lang dune 3.23)] or newer): - [(menhir (>= 20100101))] -> warn, [{>= "20180523"}]. - [(menhir :with-test)] -> [{>= "20180523" & with-test}]. - [(menhir (>= 20211128))] -> [{>= "20211128"}] (unchanged; user's bound already at or above dune's). - bare [(menhir)] -> [{>= "20180523"}] (was already the behaviour from ocaml#14434, now gated). Suggestion from @Leonidas-from-XIV in ocaml#14434 (comment) and agreed direction in ocaml#14434 (comment). Lang-version gate per @Leonidas-from-XIV's review of ocaml#14453. Updates [opam-menhir-dep.t] Case 5 expected output, adds Case 6 covering the override-low warning path, and adds Case 7 covering the [(lang dune 3.22)] gate-off behaviour. Case 6 reads the generated file directly from [_build/default/] instead of going through [--auto-promote], and uses [dune_cmd print-from | print-until] to extract just the warning text (dropping the [Promoting] line that was incidental to what the case actually asserts), addressing review feedback that the prior compound [sed] was unreadable. Signed-off-by: Robin Bate Boerop <[email protected]>
Follows up ocaml#14434, which fixed the over-injection bug from ocaml#14428 by filling [>= "20180523"] only on packages that already declare a [menhir] dep. That fix only handled the *bare* declaration case; user-written constraints were preserved verbatim, including bounds below dune's required minimum and non-version filters that didn't combine with the bound. Tighten [upgrade_menhir_constraint] in [opam_create.ml] to mirror [merge_dune_constraints]: - Two [>= "v"] literals collapse to the higher of the two. When the user's bound is below dune's required minimum, emit a [User_warning.emit] and use [>= "20180523"]. - Any other shape (e.g. [{with-test}]) is ANDed with the lower bound: [(menhir :with-test)] now generates [{>= "20180523" & with-test}], not just [{with-test}]. Gate the entire menhir upgrade step on [(lang dune 3.23)] or newer. Earlier lang versions get the user's constraint verbatim (no fill, no warn), parallelling ocaml#14466 / the fix for ocaml#14436. The 3.23 gate also covers the bare-[(menhir)] fill from ocaml#14434, which previously applied unversioned. Effects on the generated opam file (under [(lang dune 3.23)] or newer): - [(menhir (>= 20100101))] -> warn, [{>= "20180523"}]. - [(menhir :with-test)] -> [{>= "20180523" & with-test}]. - [(menhir (>= 20211128))] -> [{>= "20211128"}] (unchanged; user's bound already at or above dune's). - bare [(menhir)] -> [{>= "20180523"}] (was already the behaviour from ocaml#14434, now gated). Suggestion from @Leonidas-from-XIV in ocaml#14434 (comment) and agreed direction in ocaml#14434 (comment). Lang-version gate per @Leonidas-from-XIV's review of ocaml#14453. Updates [opam-menhir-dep.t] Case 5 expected output, adds Case 6 covering the override-low warning path, and adds Case 7 covering the [(lang dune 3.22)] gate-off behaviour. Case 6 reads the generated file directly from [_build/default/] instead of going through [--auto-promote], and uses [dune_cmd print-from | print-until] to extract just the warning text (dropping the [Promoting] line that was incidental to what the case actually asserts), addressing review feedback that the prior compound [sed] was unreadable. Signed-off-by: Robin Bate Boerop <[email protected]>
c4aba61 to
a8f1787
Compare
Follows up ocaml#14434, which fixed the over-injection bug from ocaml#14428 by filling [>= "20180523"] only on packages that already declare a [menhir] dep. That fix only handled the *bare* declaration case; user-written constraints were preserved verbatim, including bounds below dune's required minimum and non-version filters that didn't combine with the bound. Tighten [upgrade_menhir_constraint] in [opam_create.ml] to mirror [merge_dune_constraints]: - Two [>= "v"] literals collapse to the higher of the two. When the user's bound is below dune's required minimum, emit a [User_warning.emit] and use [>= "20180523"]. - Any other shape (e.g. [{with-test}]) is ANDed with the lower bound: [(menhir :with-test)] now generates [{>= "20180523" & with-test}], not just [{with-test}]. Gate the entire menhir upgrade step on [(lang dune 3.23)] or newer. Earlier lang versions get the user's constraint verbatim (no fill, no warn), parallelling ocaml#14466 / the fix for ocaml#14436. The 3.23 gate also covers the bare-[(menhir)] fill from ocaml#14434, which previously applied unversioned. Effects on the generated opam file (under [(lang dune 3.23)] or newer): - [(menhir (>= 20100101))] -> warn, [{>= "20180523"}]. - [(menhir :with-test)] -> [{>= "20180523" & with-test}]. - [(menhir (>= 20211128))] -> [{>= "20211128"}] (unchanged; user's bound already at or above dune's). - bare [(menhir)] -> [{>= "20180523"}] (was already the behaviour from ocaml#14434, now gated). Suggestion from @Leonidas-from-XIV in ocaml#14434 (comment) and agreed direction in ocaml#14434 (comment). Lang-version gate per @Leonidas-from-XIV's review of ocaml#14453. Updates [opam-menhir-dep.t] Case 5 expected output, adds Case 6 covering the override-low warning path, and adds Case 7 covering the [(lang dune 3.22)] gate-off behaviour. Case 6 reads the generated file directly from [_build/default/] instead of going through [--auto-promote], and uses [dune_cmd print-from | print-until] to extract just the warning text (dropping the [Promoting] line that was incidental to what the case actually asserts), addressing review feedback that the prior compound [sed] was unreadable. Signed-off-by: Robin Bate Boerop <[email protected]>
a8f1787 to
b83c054
Compare
The feature is gated on (lang dune 3.23) per the implementation in upgrade_menhir_constraint and the test prose at the gate-boundary cases. Setup blocks for the at-or-above-gate cases used 3.24 (stale from before the backport target was lowered); align them to 3.23 so the canonical demonstration of the feature matches the gate's lowest supported lang version. Signed-off-by: Robin Bate Boerop <[email protected]>
Clarify the changes related to opam output on dune-binary upgrade and reference issue ocaml#14453. Signed-off-by: Robin Bate Boerop <[email protected]>
Per @shonfeder's review (ocaml#14453 r3221756169): the previous [merge_menhir_constraint] matched only top-level shapes ([>= "v"] or fall through to outer AND), missing nested cases like [(and :with-test (>= "20100101"))] where the user's lower bound is buried inside an [And]/[Or]/[Not]. The same shallowness was present in [merge_dune_constraints]. Factor out two helpers parameterised on [~min_version] and use them in both merge functions: - [has_sufficient_lower_bound]: over-approximates "every satisfying valuation of [c] has version >= min". Recurses through [And] (exists) and [Or] (forall); treats [Not], [Bop], and [Bvar] as opaque (returns false), so an outer AND with the minimum is added for those leaves. - [normalize_lower_bounds]: walks [c] replacing each [>= "v"] literal where [v < min] with [>= min]; returns the rewritten expression and a flag for whether any rewrite occurred so the caller emits a single user-facing warning. The merge then returns the normalized expression directly when the lower bound is already implied; otherwise it AND's the minimum at the top level. Five new test cases in [opam-menhir-dep.t] cover nested-[And] (low and high bounds), [Or] (filter-only branch escapes), version range, and the unsatisfiable case (no [>=] to tighten — output is generated faithfully and the conflict surfaces at opam install time). Signed-off-by: Robin Bate Boerop <[email protected]>
The merge function has two pattern arms that no existing test in [opam-menhir-dep.t] exercised: - [has_sufficient_lower_bound] treats [Uop (Eq, lit)] like [Uop (Gte, lit)] — a pinned version at or above the minimum implies the lower bound, so no outer AND is added. - [normalize_lower_bounds] descends into [Not], rewriting any [>= "v"] literal inside; [has_sufficient_lower_bound] treats [Not] as opaque, so the outer AND is still emitted. Add Cases 14 and 15 to pin both. Without Case 14, a regression dropping [Eq] from the sufficiency check would silently append the outer AND for [(= "v")] pins. Without Case 15, treating [Not] as opaque in [normalize] (skipping descent) would silently drop the warning, and treating [Not] as sufficient in the check would silently drop the outer AND. Also tighten the prose block preceding Cases 9-15 to match: the sufficiency check decomposes [And]/[Or] (and matches [Eq] on a version literal), while [Not] / [Bop] / [Bvar] / non-version [Uop]s are opaque. The previous wording listed [Not] as passing through unchanged, which is no longer accurate now that Case 15 demonstrates [Not] descent. Signed-off-by: Robin Bate Boerop <[email protected]>
- Test file: rewrite Cases 5-15 prose in terms of what the user writes in dune-project and what dune emits, instead of describing the merge algorithm. Convert bracket code-spans to backticks (per cram-test-tips.md). Drop the @shonfeder attribution. Reflow prose at 80 columns. - Implementation: drop the doc-comment above has_sufficient_lower_bound. - Changelog: drop the recursive-descent / sufficiency-check description; keep user-visible behavior. Signed-off-by: Robin Bate Boerop <[email protected]>
shonfeder
left a comment
There was a problem hiding this comment.
We discussed in a meeting today with @robinbb and @Alizter, and I we reached the conclusion that this way of trying to improve the dependency constraint is not worth the added complexity. If we already had a sound system for querying and updating these constraints, it could make sense to try to give a general solution to the dependency constraints.
Before that meeting, I made note of some issues in the current logic, and am posting those to help illustrating why the scope here quickly outgrows the expected benefit.
Our conclusion was that this will not go into the patch release, and I think will be closed, but I'll leave this up to @robinbb
Signed-off-by: Robin Bate Boerop <[email protected]>
[has_sufficient_lower_bound]: [(> v)] with v >= min_version is sufficient. [normalize_lower_bounds]: [(> v)] with v < min_version is rewritten to [(>= min_version)] and warns, parallel to the existing [(>= v)] case. [(= v)] (pin) is left untouched: rewriting it would silently drop the user's intent, so an unsupported pin surfaces at install time via the [(>= min_version)] AND. Signed-off-by: Robin Bate Boerop <[email protected]>
Both [merge_dune_constraints] and [merge_menhir_constraint] now short-circuit on [has_sufficient_lower_bound] of the original user expression, skipping [normalize_lower_bounds] when the user already implies the minimum. This suppresses a misleading warning for sub-minimum literals masked by a stronger sibling, e.g. [(and (>= 20211128) (>= 10))]: the [>= 10] is dead given the sibling, and the previous "the generated opam file will use >= min_version instead" warning was inaccurate since dune ended up using neither [>= 10] nor a substitute. Case 22 locks in the new behaviour. Signed-off-by: Robin Bate Boerop <[email protected]>
[merge_dune_constraints] previously took a [Package_constraint.t] just to destructure out the version string, and carried a dead [| _ -> ...] catch-all because the sole caller always supplies a [Gte] shape. It now takes [~min_version] directly. [merge_with_min_lower_bound] factors out the body shared by both [merge_dune_constraints] and [merge_menhir_constraint]: short-circuit on sufficient-as-is, then normalise, then AND. The two callers now differ only in the warning text and the [min_version] they pass. Signed-off-by: Robin Bate Boerop <[email protected]>
Under a [Not], a [(>= v)] literal contributes to an upper bound, not
a lower bound — rewriting it from [(>= v)] to [(>= min_version)]
shifts the upper-bound threshold and loosens the constraint, the
opposite of what [normalize_lower_bounds] is meant to do. The [Not]
arm now passes through unchanged.
Updates Case 15: [(not (>= 20100101))] is preserved verbatim, so the
generated opam file is now [{>= "20180523" & !>= "20100101"}] (still
unsatisfiable, but clearly reflects the user's [< 20100101] clashing
with dune's minimum rather than the spurious [!>= 20180523] the old
rewrite produced). No warning, since nothing was rewritten.
Signed-off-by: Robin Bate Boerop <[email protected]>
Adds a tier between the existing < 3.23 skip and the post-PR full merge behaviour, so that [(lang dune 3.23)] projects keep the pre-ocaml#14453 behaviour they shipped under and avoid silent changes to opam output on dune-binary upgrade. [merge_dune_constraints] (dune-package side): - >= 3.24: enhanced rewrite/normalise (this PR's new behaviour). - >= 3.23: [merge_dune_constraints_simple] — the pre-ocaml#14453 simple dedup body (resurrected). Preserves the ocaml#14436 / ocaml#14466 behaviour shipped in 3.23.1. - else: bare [And [lang_constraint; user_constraint]]. [upgrade_menhir_constraint] (menhir-package side): - >= 3.24: full upgrade — bare-fill plus [merge_menhir_constraint] for user-written constraints (this PR's new behaviour). - >= 3.23: [upgrade_menhir_constraint_simple] — the pre-ocaml#14453 bare-fill-only body (resurrected). Preserves the ocaml#14168 / ocaml#14434 behaviour shipped in 3.23.x. - < 3.23: skip entirely (the original guard added by this PR). Tests: - Cases 7-8 reverted to [(lang dune 3.22)] — they exercise the < 3.23 skip tier. - New Cases 23-24 at [(lang dune 3.23)] — they exercise the simple tier (bare-fill yes, user-constraint preserved verbatim). - Existing Cases 0-22 stay at [(lang dune 3.24)] for the enhanced tier. - [dune-dep.t]'s "lower bound below lang" warning reverts to the pre-ocaml#14453 wording, since lang 3.23 now uses the simple function again. Signed-off-by: Robin Bate Boerop <[email protected]>
The changelog and the two new function comments in [opam_create.ml] described behaviour by referring to specific past PRs. Rewritten to describe current functionality directly — what each function/tier does, not which PR introduced or shipped it. Test-case prose is untouched (test files are allowed to reference PRs for context). Signed-off-by: Robin Bate Boerop <[email protected]>
Let me make the case for this issue being the first one to start giving the "general solution to the dependency constraints":
Here are the other issues related to version bounds:
I'm not saying that all of these issues would necessarily benefit from this PR's code, merely that Dune often deals with version bounds stuff. |
Summary
Follow-up to #14434. Tightens
upgrade_menhir_constraintinopam_create.mlto mirrormerge_dune_constraints: two>=literals collapse to the higher (warn when the user's bound is below dune's minimum); other shapes are ANDed with the lower bound. Gated on(lang dune 3.23)or newer; the same gate also covers #14434's bare-fill (previously unversioned), parallelling #14466 / the fix for #14436.Stacked on #14466.Provenance
Suggested by @Leonidas-from-XIV in #14434 (discussion_r3193896678); agreed direction in #14434 (discussion_r3196549007). Lang-version gate per @Leonidas-from-XIV's review on this PR.