search

mfp / ocaml-sqlexpr

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

Jbuilder and migrate-parsetree migration #27

Closed meadofpoetry closed 6 years ago

meadofpoetry commented 6 years ago

Ok, so I've replaced oasis with jbuilder and implemented ppx extension with migrate-parsetree and versioned ppx_tools. Tests implemented only for ppx since I haven't managed to build execs with camlp4 syntax and jbuilder. Package was split into three: sqlexpr, ppx_sqlexpr and pa_sqlexpr(camlp4 syntax). It has to be checked carefully since I'm neither very familiar with ppx nor with jbuilder. However I've tried it with a project of my own and it seems to work. Tests can be run with jbuilder runtest, docs can be generated with jbuilder build @doc or by odig

Also camlp4 is now called pa_sqlexpr, but lib's public name can be changed back in syntax/jbuild for the sake of backward compatibility.

mfp commented 6 years ago

On Fri, Sep 29, 2017 at 07:01:44AM -0700, Freyr666 wrote:

Ok, so I've replaced oasis with jbuilder and implemented ppx extension with migrate-parsetree and versioned ppx_tools. Tests implemented only for ppx since I haven't managed to build execs with camlp4 syntax and jbuilder. Package was split into three: sqlexpr, ppx_sqlexpr and pa_sqlexpr(camlp4 syntax). It needs to be checked carefully since I'm neither very familiar with ppx nor with jbuilder. However I've tried it with a project of my own and it seems to work. Tests can be run with jbuilder runtest, docs can be generated with jbuilder build @doc or by odig

Thanks!

I took a quick look and verified it does build & test correctly with 4.05. I'm getting more and more behind the times, because I'm now lacking knowledge of both ppx AND jbuilder :)

I've noticed that the META generated for pa_sqlexpr would link the extension instead of using it as a preprocessor. After some googling, it seems jbuilder offers no support out of the box for building/installing or using camlp4 extensions, so unless we can provide the META manually this would mean dropping camlp4 support.

I'm thinking about splitting this PR into 2 parts, one to migrate to migrate-parsetree (heh) and ppx_tools_versioned, and another to drop the oasis build system (but I'd keep the OMake one because I use it myself and jbuilder doesn't have much of a cross-compilation story atm.). The former would be merged right away, and the second could take some thinking time to decide whether it's time to drop camlp4 support.

-- Mauricio Fernández

meadofpoetry commented 6 years ago

I've noticed that the META generated for pa_sqlexpr would link the extension instead of using it as a preprocessor. After some googling, it seems jbuilder offers no support out of the box for building/installing or using camlp4 extensions,

Isn't js_of_ocaml-camlp4 [1] a proper camlp4 extension? Their jbuild file is not very different from mine except -linkall flag which I've missed.

[1] https://github.com/ocsigen/js_of_ocaml/blob/master/camlp4/pa_js/lib/jbuild

meadofpoetry commented 6 years ago

@mfp Ok, I've added META.pa_sqlexpr template as the first step. However I can't build an example using camlp4 extension. Excuse my ignorance but how can I build it? ocamlfind ocamlc -thread -package sqlexpr,pa_sqlexpr,camlp4 -syntax camlp4r -c t_sqlexpr_sqlite.ml do not work for some reason (neither it works with your old sqlexpr.syntax, which means I'm doing it wrong).

mfp commented 6 years ago

On Tue, Oct 03, 2017 at 09:48:47AM +0000, Freyr666 wrote:

@mfp Ok, I've added META.pa_sqlexpr template as the first step. However I can't build example using camlp4 extension. Excuse my ignorance but how can I build it? ocamlfind ocamlc -thread -package sqlexpr,sqlexpr.syntax,camlp4 -syntax camlp4r -c t_sqlexpr_sqlite.ml do not work for some reason.

Try with -syntax camlp4o (camlp4r is for the revised syntax).

-- Mauricio Fernández

meadofpoetry commented 6 years ago

@mfp

ocamlfind ocamlc -thread -package sqlexpr,sqlexpr.syntax,camlp4 -syntax camlp4o -c t_sqlexpr_sqlite.ml

File "t_sqlexpr_sqlite.ml", line 94, characters 32-36:
Parse error: ";" or ":" or ":>" or ")" expected after [expr] (in [expr])
File "t_sqlexpr_sqlite.ml", line 1:
Error: Error while running external preprocessor
Command line: camlp4 '-I' '/home/freyr/.opam/main/lib/ocaml/camlp4' '-I' '/home/freyr/.opam/main/lib/estring' '-I' '/home/freyr/.opam/main/lib/sqlexpr' '-parser' 'o' '-parser' 'op' '-printer' 'p' 'estring.cma' 'sqlexpr_syntax.cma'  't_sqlexpr_sqlite.ml' > /tmp/ocamlpp4d5ee9

Edit: Ah, It was due to lwt.syntax. Those error msgs are really awkward.

Ok, it seems that now pa_sqlexpr works. I will add tests and return OMakefiles a little bit later (if you are not against jbuilder in general, I tried to make ppx_driver work with oasis but didn't manage to make it work)

mfp commented 6 years ago

On Tue, Oct 03, 2017 at 10:37:47AM +0000, Freyr666 wrote:

@mfp

ocamlfind ocamlc -thread -package sqlexpr,sqlexpr.syntax,camlp4 -syntax camlp4o -c t_sqlexpr_sqlite.ml

You also need the lwt.syntax and oUnit packages.

I assume you're testing against an older install though? I just tried a fresh one with jbuilder install and the syntax extension is installed as pa_sqlexpr. This works for me:

ocamlfind ocamlc -verbose -thread -package sqlexpr,pa_sqlexpr,lwt.syntax,oUnit -syntax camlp4o -o t_sqlexpr_sqlite t_sqlexpr_sqlite.ml -linkpkg

so it does look like you managed to keep the syntax extension working :)

(This will require a major version bump though, because of the package name changes.)

-- Mauricio Fernández

meadofpoetry commented 6 years ago

@mfp Ok, so I've added omake files, although the release: section in the root OMakefile should be fixed since it uses oasis and some strange git commands. Could you check OMakefiles and readme, and bump the package version?

mfp commented 6 years ago

I apologize for the delay.

I've reworked the packaging and small parts of ppx_sqlexpr.ml to preserve compatibility with OCaml 4.02.3 and retain the current package names for the syntax extensions sqlexpr.ppx and sqlexpr.syntax. The initial plan for the latter is to drop the camlp4 depopts at some point and move it eventually to a separate package, with an install-time deprecation warning in the meantime.

I've pushed this to https://github.com/mfp/ocaml-sqlexpr/tree/Freyr666-migrate-parsetree

I've checked manually build&install with jbuilder and use of both extensions via ocamlfind, but not direct in-tree integration with jbuilder (did I say I don't do jbuilder yet?), so testing of that would be appreciated. The sqlexpr.ppx and sqlexpr.omp package definitions are modeled after lwt.ppx and lwt.omp -- to be honest I don't know the reasons to do it that way, but if it works for lwt it should work here too...

