c-cube / qcheck

QuickCheck inspired property-based testing for OCaml.
https://c-cube.github.io/qcheck/
BSD 2-Clause "Simplified" License
347 stars 36 forks source link

Deriver: missing dependency? #207

Closed jmid closed 2 years ago

jmid commented 2 years ago

A fresh install of the development version from a clean local switch w/4.13.1 misses ppx_deriving it seems: @vch9 ?

$ opam switch create . 4.13.1
$ opam install ocamlfind
$ opam install utop
$ opam source qcheck --dev
$ opam pin add qcheck qcheck/
$ opam pin add ppx_deriving_qcheck qcheck/
$ utop
────────────────────────────────────────┬─────────────────────────────────────────────────────────────┬────────────────────────────────────────
                                        │ Welcome to utop version 2.8.0 (using OCaml version 4.13.1)! │                                        
                                        └─────────────────────────────────────────────────────────────┘                                        
Findlib has been successfully loaded. Additional directives: [...]

utop # #require "qcheck";;

utop # #require "ppx_deriving_qcheck";;
No such package: ppx_deriving - required by `ppx_deriving_qcheck'

utop # #require "ppx_deriving";;
No such package: ppx_deriving

When I've used ppx_deriving_qcheck it has been together with ppx_deriving.show - so I wouldn't catch the missing dependency.

@vch9 Do you agree?

vch9 commented 2 years ago

I have the same issue with your commands. I do not really understand why is there a dependency missing when the build nor the install do not complain. I do not know that much utop, maybe he requires an historical dependency to ppx_deriving?

c-cube commented 2 years ago

I thought this used ppxlib, hence my comment on deriving_inline recently. The difference should be relatively small but ppxlib's deriving API is definitely the modern way to go. I wrote a small deriver using it, for reference: mhash.

pitag-ha commented 2 years ago

That's a very interesting situation! I've just had a quick look and ppx_deriving_qcheck really doesn't seem to use ppx_deriving at all (some derivers use ppxlib for some things and then ppx_deriving for registration). Also, ppx_deriving isn't a transitive dependency of ppx_deriving_qcheck. Maybe the problem is that the deriver's "kind" is declared as "ppx_deriver" instead of "ppx_rewriter"? Could you check if making it a ppx_rewriter would solve the problem?

jmid commented 2 years ago

My first hypothesis was that this was somehow a consequence of using it interactively.

So I created a simple file test.ml:

open QCheck
type pair = int * int [@@deriving qcheck]
;;
Gen.generate1 gen_pair |> Print.(pair int int) |> print_endline

...which shot that down:

$ ocamlfind ocamlopt -package qcheck,ppx_deriving_qcheck -linkpkg test.ml
ocamlfind: Package `ppx_deriving' not found - required by `ppx_deriving_qcheck'

The ocamlfind (which utop uses) error made me look in the META file for ppx_deriving_qcheck. That file lists the dependency:

$ cat _opam/lib/ppx_deriving_qcheck/META 
version = "0.2.0"
description = ""
requires(ppx_driver) = "ppxlib ppxlib.ast"
archive(ppx_driver,byte) = "ppx_deriving_qcheck.cma"
archive(ppx_driver,native) = "ppx_deriving_qcheck.cmxa"
plugin(ppx_driver,byte) = "ppx_deriving_qcheck.cma"
plugin(ppx_driver,native) = "ppx_deriving_qcheck.cmxs"
# This is what dune uses to find out the runtime dependencies of
# a preprocessor
ppx_runtime_deps = "qcheck-core"
# This line makes things transparent for people mixing preprocessors
# and normal dependencies
requires(-ppx_driver) = "qcheck-core"
requires(-ppx_driver,-custom_ppx) += "ppx_deriving"
ppxopt(-ppx_driver,-custom_ppx) = "ppx_deriving,package:ppx_deriving_qcheck"
jmid commented 2 years ago

Since that file is generated (by dune or ppxlib?) I then tried compiling the test with dune instead.

$ cat dune
(executable
  (name test)
   (libraries qcheck)
   (preprocess (pps ppx_deriving_qcheck)))
$ dune build ./test.exe
$ _build/default/test.exe 
(-352707198292687490, -2683955259025810335)
$ 

So is seems it is not a dependency to dune - but an artifact of the generated findlib/ocamlfind META file. It is unfortunate to have a mismatch between the dependencies required through ocamlfind vs dune... 😬 ... and so more an issue with either ppxlib or dune.

jmid commented 2 years ago

@pitag-ha Sorry, I missed your message while digging further... 😅 It seems it is an artificial META-file requirement which I don't hit when just using dune. Is it dune or ppxlib or some collaboration between them that generates the META file?

c-cube commented 2 years ago

replacing with (kind ppx_rewriter) in the deriving dune file, the dependency seems to vanish. The new meta is:

version = "0.2.0"
description = ""
requires(ppx_driver) = "ppxlib ppxlib.ast"
archive(ppx_driver,byte) = "ppx_deriving_qcheck.cma"
archive(ppx_driver,native) = "ppx_deriving_qcheck.cmxa"
plugin(ppx_driver,byte) = "ppx_deriving_qcheck.cma"
plugin(ppx_driver,native) = "ppx_deriving_qcheck.cmxs"
# This is what dune uses to find out the runtime dependencies of
# a preprocessor
ppx_runtime_deps = "qcheck-core"
# This line makes things transparent for people mixing preprocessors
# and normal dependencies
requires(-ppx_driver) = "qcheck-core"
ppx(-ppx_driver,-custom_ppx) = "./ppx.exe --as-ppx"
library_kind = "ppx_rewriter"
vch9 commented 2 years ago

Thank you all for your digging, (kind ppx_rewriter) solves the issue on my end as well :)

jmid commented 2 years ago

Thanks for helping with the investigation @pitag-ha and @c-cube!

So apparently this is a known dune issue: https://github.com/ocaml/dune/issues/1327 That issue also explains the technical reason for the dependency in utop and the plain top-level and has pointers, e.g., to others getting bitten by it.

They seem to recommend simply introducing a ppx_deriving dependency. For now @vch9 has gone with switching to a (kind ppx_rewriter) instead. I'm not sure I understand the consequences of doing so based on the description in dune issue#1327 🤔

Does that mean

vch9 commented 2 years ago

So apparently this is a known dune issue: ocaml/dune#1327 That issue also explains the technical reason for the dependency in utop and the plain top-level and has pointers, e.g., to others getting bitten by it.

Thank you for the pointer. Based on this issue, the right solution for the toplevel seems be be adding the dependency to ppx_deriving.

c-cube commented 2 years ago

I think it deserves a discussion, like #1327 suggests. Adding a dependency on ppx_deriving just so we can use derivers in the toplevel, is it worth it? (I would tend to think yes, btw).

jmid commented 2 years ago

I think it deserves a discussion, like #1327 suggests. Adding a dependency on ppx_deriving just so we can use derivers in the toplevel, is it worth it? (I would tend to think yes, btw).

I also think it is worth it. It is a nice tool for experimenting (I just did so, for overriding the default int generator some days ago). I can also imagine users getting confused if utop or ocaml usage will not work out-of-the-box if we leave out the dependency.

jmid commented 2 years ago

OK. We seem to agree - and @vch9 has again added the dependency in 1971e54 - so I'll close this issue.