Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align QCheck 0.9 and 0.10 versions #24703

Merged
merged 11 commits into from
Nov 7, 2023
Merged

Conversation

jmid
Copy link
Contributor

@jmid jmid commented Oct 27, 2023

The qcheck* opam files allow mixing versions across releases such as

  - install   qcheck        0.10   [required by fadbadml]
  - install   qcheck-core   0.21.2 [required by qcheck]
  - install   qcheck-ounit  0.21.2 [required by qcheck]

which ends up triggering a fadbadml lower bound failure in #24702:

#=== ERROR while compiling fadbadml.0.1.2 =====================================#
# context              2.2.0~alpha3~dev | linux/x86_64 | ocaml-base-compiler.5.1.0 | file:///home/opam/opam-repository
# path                 ~/.opam/5.1/.opam-switch/build/fadbadml.0.1.2
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p fadbadml -j 71 @install @runtest
# exit-code            1
# env-file             ~/.opam/log/fadbadml-7-6ebe6b.env
# output-file          ~/.opam/log/fadbadml-7-6ebe6b.out
### output ###
# (cd _build/default && /home/opam/.opam/5.1/bin/ocamlc.opt -w -40 -g -bin-annot -I test/unit/.run_test.eobjs/byte -I /home/opam/.opam/5.1/lib/bytes -I /home/opam/.opam/5.1/lib/ocaml/unix -I /home/opam/.opam/5.1/lib/ounit2 -I /home/opam/.opam/5.1/lib/ounit2/advanced -I /home/opam/.opam/5.1/lib/qcheck -I /home/opam/.opam/5.1/lib/qcheck-core -I /home/opam/.opam/5.1/lib/qcheck-core/runner -I /home/opam/.opam/5.1/lib/qcheck-ounit -I /home/opam/.opam/5.1/lib/seq -I /home/opam/.opam/5.1/lib/stdlib-shims -I src/.fadbad.objs/byte -no-alias-deps -open Dune__exe -o test/unit/.run_test.eobjs/byte/dune__exe__Run_test.cmo -c -impl test/unit/run_test.ml)
# File "test/unit/run_test.ml", line 43, characters 16-39:
# 43 |       match res.QCheck.TestResult.state with
#                      ^^^^^^^^^^^^^^^^^^^^^^^
# Error: Unbound record field QCheck.TestResult.state

For all qcheck* releases from 0.11 and above their versions are aligned.
This PR extends that range to uniformly include 0.9 and 0.10 too.

@raphael-proust
Copy link
Collaborator

This MR is a strict improvement ("consider for merge").

But there are some interesting errors in the CI:

qcheck-alcotest.0.10
lower-bounds (failed: The type constructor Alcotest.test_case expects 0 argument(s),)

We could use the opportunity to fix those, or we can do it in a follow-up MR.

@jmid
Copy link
Contributor Author

jmid commented Oct 31, 2023

I agree Raphael!

That one happens when testing qcheck-alcotest with alcotest.0.5.0, so I suspect qcheck-alcotest is missing a proper alcotest lower bound...
There are several other failed: Unbound value ... errors indicating missing lower bounds in other packages using qcheck.

@jmid
Copy link
Contributor Author

jmid commented Oct 31, 2023

I noticed an alcotest lower bound for qcheck-alcotest.0.9 but none for 0.10. 9bcbc1a therefore adds one.
Versions 0.11-0.18.1 were unfortunately also missing lower bounds so c0fe758 adds them.

Finally I spotted multiple opam lint warnings. f5447d2 fixes the don't-finish-synopsis-with-a-dot one.

@jmid
Copy link
Contributor Author

jmid commented Oct 31, 2023

The latest commit gets the 0.9-0.17 packages linting cleanly

@jmid jmid mentioned this pull request Nov 1, 2023
@jmid
Copy link
Contributor Author

jmid commented Nov 1, 2023

From qcheck.0.9 the package was split into a qcheck-core package - and a backward compatible qcheck package.
This unfortunately opens for weird situations found by the opam lower bound search such as this one https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/8607834eb613eb7bac64c9ccbcbb86dd1a0053fa/variant/compilers,4.14,qcheck-alcotest.0.9,revdeps,pratter.1.2.1
allowing for

  - install qcheck-alcotest    0.9
  - install qcheck-core        0.9
  - install qcheck             0.7

I guess this can be solved by either

  • marking qcheck-core.0.9 and greater as conflicting with qcheck < "0.9"
  • marking qcheck < "0.9" as conflicting with qcheck-core

I've gone for the latter in d545271.
While fiddling in these ancient opam files I spotted that many of them would not pass opam lint.
In 137b25f I thus added missing fields and removed the final synopsis dots.

@jmid
Copy link
Contributor Author

jmid commented Nov 1, 2023

#24734 fixes missing lower bounds for some of revdeps.

@jmid
Copy link
Contributor Author

jmid commented Nov 2, 2023

Rebased on master after #24734 has been merged

@jmid
Copy link
Contributor Author

jmid commented Nov 7, 2023

Finally, pratter.3.0.0 failed with Unbound value char_range because the test lower bound for qcheck should not be 0.9 as stated - and undetected - in #24746 but 0.12 as it was set to in #24734 for pratter.2.0.0. CC @gabrielhdt

I'm starting to suspect that this is a coincidence:
Is the reason that we catch this on changes to dependencies (here qcheck) and not on the original package (here pratter) that dependencies are installed with --with-test during a revdeps lower bound search?
Would you know @kit-ty-kate?

If so, extending the lower bound search to test dependencies would be a nice feature to add... 🙏 🙂

@mseri
Copy link
Member

mseri commented Nov 7, 2023

Thanks

@mseri mseri merged commit f06c012 into ocaml:master Nov 7, 2023
1 check failed
@jmid jmid deleted the qcheck-version-align branch November 7, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants