mfp / ocaml-sqlexpr

Minimalistic syntax extension for type-safe, convenient execution of SQL statements.
Other
86 stars 17 forks source link

Convert internal code to use ppx #21

Closed j0sh closed 7 years ago

j0sh commented 7 years ago

Based off https://github.com/c-cube/ocaml-sqlexpr/commit/00bb21065077620a44a79c6658c1b3ab91df57ed, updated for current master. Migrates the internal code to lwt's ppx, no external API changes. This should allow us to make camlp4 an optional dependency (eg, only build the syntax extension if camlp4 is already installed), but not sure how to specify that with oasis.

Couple things to note that are mostly unrelated to this patch:

Not sure how to build with OMakefile (I go the oasis setup; make route), but had to manually add this line to myocamlbuild.ml: let () = Ocamlbuild_plugin.dispatch (fun hook -> Ocamlbuild_cppo.dispatcher hook) Should this be documented, or is there a better way?

After generating setup.ml with oasis 0.4.7 (latest release), myocamlbuild.ml doesn't seem to compile anymore. Something about the Ocamlbuild_cppo module being unbound, but haven't been able to figure out whether its changed behavior or a bona fide bug. Because of that, oasis setup should be run with oasis 0.4.6 or earlier for now.

mfp commented 7 years ago

On Sat, Aug 27, 2016 at 01:18:56AM -0700, Josh Allmann wrote:

Based off https://github.com/c-cube/ocaml-sqlexpr/commit/00bb21065077620a44a79c6658c1b3ab91df57ed, updated for current master. Migrates the internal code to lwt's ppx, no external API changes. This should allow us to make camlp4 an optional dependency (eg, only build the syntax extension if camlp4 is already installed), but not sure how to specify that with oasis.

AFAICS that's done using https://ocaml.org/learn/tutorials/setting_up_with_oasis.html#Flagsandconditionalcompilation and flags in "opam" like https://github.com/ocaml/opam-repository/blob/master/packages/lwt/lwt.2.5.2/opam#L15

Couple things to note that are mostly unrelated to this patch:

Not sure how to build with OMakefile (I go the oasis setup; make route), but had to manually add this line to myocamlbuild.ml: let () = Ocamlbuild_plugin.dispatch (fun hook -> Ocamlbuild_cppo.dispatcher hook) Should this be documented, or is there a better way?

The OASIS build system is not in the repository because I'd rather not pollute it with auto-generated files; it is generated by the "release" target of the OMakefile when I create the release tarball, which adds that line to myocamlbuild.ml. I could add a trivial "generate-oasis.sh" (that'd just run oasis and append the Ocamlbuild_cppo line to myocamlbuild.ml).

You can use omake to build from the dev repos out of the box (e.g. "omake -j9"; the tests are built & run with "omake test" ). Note that there is no install target, though.

After generating setup.ml with oasis 0.4.7 (latest release), myocamlbuild.ml doesn't seem to compile anymore. Something about the Ocamlbuild_cppo module being unbound, but haven't been able to figure out whether its changed behavior or a bona fide bug. Because of that, oasis setup should be run with oasis 0.4.6 or earlier for now.

It seems to be related to this

https://forge.ocamlcore.org/tracker/index.php?func=detail&aid=1659&group_id=54&atid=291

and I'd expect it to be fixed in oasis 0.4.8 fairly soon since it affects relatively high-profile things like nocrypto.

Mauricio Fernández

j0sh commented 7 years ago

AFAICS that's done using https://ocaml.org/learn/tutorials/setting_up_with_oasis.html#Flagsandconditionalcompilation and flags in "opam" like https://github.com/ocaml/opam-repository/blob/master/packages/lwt/lwt.2.5.2/opam#L15

Thanks for the pointers. Added another patch to this PR making camlp4 optional: https://github.com/mfp/ocaml-sqlexpr/pull/21/commits/51c77634400fbf72b2fef0e164de676d246079aa

c-cube commented 7 years ago

So, hum, what's the status on this? :-)

j0sh commented 7 years ago

Added two more commits: OMake support for this PR, and opam for support OCaml <= 4.04.1.

OMake: Not sure how to exclude the syntax extension from the build. For now it assumes both camlp4/estring are installed.

4.04: Tests pass on both 4.04.0 and 4.04.1. Luckily there were no changes to the AST for 4.04 affecting sqlexpr, as far as I can tell. The opam change doesn't strictly belong in this PR, so let me know if you'd prefer it separate.

Mauricio, is there anything else in this PR that should be done or cleaned up before merging?

mfp commented 7 years ago

Looks good 'as is' j0sh :) I've reviewed it again summarily, built and tested against 4.02.3, 4.04 and 4.05.0+beta2; everything OK. I'm relaxing the ocaml-version constraint to <= 4.05.0 and releasing in the next few days.

Thanks a lot!