Code updates and preliminary changes for Mirage compatibility

[paurkedal]

I am experimenting with running pgx under Mirage. I got it working, but with various patches to pgx. This PR covers the ones I think can be merged, but I can cherry pick and send them individually if you prefer:

• The first commit merely fixes some warnings under OCaml 4.08 related to ambiguous doc comments and reference to the now deprecated Pervasives module. About the latter, I avoided replacing it with Stdlib so we don't need to pull in the stdlib shim for backwards compatibility.
• The second commit upgrades to base64 >= 3.0.0.
• The third commit is the only essential one, as it eliminates dependency on the Unix from the API.

(I am proposing this on its own, but for the reference:

While Unix no longer is a direct dependency, it is pulled in along with base through ppx_jane. ppx_sexp_conv is harmless as it only uses sexplib0, so the remaining change I needed to make it work under Mirage, apart from the IO adapter module, was:

diff --git a/pgx/src/dune b/pgx/src/dune
index f6c5b85..3ed4f7c 100644
--- a/pgx/src/dune
+++ b/pgx/src/dune
@@ -3,4 +3,4 @@
   (wrapped false)
   (inline_tests (flags (-verbose)))
   (libraries ipaddr uuidm re sexplib0)
-  (preprocess (pps ppx_jane bisect_ppx -conditional)))
+  (preprocess (pps ppx_sexp_conv)))
diff --git a/pgx/src/error_response.ml b/pgx/src/error_response.ml
index 5cc89f6..aea15a6 100644
--- a/pgx/src/error_response.ml
+++ b/pgx/src/error_response.ml
@@ -27,6 +27,7 @@ let to_string ?(verbose=false) t =
     else [] in
   String.concat "\n" (msg :: field_info)
 
+(*
 let%test_module "to_string and should_print inline tests" =
   (module struct
     let info_msg =
@@ -67,3 +68,4 @@ let%test_module "to_string and should_print inline tests" =
         let msg = { info_msg with severity } in
         [%test_result: bool] ~expect:true (should_print msg ~verbose));
   end)
+*)
diff --git a/pgx_lwt/src/dune b/pgx_lwt/src/dune
index 65f0f15..944c2f6 100644
--- a/pgx_lwt/src/dune
+++ b/pgx_lwt/src/dune
@@ -2,4 +2,4 @@
   (public_name pgx_lwt)
   (wrapped false)
   (libraries lwt lwt.unix pgx)
-  (preprocess (pps bisect_ppx -conditional ppx_jane)))
+  (preprocess (pps ppx_sexp_conv)))

One thing was still not quite as it should be, my Pgx_mirage has the following signature:

module Make :
  functor (Random : Mirage_random.S) ->
  functor (Clock : Mirage_clock.MCLOCK) ->
  functor (Stack : sig include Mirage_stack.V4 val singleton : t end) ->
  Pgx.S with type 'a monad = 'a Lwt.t

That is, I need to bring in a singleton of the network stack. From the Mirage point of view, I think this should have been an argument to the open_connection function, though that would seem odd from the "normal" point of view.)

[brendanlong]

Thanks for this! We'll take a look soon. The only piece I'm worried about is:

The second commit upgrades to base64 >= 3.0.0.

Unfortunately base64 broke compatibility with anything that (transitively) links to Core_extended so we can't use the latest version, see: https://discuss.ocaml.org/t/what-to-do-about-files-both-define-module-base64/3426/3

I'll see if I can do this upgrade now though.

[brendanlong]