meadofpoetry commented 6 years ago

@mfp

As far as I see you've also merged ppx to the main package, do you think it is necessary? I thought that nowadays ocaml community is firmly against depopts and stands for syntax extensions provided as separate packages. But in any case it seems it does not work for me so far

Error: No implementations provided for the following modules:
         Re referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Re_pcre referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Re_perl referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Unix referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Ppx_sqlexpr)

I will try to investigate why it refuses to work properly.

It seems like a packaging issue. Separated ppx works fine, and I don't know how to make a single package with ppx and sqlexpr with jbuilder, I haven't seen such examples so far.

rgrinberg commented 6 years ago

I do not think it's possible to combine a ppx/normal library into a single package either. I'd also recommend to follow the guidelines of making the ppx separate.

If you'd like to retain old findlib names, I recommend that you use a scheme like cohttp: https://github.com/mirage/ocaml-cohttp/blob/master/META.cohttp.template

This will create compatibility aliases for the old package names. Note that people will still have to install these sub packages separately.

mfp commented 6 years ago

On Mon, Oct 30, 2017 at 07:11:41AM +0000, Freyr666 wrote:

@mfp

As far as I see you've also merged ppx to main package, do you think it is necessary? I thought that nowadays ocaml community is firmly against depopts and stands for syntax extensions provided as separate packages.

I took the deps from the ppx package and made them hard dependencies of the sqlexpr package (now including sqlexpr.ppx) because the PPX extension is quite integral to (you could say the very point of) Sqlexpr...

This differs from the case of sqlexpr.syntax, which is going the deprecation route, and is built conditionally depending on the status of the camlp4 and estring depopts. It is kept as sqlexpr.syntax (and not yet split into a separate package) for the sake of backwards compatibility during a transition period (I'm adding an install-time warning in the .opam to warn about the future rename).

But in any case it seems it does not work for me so far

Error: No implementations provided for the following modules:
         Re referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Re_pcre referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Re_perl referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Unix referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Ppx_sqlexpr)

This is not with in-tree jbuilder integration, but after installation with opam/jbuilder install, right?

How are you compiling when you get these errors? The sample build instructions we give in README.md are working for me:

ocamlfind ocamlopt -package sqlexpr.ppx -linkpkg -thread -o sqlexpr_ppx tests/ppx/example.ml

-- Mauricio Fernández

mfp commented 6 years ago

On Mon, Oct 30, 2017 at 01:00:02PM +0000, Rudi Grinberg wrote:

I do not think it's possible to combine a ppx/normal library into a single package either. I'd also recommend to follow the guidelines of making the ppx separate.

Doesn't Lwt do exactly that though for lwt.ppx? Lwt does quite a lot of custom massaging of the install procedure though, so if that's what it takes to achieve it, the way you point appears much more attractive...

If you'd like to retain old findlib names, I recommend that you use a scheme like cohttp: https://github.com/mirage/ocaml-cohttp/blob/master/META.cohttp.template

