Rewrite Idl to unifiy almost all GenClient and GenServer under a functor #117

mseri · 2018-06-17T10:34:38Z

Do not merge yet. This is a work in progress and I am posting it here for discussion.

I have now found a structure that compiles and allows to unify GenClient and GenServer for the Result.result, lwt and async interfaces. This does not yet work with the Exn version of the Client and Server.

Also some more cleanup needs to be done, e.g. pass the RPCMONAD in a Make functor that generates all the modules and unify the interfaces and signatures.

Signed-off-by: Marcello Seri <[email protected]>

mseri · 2018-06-17T12:57:17Z

The latest version should work, at least passes all the tests on my machine. If you review the code you will see that the current state requires some ugly returns and Obj.magic (these could be hidden in a safe interface, like for lwt and async, though).

There is definitely more work needed, but before moving on I'd like to get some feedback and ideas on how to restructure it further to simplify the issues mentioned above (they are clear if you look at the changes needed for the examples to work).

Signed-off-by: Marcello Seri <[email protected]>

mseri · 2018-06-17T15:15:58Z

I have an idea to deal with it using GADTS but it makes writing the RPCMONAD a bit more involute, on the other hand it should become much simpler to use.
Naming of some of the new things needs to change, it is kind of confusing and obscure at the moment

gaborigloi · 2018-06-18T10:36:05Z

I'd be interested in that idea using GADTS, it's great if that makes it easier to use.
Maybe we could implement the exn functors by passing in the function that handles the ok and error cases, and the result type.

jonludlam · 2018-06-18T11:04:04Z

src/async/rpc_async.ml

+module M = struct
+  include AsyncIdl.ImplM
+  (* TODO: can we avoid Obj.magic here? *)
+  let deferred x : 'a Async.Deferred.t = Obj.magic (AsyncM.unbox x)


Yeah, I think this was the line I didn't cross when trying to do this...! Not that I'm saying it's a showstopper because unifying these things certainly reduces a large amount of copy-pasted code.

I think this can be avoided, I have only to figure out what is the best way

jonludlam · 2018-06-18T11:06:24Z

tests/rpc/client_lwt_new.ml

@@ -125,7 +125,7 @@ let main () =
  Server.rpc4 ImplM.rpc4;

  let funcs = Server.implementation in
-  let rpc = rpc (Rpc_lwt.server funcs) in
+  let rpc r = rpc (Rpc_lwt.server funcs |> Obj.magic) r |> Obj.magic in


Although leaking Obj.magic calls into the code that end-users of the library write is a lot more worrying.

I thin this should be avoidable as well. One of these calls is actually Rpc_lwt.M.lwt but I realised it only later

Anyways I agree with you that, if this requires leaking Obj.magic, then is the wrong approach.

Signed-off-by: Marcello Seri <[email protected]>

mseri · 2018-06-18T15:26:26Z

The latest version does not require any Obj.magic at all. The only annoyance is that to apply a type-level symplification, the Identity monad needs to be copy/pasted in the libraries that need to generate cliets and servers using it.

From the test and examples updates it seems clear that a new combinator is needed in the ErrM, currently I am using let (>>>=) x f = T.(x |> unbox |> run |> f) but there is scope for improvement.

Naming has improved a lot in my opinion, although there is quite a lot of room for improvement.
I don't have (yet) a decent way to support the autogeneration of the Exn versions using the same functor.

I guess the next steps are:

cleaning up the naming scheme
figure out how to deal with the generation of the Exn modules

…yle if we want to be pedantic Signed-off-by: Marcello Seri <[email protected]>

Signed-off-by: Marcello Seri <[email protected]>

In fact, it did not ocamlformat the following - ppx_deriving_rpc (because it uses cppo) - examples/example3_idl (reported bug) - tests/libs/json (reported bug) Signed-off-by: Marcello Seri <[email protected]>

gaborigloi

Thanks so much, looks great! Just some comments about unused things, I'll do a proper review soon, after the Gen...Exn behaviour is finalized.

gaborigloi · 2018-06-22T08:33:11Z

src/lib/idl.ml

+
+[@@@warning "-32-34"]
+
+module type RPCTRANSFORMER = functor (M : MONAD) -> sig


This seems to be unused

Yes, It is kind of legacy you are right. I may resuscitate it to simplify further the interface of Rpc_lwt and Rpc_async

That would be useful, because we use the async one in xapi-storage-script.

gaborigloi · 2018-06-22T08:35:03Z

src/lib/idl.ml

+
+    val return : 'a -> 'a box
+
+    val get : 'a box -> 'a M.t


unbox and get seem to be the same - but if we want more synonyms for readability that's fine by me

Well spotted, with the last interface put and get are the same as box and unbox. I like the former more, what do you think?