(Oh and for the record, I think we're fine with not using ppx_jane and just using ppx_sexp_conv).

[paurkedal]

I'll wait for your to look at base64, we can postpone it if it still turns out to be tricky.

Good we can drop the full ppx_jane, but what do we do about the inline test? Change it into a regular dune test? Or move it into a separate private library? I haven't used inline tests myself, so I'm qualified to make any recommendations here.

[brendanlong]

Does ppx_inline_test not work in Mirage?

I've been looking into the base64 update and I think we can do it relatively soon.

[paurkedal]

Not out of the box, at least. If I add ppx_inline_test to pgx/src/dune, it pulls in base which cause a lot of undefined references to C functions. Here are the first ones:

mirage build
config.exe: [WARNING] pkg-config solo5-bindings-virtio --variable=ld returned nothing, using ld
_build/main.native.o: In function `camlBase__Import0__entry':
~/.opam/4.08.1/.opam-switch/build/base.v0.12.2/_build/default/src/import0.ml:392: undefined reference to `Base_am_testing'
_build/main.native.o: In function `camlBase__Hash__fun_2813':
(.text+0x1725e8): undefined reference to `Base_internalhash_get_hash_value'
_build/main.native.o: In function `camlBase__Hash__fun_2815':
(.text+0x17260b): undefined reference to `Base_internalhash_fold_string'
_build/main.native.o: In function `camlBase__Hash__fun_2817':
(.text+0x17262b): undefined reference to `Base_internalhash_fold_float'
_build/main.native.o: In function `camlBase__Hash__fun_2819':
(.text+0x17264b): undefined reference to `Base_internalhash_fold_int64'
_build/main.native.o: In function `camlBase__Hash__fun_2821':
(.text+0x17266b): undefined reference to `Base_internalhash_fold_int'
_build/main.native.o: In function `camlBase__Hash__fun_2948':
~/.opam/4.08.1/.opam-switch/build/base.v0.12.2/_build/default/src/hash.ml:145: undefined reference to `Base_internalhash_fold_int64'
_build/main.native.o: In function `camlBase__Hash__fun_2946':
~/.opam/4.08.1/.opam-switch/build/base.v0.12.2/_build/default/src/hash.ml:146: undefined reference to `Base_internalhash_fold_int'
_build/main.native.o: In function `camlBase__Hash__fun_2950':
~/.opam/4.08.1/.opam-switch/build/base.v0.12.2/_build/default/src/hash.ml:147: undefined reference to `Base_internalhash_fold_float'
...

[brendanlong]

Hmm ok. I think I'd be fine with moving the inline tests to normal tests.

[brendanlong]

I cherry picked two of your changes in #58 and I'll merge those as soon as the build finishes.

[paurkedal]

Thanks!

[brendanlong]

Hm I'm not sure how long this is going to take us to get the base64 thing working. Base64 v3 is incompatible with Core v0.11, we can't use Core v0.12 because it's missing Async.Udp, and we can't use Core v0.13 because Async_ssl v0.13 isn't released.

So that's fun..

[paurkedal]

I see. Do you want to keep this open for when you can upgrade to the next core release, maybe change the title?

[paurkedal]

About the tests, do you have a preference for unit testing framework apart from inline tests, or just plain asserts? I can send that in an new PR.

[brendanlong]

For the tests, we would want them to use OUnit like we do with the existing tests. Or alternatively, convert everything to Alcotest and then write Alcotest tests (we will probably do this eventually but we don't have a lot of time to work on it).

And yes, I think making this work on Mirage is a good goal, I just don't know how long we'll be stuck dealing with base64's problems :( Maybe we should just vendor our own base64 library so we don't have to deal with this..

[paurkedal]

Good, I'll look into the test conversion soon.

I don't think the base64 upgrade is required for MirageOS compatibility, sorry if I made that impression. I was really putting too much into this PR, didn't think it would bite back. So, I'm okay with wait and see for now.

Corretion: I missed the part about cohttp using base64 > 3.

[brendanlong]

Ok, sounds great!

[hannesm]

FWIW, the inline tests will soon work with MirageOS according to ocaml/dune#897 (in the way that the dependency will be dropped by dune + ppx_inline_tests in release mode) :)

[brendanlong]

Thanks for the PR! We merged this in #62