This will create compatibility aliases for the old package names. Note that people will still have to install these sub packages separately.

The compatibility aliases would do if we can make sqlexpr (opam package, not findlib) depend on ppx_sqlexpr, and there's no reason it cannot, is there?

That way we make sure opam install sqlexpr gets you the whole "experience" (because writing Sqlexpr directives by hand is not something you really want to do).

-- Mauricio Fernández

meadofpoetry commented 6 years ago

@mfp >This is not with in-tree jbuilder integration, but after installation with opam/jbuilder install, right?

Yes, try to jbuilder build src/example.exe directly, it will yield the same:

File "_none_", line 1:
Error: No implementations provided for the following modules:
         Re referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Re_pcre referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Re_perl referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Sqlexpr_parser)
         Unix referenced from /home/freyr/.opam/main/lib/sqlexpr/ppx/ppx_sqlexpr.cmxa(Ppx_sqlexpr)

Doesn't Lwt do exactly that though for lwt.ppx?

I'm not sure, but they have separate opam file for ppx.

I suspect you should just add META.sqlexpr template with "ppx" stuff, just like lwt people did (or as I did with sqlexpr.syntax)

https://github.com/ocsigen/lwt/blob/master/META.lwt

mfp commented 6 years ago

So, I reverted my attempt to use subpackages, and used instead an ocamlfind alias as suggested by @rgrinberg, for ppx only, though: I haven't found a way to provide full backwards compatibility (I wanted to have sqlexpr depend on the 2 other packages for a little while and provide deprecation warnings), because the sqlexpr.syntax alias does not work -- ocamlfind complains there's "no package is selected specifying a preprocessor as required for -syntax".

So instead it seems I'll have to settle with simultaneous releases of ppx_sqlexpr and pa_sqlexpr (but not newer sqlexpr with the new package names until a later time), as well as a 0.8.1 release with install-time notices to switch to them.

rgrinberg commented 6 years ago

@mfp I think that people tweaking their build systems a bit is reasonable. As long as source compatibility is maintained. There are benefits to this as well of course, such as users who only use one syntax extension or (none at all) will not install things they don't use and can specify their opam dependencies accurately.

meadofpoetry commented 6 years ago

So what's the status of this PR?

rgrinberg commented 6 years ago

ping @mfp

meadofpoetry commented 6 years ago

BTW if you still want to merge everything into one package I think you could possibly take a look at the recent bitstring update, they've managed to meld bitstring and ppx_bitstring into one package:

https://github.com/xguerin/bitstring

rgrinberg commented 6 years ago

Imo that's not an example to be followed. Maintaining the ppx separate has many advantages

On Thu, Jan 11, 2018, 2:56 PM Freyr666 notifications@github.com wrote:

BTW if you still want to merge everything into one package I think you could possibly take a look at the recent bitstring's jbuilder update, they've managed to meld bitstring and ppx_bitstring into one package:

https://github.com/xguerin/bitstring

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mfp/ocaml-sqlexpr/pull/27#issuecomment-356843534, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIe-1UlGvju8KBrlh618S5rc0rRV88Pks5tJbCsgaJpZM4Pot-b .

jvillard commented 6 years ago

Gentle ping and registration of more interest in this PR.

rgrinberg commented 6 years ago

Ping @mfp. This is biting users: https://www.reddit.com/r/ocaml/comments/8fcful/jbuilder_help_needed/ :)

mfp commented 6 years ago

Better is the enemy of good... I'd revisited this periodically, but hadn't realized that, by trying to follow Lwt's lead and give excellent backwards compatibility to old package (sqlexpr.*) users, I was doing a disservice to jbuilder/dune AND 4.06 users -- I hadn't realized sqlexpr was uninstallable/unusable for them.

I decided to give this a last attempt or give up and merge as-is, ignoring backward (in)compatibility. The struggle seems to have borne fruit. AFAICS I've got both sqlexpr.ppx and sqlexpr.syntaxaliases working by default, plus 4.06 installability. Tested install through opam on 4.05 and 4.06, in-dir build with jbuilder, in-dir build of example with jbuilder, use of syntax extension via ocamlfind with 4.05... It's on branch https://github.com/mfp/ocaml-sqlexpr/tree/Freyr666-migrate-parsetree which subsumes this PR. I'd like to merge and release as 0.9.0 in the next few days (no need to change major version since full compat is preserved), followed some time later with a 1.0.0 release that drops the aliases.

mfp commented 6 years ago

I've merged a branch that subsumes this and fixes a number of packaging issues as well as problems with newer Lwt and other bugs.

I've tested this as exhaustively as I could by:

It is not installable on 4.07 because some ppx_sqlexpr dependencies cannot be installed there yet. I expect it to work OK on 4.07 when that is solved.

In every case, I've tested against the new and recommended package name (ppx_sqlexpr, pa_sqlexpr when installable) and the compatibility alias (sqlexpr.ppx, sqlexpr.syntax).

mfp commented 6 years ago

0.9.0 release on the way to OPAM repos https://github.com/ocaml/opam-repository/pull/11972