Skip to content
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

Httpaf types #955

Merged
merged 4 commits into from Feb 2, 2019

Conversation

Projects
None yet
6 participants
@anmonteiro
Copy link
Contributor

anmonteiro commented Dec 17, 2018

This is a proposal to add http/af support in Mirage. Happy to take suggestions on naming / config.

This depends on inhabitedtype/httpaf#83

Show resolved Hide resolved lib/mirage.mli Outdated
@anmonteiro

This comment has been minimized.

Copy link
Contributor Author

anmonteiro commented Dec 17, 2018

@hannesm I addressed your comments, let me know if there's something that could still look better.

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Dec 17, 2018

LGTM! I'd appreciate some docstrings for the HTTP section, since now I'm confused why there's http and httpaf. On a related note, I'd appreciate to rename val http to val cohttp, and add a deprecated http (alias to cohttp for now), please see https://github.com/mirage/canopy-data/blob/master/Posts/Deprecating for more details if you are interested.

@anmonteiro

This comment has been minimized.

Copy link
Contributor Author

anmonteiro commented Dec 17, 2018

Done. Would you recommend different docstrings or deprecation message?

@anmonteiro anmonteiro force-pushed the anmonteiro:httpaf branch from e6b137d to 8b19705 Dec 17, 2018

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Dec 17, 2018

thanks again @anmonteiro! The @@deprecated annotation is only needed in the mirage.mli AFAICT, no need to retain it in mirage.ml.

@anmonteiro

This comment has been minimized.

Copy link
Contributor Author

anmonteiro commented Dec 17, 2018

Ah right. Makes sense. Updated.

@Drup

This comment has been minimized.

Copy link
Member

Drup commented Dec 18, 2018

Just looked at the latest version, looks very nice (and definitely better than the initial one, I agree with @hannesm's comments)

@dinosaure

This comment has been minimized.

Copy link
Member

dinosaure commented Dec 18, 2018

Good job 👍 ! Are you able to make a PoC of httpaf on mirage ?

@anmonteiro

This comment has been minimized.

Copy link
Contributor Author

anmonteiro commented Dec 18, 2018

@dinosaure there’s an example here:
inhabitedtype/httpaf#83

Is that what you were looking for?

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Dec 20, 2018

imho we can merge this now, and once httpaf-mirage is released to opam-repository add some tests to mirage-skeleton using httpaf. any other opinions?

val http_server: conduit impl -> http impl
[@@ocaml.deprecated "`http_server` is deprecated. Please use `cohttp_server` or `httpaf_server` instead."]

This comment has been minimized.

@avsm

avsm Dec 27, 2018

Member

Instead of deprecating this, how about just making it a choice between Cohttp and Httpaf via an optional argument, and then swapping the defaults in a future release as httpaf support matures? Most users will probably not care too much about the specific server implementation chosen, and advanced users have the functions below with the specific choice.

This comment has been minimized.

@hannesm

hannesm Feb 2, 2019

Member

since there is no common signature of httpaf and cohttp yet (esp. in respect to headers etc.), I don't think application developers can be http-server agnostic right now. in the future, this will hopefully be cleared up.

@avsm

This comment has been minimized.

Copy link
Member

avsm commented Dec 27, 2018

@hannesm I'd prefer not to deprecate http_server if possible (see my comment inline)

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Dec 27, 2018

@avsm sounds good to me as well -- similar to how the prng device works (including a (configuration-time-only) argument to specify the http server)

@anmonteiro

This comment has been minimized.

Copy link
Contributor Author

anmonteiro commented Dec 27, 2018

Do you mean via the mirage configure ... command?

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Jan 1, 2019

@anmonteiro yes, the user should be able to:
mirage configure --http httpaf and mirage configure --http cohttp to select configure-time the http implementation -- similar to --prng stdlib and --prng fortuna where stdlib is the default atm.

to implement this, this new configure-time argument needs to be implemented in mirage_key.ml/mli (the cohttp and httpaf bindings you developed in this PR should be kept) - it should be very similar to the prng key and the net key

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Jan 4, 2019

while hacking on code (trying to port https://github.com/Engil/Canopy to httpaf), I don't think it's as easy as having a http configuration key, since unikernel code would need a common interface (i.e. a Mirage_types.HTTP) - for now each unikernel needs to decide which http server to support (i.e. to add header fields).

@anmonteiro a PR for mirage-skeleton which adds applications/static_website_tls using httpaf would be nice :)

@samoht

This comment has been minimized.

Copy link
Member

samoht commented Jan 5, 2019

unikernel code would need a common interface (i.e. a Mirage_types.HTTP

That would be a good idea to add in a later PR!

@hannesm

This comment has been minimized.

Copy link
Member

hannesm commented Feb 2, 2019

merging -- this improves the current situation, and allows to easily use httpaf in the mirage context (once httpaf-mirage is released, this will also work out of the box). thanks!

@hannesm hannesm merged commit da3dd8b into mirage:master Feb 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 5, 2019

[new release] mirage, mirage-runtime, mirage-types-lwt and mirage-typ…
…es (3.4.1)

CHANGES:

* Provide a httpaf_server device, and a cohttp_server device (mirage/mirage#955, by @anmonteiro)
* There can only be a single prng device in a unikernel, due to entropy
  harvesting setup (mirage/mirage#959, by @hannesm)
* Cleanup zarith-freestanding / gmp-freestanding dependencies (mirage/mirage#964, by @hannesm)
* ethernet is now a separate package (mirage/mirage#965, by @hannesm)
* arp now uses the mirage/arp repository by default, the tcpip.arpv4
  implementation was removed in tcpip 3.7.0 (mirage/mirage#965, by @hannesm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.