Refactors errors and split signatures into their own opam packages by samoht · Pull Request #743 · mirage/mirage

samoht · 2016-12-20T11:04:45Z

Following the discussions we had with @talex5, @yallop and @hannesm last week, I implemented the new error scheme using private rows. After finding 2 bugs in the compiler, I finally managed to get something working. Here is the full set of patches:

Adapt to the new error scheme + use the new split sigs

Fix pre-existing errors

Add new constraints

ocaml-dns-forward: Add a constraint for the version of mirage-flow to use by samoht · Pull Request #50 · djs55/ocaml-dns-forward · GitHub

hannesm · 2016-12-20T11:52:14Z

I got the same warning, also think it is harmless (maybe just put -34 in the _tags file?).

This PR looks good to me! Thanks!

hannesm · 2016-12-20T12:02:43Z

-  | `Invalid_console str -> pf pp "invalid console %s" str
-
-let unspecified pp m = pf pp "unspecified error - %s" m
+let pp_device_error pp = function


while you're at it, maybe drop the pp? Mirage_pp.pp_device_error vs Mirage_pp.device_error..

Yes, good point.

I'd prefer the pp functions to follow the convention if possible, as they're much more recognisable when cross-referenced in documentation etc. Modules can be opened...

The _pp.pp_ is indeed pretty annoying. Two options:

I can move the function to Mirage_runtime

I can just drop the pp_ prefix, and tell people to not open Mirage_pp.

I tend towards the later, but both options seems ok.

I prefer the latter as well.

avsm · 2016-12-20T12:04:11Z

@@ -1,79 +1,47 @@
-open Result
+open V1
 let pf pp = Format.fprintf pp


We could just put a dependency on Fmt here, as it defines the same function.

yup, this needs some cleanup.

avsm · 2016-12-20T12:05:56Z

    {e Release %%VERSION%% } *)

-(** {1 Abstract devices}
+type 'a pp = Format.formatter -> 'a -> unit


and likewise here, this is Fmt.t -- while we are trying to minimise dependencies here, Fmt is standalone and has a number of other useful functions to sensibly compose formatters. I'm not against this patch either though, just a suggestion.

Today mirage-types does not have any dependency.

I do appreciate mirage-types not having any dependency (since we require 4.03+, result must be present in any case)

Fine by me.

One thing that would help beginners (as a documentation pass) is some tips on where to find functions that use the defined types -- for example, a pointer to Ptime for the time functions, Fmt for the pretty printers, Rresult for result combinators. This can be be done later once the code compiles :)

avsm · 2016-12-20T12:08:10Z

# File "lib_runtime/mirage_pp.mli", line 3, characters 42-55:
# Error: Unbound module Console
# Command exited with code 2.

samoht · 2016-12-20T12:12:29Z

@avsm all the reverse builds are broken because I haven't done the work yet :-) I'm doing that next.

samoht · 2016-12-20T20:13:09Z

Finally after discussing with @yomimono and @avsm I am going to the full ride: split up V1 into pieces, and move the error/pretty-printers into the sub-packages instead of having to deal centrally with all the boiler-plate.

This is a rather large change, but I suspect that it's possible to do it in a few days (and I'm very motivated to do it :p) so I started to do here (it's not strictly speaking part of this PR, but this is somehow related).

So far I have:

created https://github.com/mirage/mirage-device and put the DEVICE signature there
created https://github.com/mirage/mirage-random and put the RANDOM signature there
created https://github.com/mirage/mirage-time and put the SLEEP, MCLOCK and PCLOCK signatures there. 2 packages here: mirage-time and mirage-time-lwt for the previous V1_LWT.TIME (now called Mirage_type_lwt.SLEEP).
refactored mirage-flow to use topkg and to define 3 subpackages (mirage-flow, mirage-flow-lwt and mirage-flow-unix): modernize the build, split into 3 packages + adapt to the new error scheme mirage-flow#25 This is a rather big change, but the tests still pass so I am sure that's fine

I'll move out the other signatures packages tomorrow, and I hope to have something mergeable before the end of the week.

dbuenzli · 2016-12-20T20:29:09Z

created https://github.com/mirage/mirage-time and put the SLEEP

While SLEEP and clocks can be related (e.g. wait for a clock interrupt to continue), it seems to me that the current SLEEP, being tied to cooperative concurrency has nothing to do in there.

samoht · 2016-12-20T20:31:53Z

@dbuenzli any suggestion where should I should put it? mirage-sleep?

dbuenzli · 2016-12-20T20:33:48Z

@dbuenzli any suggestion where should I should put it? mirage-sleep?

Wherever your run loop abstraction is defined. Also mirage-time could be renamed to mirage-clock

samoht · 2016-12-20T20:52:26Z

Wherever your run loop abstraction is defined

It's already duplicated on every platform and tests:

Which IMHO seems to indicate that we have to either remove it (and assume an ambient sleep function) or have a central place to define this helper and make people use it.

talex5 · 2016-12-20T20:59:48Z

I often want my sleeps to depend on a clock, rather than on the run loop, for unit tests (e.g. mirage-clock-test) . It seems reasonable that a clock might want to offer a way to get a notification at a certain time, or after some period of time has elapsed according to that clock.

samoht · 2016-12-20T21:01:36Z

@MagnusS just pointed me to https://github.com/MagnusS/mirage-event-clock/blob/master/lib/event_clock.ml#L74 so we have use-cases where sleep is not an ambient function.

dbuenzli · 2016-12-20T21:23:48Z

I often want my sleeps to depend on a clock, rather than on the run loop, for unit tests (e.g. mirage-clock-test) .

It seems you rather want to configure which clock you use for your run loop.

It seems reasonable that a clock might want to offer a way to get a notification at a certain time, or after some period of time has elapsed according to that clock.

While a clock could provide you this under the form of interrupts, you wouldn't want cooperative programs to use this directly. Rather a run loop would use this with a priority queue to implement a cooperative sleep.

Drup · 2016-12-20T21:50:48Z

@samoht I feel like this would be simpler if you had all the packages in the same repository (still as separate packages).

hannesm · 2016-12-20T23:35:22Z

as discussed several times, I'd be happy when the whole mirage-os-shim would just be there, no need to functorise over basic OS functionality which should be there exactly once (or is anyone seriously interested in running two clocks at different speed in the same unikernel?) -- and I still think if you use time or clock in your tests, you made a mistake (see for example the thread on arp - if you make time explicit to your pure protocol implementation (which you should), there's no need to rely on a concrete Time.sleep_ns in your tests).

but I'm too tired to discuss this again.

samoht · 2016-12-21T00:10:39Z

Let's say that this won't be resolved for Mirage3 :p

My current goal is just to split the types in separate repos, while keeping the signatures as similar as possible as what we have today in V1. I think this is pretty non-controversial for now :-) We can revisit that in 6 months.

avsm · 2016-12-21T09:48:49Z

I agree with splitting up the types into separate OPAM packages, but is there any reason not to keep them in the same Git repo with multiple .opam files in the repo? We would avoid a preponderance of repositories and make future name refactoring easier.

samoht · 2016-12-21T23:55:56Z

A quick update: I have made some progress and I am able to compile a few examples in mirage-skeleton with the new scheme: I managed to port tls with relatively few changes, I thing the last blocker (hopefully) is conduit. The packages that I am porting are tracked in https://github.com/samoht/mirage-dev/commits/master

My goal is to make all the examples compile in mirage-skeleton and then polish the patches and merge. I realise that it is a large patchset, which is blocking progress for other people, so I am trying to be quick ...

samoht · 2016-12-22T12:17:44Z

See mirage/mirage-skeleton#213 for the changes needed to mirage-skeleton to compile all of the example on the unix backend. Polishing up all the patches/PRs now.

One thing that maybe went wrong is that I notice that the TCP/IP stack seems a bit slow on OSX (using vmnetd). ICMP packets seems to get lost pretty often, and the HTTP request takes >1s (so maybe we lost the first SYN too). Not sure where the error comes from, I will try to reproduce without my patches.

yomimono · 2016-12-22T16:04:56Z

I'm working on the tcpip problem @samoht reported. I suspect I broke this in #643 and it's only now been discovered.

later: yes, this dates back to #643. mirage-net-xen was the only network impl I tested DHCP leases on, and it's also the only one that didn't swallow all exceptions, including Lwt.Canceled, and continue running the active listener. PRs submitted to fix this for mirage-net-unix, mirage-net-macosx, and mirage-net-solo5.

samoht · 2016-12-22T17:14:58Z

I have updated the issue body with all the current PRs. My plan is to continue fixing the last few remaining issues, and do a merge tomorrow.

The second-half of splitting the Mirage signature could (and probably should) be done separately. I plan to look at that next week.

V1 signatures are now split into: - V1.DEVICE is Mirage_device.S in mirage/mirage-device - V1.TIME is Mirage_time.S in mirage/mirage-time - V1.MCLOCK and V1.PCLOCK are Mirage_clock.{MCLOCK, PCLOCK} in mirage/mirage-clock - V1.RANDOM is Mirage_random.S in mirage/mirage-random - V1.FLOW is Mirage_flow.S in mirage/mirage-flow - V1.CONSOLE is Mirage_console.S in mirage/mirage-console - V1.BLOCK is Mirage_block.S in mirage/mirage-block And similar changes for V1_LWT, with the following convention: - V1_LWT.FOO is Mirage_foo_lwt.S For now V1 is kept unchanged and define aliases to the new signatures.

samoht · 2016-12-22T21:20:07Z

Rebased, with hopefully more green ticks this time.

samoht · 2016-12-23T07:51:13Z

mirage-skeleton is green on unix, xen and solo5. Most of the individual PRs are green too. I'll start to merge everything, expect a little bit of breakage for a few hours...

samoht force-pushed the errors branch from b2a56f8 to b73396d Compare December 20, 2016 11:59

hannesm reviewed Dec 20, 2016

View reviewed changes

avsm approved these changes Dec 20, 2016

View reviewed changes

samoht force-pushed the errors branch from 421f38c to f2d336e Compare December 20, 2016 21:43

samoht changed the title ~~refactors error~~ Refactors errors and split signatures into their own opam packages Dec 21, 2016

samoht force-pushed the errors branch 3 times, most recently from e58559e to 0ade9c8 Compare December 21, 2016 19:05

yomimono mentioned this pull request Dec 22, 2016

Allow cancelling input mirage/mirage-net-unix#36

Merged

samoht added 2 commits December 22, 2016 22:19

fix META files for types

b942d04

samoht added 2 commits December 22, 2016 22:19

Add TCP write errors

bda8658

TMP: use my fork of mirage-dev

4f4c16b

samoht force-pushed the errors branch from 0ade9c8 to 4f4c16b Compare December 22, 2016 21:19

yomimono approved these changes Dec 22, 2016

View reviewed changes

MagnusS mentioned this pull request Dec 23, 2016

Use topkg, use the new mirage-time and mirage-clock module, adapt to the new error scheme mirage/mirage-vnetif#11

Merged

samoht merged commit 207e122 into mirage:master Dec 23, 2016

samoht deleted the errors branch December 23, 2016 11:09

hannesm mentioned this pull request Dec 26, 2016

being able to choose arp implementation #750

Merged

This was referenced Mar 14, 2019

typing assert failure with nonrec priv ocaml/ocaml#7437

Closed

warning +34 exposes #row with private types ocaml/ocaml#7438

Closed

Conversation

samoht commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

new repos with new signatures

move V1 signatures in existing repo:

Use the new split sigs only

Adapt to the new error scheme + use the new split sigs

Fix pre-existing errors

Meta

Add new constraints

Uh oh!

hannesm commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avsm commented Dec 20, 2016

Uh oh!

samoht commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samoht commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbuenzli commented Dec 20, 2016

Uh oh!

samoht commented Dec 20, 2016

Uh oh!

dbuenzli commented Dec 20, 2016

Uh oh!

samoht commented Dec 20, 2016

Uh oh!

talex5 commented Dec 20, 2016

Uh oh!

samoht commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbuenzli commented Dec 20, 2016

Uh oh!

Drup commented Dec 20, 2016

Uh oh!

hannesm commented Dec 20, 2016

Uh oh!

samoht commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avsm commented Dec 21, 2016

Uh oh!

samoht commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samoht commented Dec 22, 2016

Uh oh!

yomimono commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samoht commented Dec 22, 2016

Uh oh!

samoht commented Dec 20, 2016 •

edited

Loading

hannesm commented Dec 20, 2016 •

edited

Loading

samoht commented Dec 20, 2016 •

edited

Loading

samoht commented Dec 20, 2016 •

edited

Loading

samoht commented Dec 20, 2016 •

edited

Loading

samoht commented Dec 21, 2016 •

edited

Loading

samoht commented Dec 21, 2016 •

edited

Loading

yomimono commented Dec 22, 2016 •

edited

Loading