I agree, they just sound better :)

Signed-off-by: Marcello Seri <[email protected]>

mseri · 2018-06-22T11:55:39Z

@gaborigloi I have updated the interface with the make_strict (not sure it is a good name)

mseri · 2018-06-25T14:44:09Z

Merged to include the recent fixes and resolve the merge conflicts (doing a rebase was too painful and error prone)

gaborigloi · 2018-08-07T08:24:57Z

@jonludlam Should we just merge & test it on a branch, and then refine it later (by removing e.g. the legacy exn stuff that we don't need)?
The best way of reviewing this is to actually start using it IMO on a dedicated branch.

gaborigloi · 2018-08-08T14:59:58Z

We've agreed to merge this.

Signed-off-by: Marcello Seri <[email protected]>

mseri · 2018-08-31T13:36:07Z

After reading https://discuss.ocaml.org/t/reducing-mirageos-image-size-using-ocamlclean/ I double checked the dependencies, and updated the jbuild file of the ppx rewriter. It does not affect the size of the executables though, so it should not have anything to do with the discussion there

mseri · 2018-08-31T13:54:15Z

I have also moved to ppx_tools_versioned that should allow us to drop the need of ppxfind and cppo (needs testing with different compilers though, so we'll need to wait for travis to know better)

Signed-off-by: Marcello Seri <[email protected]>

mseri · 2018-08-31T14:29:33Z

I took the last thing back. Unless ppx_deriving is converted to omp, it is not possible to drop cppo and the use of ppxfind

mseri · 2018-10-24T13:29:16Z

Ping, any progress on this?

gaborigloi · 2018-10-24T13:58:16Z

We wanted to just merge it and create a feature branch with @jonludlam to test it.
Can we merge it now @jonludlam ?

src/lib/idl: first level reorganisation for code deduplication

7e62462

Signed-off-by: Marcello Seri <[email protected]>

mseri requested review from gaborigloi and jonludlam June 17, 2018 10:35

mseri added 12 commits June 17, 2018 12:57

idl: further cleanup

5cf573f

Signed-off-by: Marcello Seri <[email protected]>

Use new idl functor with lwt

4706101

Signed-off-by: Marcello Seri <[email protected]>

Use new idl functor with async

f436820

Signed-off-by: Marcello Seri <[email protected]>

idl: further reduce duplication

10b3a5d

Signed-off-by: Marcello Seri <[email protected]>

lwt: update to latest idl

c3f664a

Signed-off-by: Marcello Seri <[email protected]>

async: update to latest idl

6054280

Signed-off-by: Marcello Seri <[email protected]>

idl: generate also implementations monad

17082ab

Signed-off-by: Marcello Seri <[email protected]>

idl: update tests using Legacy

88253ba

Signed-off-by: Marcello Seri <[email protected]>

lwt: make interface backward compatible

d310b60

Signed-off-by: Marcello Seri <[email protected]>

async: make interface backward compatible

0c24c85

Signed-off-by: Marcello Seri <[email protected]>

example: this makes it clear that some more work is needed

997b466

Signed-off-by: Marcello Seri <[email protected]>

idl: add some inline todos

7560aff

Signed-off-by: Marcello Seri <[email protected]>

update rpc tests: this makes it clear that some more work is needed

ae1dc15

Signed-off-by: Marcello Seri <[email protected]>

jonludlam reviewed Jun 18, 2018

View reviewed changes

mseri added 7 commits June 18, 2018 13:52

idl: use GADT based implementation

d2f5d00

Signed-off-by: Marcello Seri <[email protected]>

idl: major cleanup and refactoring

d728fc1

Signed-off-by: Marcello Seri <[email protected]>

idl: further mli cleanup

5daa991

Signed-off-by: Marcello Seri <[email protected]>

lwt: port to new idl

961beba

Signed-off-by: Marcello Seri <[email protected]>

async: port to new idl

2a8c305

Signed-off-by: Marcello Seri <[email protected]>

tests: update

9860187

Signed-off-by: Marcello Seri <[email protected]>

example: update with new syntax

331b57f

Signed-off-by: Marcello Seri <[email protected]>

example: even in this trivial case, the code should have a monadic st…

da3a112

…yle if we want to be pedantic Signed-off-by: Marcello Seri <[email protected]>

mseri added 3 commits June 21, 2018 23:53

rpcty: remove unnecessary use of cppo

4c5f1c4

Signed-off-by: Marcello Seri <[email protected]>

idl: update generated server to be fully monadic

75264ab

Signed-off-by: Marcello Seri <[email protected]>

ocamlformat all the code

26dd289

In fact, it did not ocamlformat the following - ppx_deriving_rpc (because it uses cppo) - examples/example3_idl (reported bug) - tests/libs/json (reported bug) Signed-off-by: Marcello Seri <[email protected]>

mseri force-pushed the idl-reorg branch from 598955e to 26dd289 Compare June 21, 2018 22:03

gaborigloi reviewed Jun 22, 2018

View reviewed changes

mseri added 4 commits June 22, 2018 13:53

idl: remove redundant functions

a16c8a0

Signed-off-by: Marcello Seri <[email protected]>

idl: add strict mode, to fail on rpc error

6206ac8

Signed-off-by: Marcello Seri <[email protected]>

async: update signature

137fc63

Signed-off-by: Marcello Seri <[email protected]>

lwt: update signature

ceb4ec3

Signed-off-by: Marcello Seri <[email protected]>

mseri changed the title ~~[WIP] rewrite idl to unifiy GenClient and GenServer under a functor~~ Rewrite Idl to unifiy almost all GenClient and GenServer under a functor Jun 22, 2018

Merge remote-tracking branch 'origin/master' into idl-reorg

7207d52

jonludlam mentioned this pull request Jul 12, 2018

Optional named params fix #126

Merged

Merge branch 'master' into idl-reorg

1c73940

gaborigloi mentioned this pull request Aug 7, 2018

Add rpclib ocaml/opam-repository#12441

Merged

mseri added 2 commits August 31, 2018 15:26

ppx: update jbuild file to remove unnecessary dependencies

f158c78

Signed-off-by: Marcello Seri <[email protected]>

Merge remote-tracking branch 'origin/master' into idl-reorg

1e7ca14

mseri force-pushed the idl-reorg branch from e7cb075 to 292592c Compare August 31, 2018 13:51

mseri force-pushed the idl-reorg branch from 292592c to 86a44b0 Compare August 31, 2018 13:54

ppx: also remove unnecessary use of ppx_deriving.runtime

f07e9d4

Signed-off-by: Marcello Seri <[email protected]>

mseri force-pushed the idl-reorg branch from c8d691d to f07e9d4 Compare August 31, 2018 14:28

gaborigloi merged commit edcbace into mirage:master Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Idl to unifiy almost all GenClient and GenServer under a functor #117

Rewrite Idl to unifiy almost all GenClient and GenServer under a functor #117

mseri commented Jun 17, 2018 •

edited

Loading

mseri commented Jun 17, 2018

mseri commented Jun 17, 2018 •

edited

Loading

gaborigloi commented Jun 18, 2018

jonludlam Jun 18, 2018

mseri Jun 18, 2018

jonludlam Jun 18, 2018

mseri Jun 18, 2018

mseri Jun 18, 2018

mseri commented Jun 18, 2018

gaborigloi left a comment •

edited

Loading

gaborigloi Jun 22, 2018

mseri Jun 22, 2018

gaborigloi Jun 22, 2018

gaborigloi Jun 22, 2018

mseri Jun 22, 2018 •

edited

Loading

gaborigloi Jun 22, 2018

mseri commented Jun 22, 2018

mseri commented Jun 25, 2018

gaborigloi commented Aug 7, 2018 •

edited

Loading

gaborigloi commented Aug 8, 2018

mseri commented Aug 31, 2018 •

edited

Loading

mseri commented Aug 31, 2018 •

edited

Loading

mseri commented Aug 31, 2018

mseri commented Oct 24, 2018

gaborigloi commented Oct 24, 2018 •

edited

Loading


		[@@@warning "-32-34"]

		module type RPCTRANSFORMER = functor (M : MONAD) -> sig

Rewrite Idl to unifiy almost all GenClient and GenServer under a functor #117

Rewrite Idl to unifiy almost all GenClient and GenServer under a functor #117

Conversation

mseri commented Jun 17, 2018 • edited Loading

mseri commented Jun 17, 2018

mseri commented Jun 17, 2018 • edited Loading

gaborigloi commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri commented Jun 18, 2018

gaborigloi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri commented Jun 22, 2018

mseri commented Jun 25, 2018

gaborigloi commented Aug 7, 2018 • edited Loading

gaborigloi commented Aug 8, 2018

mseri commented Aug 31, 2018 • edited Loading

mseri commented Aug 31, 2018 • edited Loading

mseri commented Aug 31, 2018

mseri commented Oct 24, 2018

gaborigloi commented Oct 24, 2018 • edited Loading

mseri commented Jun 17, 2018 •

edited

Loading

mseri commented Jun 17, 2018 •

edited

Loading

gaborigloi left a comment •

edited

Loading

mseri Jun 22, 2018 •

edited

Loading

gaborigloi commented Aug 7, 2018 •

edited

Loading

mseri commented Aug 31, 2018 •

edited

Loading

mseri commented Aug 31, 2018 •

edited

Loading

gaborigloi commented Oct 24, 2018 •

edited

Loading