/mirage

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactors errors and split signatures into their own opam packages #743

Merged
merged 4 commits into from Dec 23, 2016

Conversation

Reviewers

@avsm avsm

@hannesm hannesm

@yomimono yomimono

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@samoht @hannesm @avsm @dbuenzli @talex5 @Drup @yomimono
@samoht
Member

samoht commented Dec 20, 2016 •
edited

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:

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
@hannesm

This comment has been minimized.

Sign in to view
Member

hannesm commented Dec 20, 2016 •
edited

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!
lib_runtime/mirage_pp.ml Outdated
| `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

This comment has been minimized.

Sign in to view
Copy link
@hannesm

hannesm Dec 20, 2016

Member

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

This comment has been minimized.

Sign in to view
Copy link
@samoht

samoht Dec 20, 2016

Member

Yes, good point.

This comment has been minimized.

Sign in to view
Copy link
@avsm

avsm Dec 20, 2016

Member

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...

This comment has been minimized.

Sign in to view
Copy link
@samoht

samoht Dec 20, 2016

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@hannesm

hannesm Dec 20, 2016

Member

I prefer the latter as well.
@avsm

avsm approved these changes Dec 20, 2016
View changes

lib_runtime/mirage_pp.ml Outdated
@@ -1,79 +1,47 @@
open Result
open V1
let pf pp = Format.fprintf pp

This comment has been minimized.

Sign in to view
Copy link
@avsm

avsm Dec 20, 2016

Member

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

This comment has been minimized.

Sign in to view
Copy link
@samoht

samoht Dec 20, 2016

Member

yup, this needs some cleanup.
types/V1.mli Outdated
@@ -48,10 +48,23 @@ open Result
{e Release %%VERSION%% } *)
(** {1 Abstract devices}
type 'a pp = Format.formatter -> 'a -> unit

This comment has been minimized.

Sign in to view
Copy link
@avsm

avsm Dec 20, 2016

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@samoht

samoht Dec 20, 2016

Member

Today mirage-types does not have any dependency.

This comment has been minimized.

Sign in to view
Copy link
@hannesm

hannesm Dec 20, 2016

Member

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

This comment has been minimized.

Sign in to view
Copy link
@avsm

avsm Dec 20, 2016

Member

Fine by me.

This comment has been minimized.

Sign in to view
Copy link
@avsm

avsm Dec 20, 2016

Member

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

This comment has been minimized.

Sign in to view
Member

avsm commented Dec 20, 2016 

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 20, 2016 •
edited

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 20, 2016 •
edited

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:

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

This comment has been minimized.

Sign in to view
Contributor

dbuenzli commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Contributor

dbuenzli commented Dec 20, 2016

@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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Contributor

talex5 commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 20, 2016 •
edited

@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

This comment has been minimized.

Sign in to view
Contributor

dbuenzli commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Member

Drup commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Member

hannesm commented Dec 20, 2016

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 21, 2016 •
edited

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

This comment has been minimized.

Sign in to view
Member

avsm commented Dec 21, 2016

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 samoht changed the title from refactors error to Refactors errors and split signatures into their own opam packages Dec 21, 2016

@samoht

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 21, 2016 •
edited

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 22, 2016

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

This comment has been minimized.

Sign in to view
Member

yomimono commented Dec 22, 2016 •
edited

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

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 22, 2016

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.

@yomimono yomimono referenced this pull request Dec 22, 2016

Merged

Allow cancelling input #36

samoht added some commits Dec 20, 2016

@samoht
Split V1 into pieces 
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.
ca24715
@samoht
TMP: use my fork of mirage-dev
Loading status checks…
4f4c16b
@samoht
fix META files for types
b942d04
@samoht
Add TCP write errors
bda8658
@samoht

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 22, 2016

Rebased, with hopefully more green ticks this time.
@samoht

This comment has been minimized.

Sign in to view
Member

samoht commented Dec 23, 2016

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...

@MagnusS MagnusS referenced this pull request Dec 23, 2016

Merged

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

@samoht samoht merged commit 207e122 into mirage:master Dec 23, 2016
0 of 6 checks passed

0 of 6 checks passed

ci/datakit/1 Build Build exit 66
Details
ci/datakit/2 Revdeps depfail
Details
ci/datakit/3 Compilers depfail
Details
ci/datakit/4 Common Distros depfail
Details
ci/datakit/5 All Distros depfail
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@samoht samoht deleted the samoht:errors branch Dec 23, 2016

@hannesm hannesm referenced this pull request Dec 26, 2016

Merged

being able to choose arp implementation #750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment