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.

Sign up for GitHub

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

Jump to bottom

Remove dependency to base #745

Merged
merged 3 commits into from
Feb 6, 2021
Merged

Remove dependency to base #745

merged 3 commits into from
Feb 6, 2021

Conversation

samoht
Copy link
Member

@samoht samoht commented Feb 5, 2021

This is causing issues to link mirage unikernels:

ld: _build/main.native.o: in function `camlBase__Import0__entry':
/home/opam/.opam/4.10/.opam-switch/build/base.v0.14.1/_build/default/src/import0.ml:358: undefined reference to `Base_am_testing'
ld: _build/main.native.o: in function `camlBase__Import0__gc_roots':
(.data+0x82ddb8): undefined reference to `Base_am_testing'

Also we don't need this to generate field accesses.

@samoht samoht mentioned this pull request Feb 5, 2021
Closed
@mseri
Copy link
Collaborator

mseri commented Feb 5, 2021

This (partially?) reverts #686, can we keep compare or that requires base for some reason?

@samoht
Copy link
Member Author

samoht commented Feb 5, 2021 •
edited
Loading

Why can't we use Stdlib.compare here? Happy to specialize it by type but what does it bring?

@mseri
Copy link
Collaborator

mseri commented Feb 5, 2021

Can you directly compare requests and responses using Stdlib.compare? I thought it was not directly possible without, which I think was the reason of that PR

@samoht
Copy link
Member Author

samoht commented Feb 6, 2021

Ha yes it needs special comparators to make sure the header maps are compared using the right equality. Will re-expose these functions too.

@samoht
Copy link
Member Author

samoht commented Feb 6, 2021

I've now re-exposed Response.compare and Request.compare.

mseri
mseri reviewed Feb 6, 2021
View reviewed changes
cohttp/test/dune
@@ -1,6 +1,7 @@
(executable
(name test_accept)
(modules test_accept)
(forbidden_libraries base)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update dune lower bound to use this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I've just bumped it in the opam file (the dune-project file was already using the right version)

mseri
mseri approved these changes Feb 6, 2021
View reviewed changes
Copy link
Collaborator

@mseri mseri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add something? If not I think this can be merged (don't forget to add a line to the changelog)

@samoht samoht merged commit 1929592 into mirage:master Feb 6, 2021
mseri added a commit to mseri/opam-repository that referenced this pull request Mar 24, 2021
@mseri
[new release] cohttp-lwt-jsoo, cohttp, cohttp-lwt, cohttp-lwt-unix, c…
3712b50 
…ohttp-top, cohttp-async and cohttp-mirage (4.0.0)

CHANGES:

- cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev mirage/ocaml-cohttp#752)
- Remove dependency to base (@samoht mirage/ocaml-cohttp#745)
- fix opam files and dependencies
- add GitHub Actions workflow (@smorimoto mirage/ocaml-cohttp#739)
- lwt_jsoo: Forward exceptions to caller when response is null (@mefyl mirage/ocaml-cohttp#738)
- Remove wrapped false (@rgrinberg mirage/ocaml-cohttp#734)
- Use implicit executable dependency for generate.exe (@TheLortex mirage/ocaml-cohttp#735)
- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
- [reverted] breaking changes to client and server API to use conduit
  3.0.0 (@dinosaure mirage/ocaml-cohttp#692). However, as the design discussion did not reach
  consensus, these changes were reverted to preserve better compatibility with
  existing cohttp users. (mirage/ocaml-cohttp#741,  @samoht)
@mseri mseri mentioned this pull request Mar 24, 2021
Merged
mseri added a commit to mseri/opam-repository that referenced this pull request Mar 25, 2021
@mseri
[new release] cohttp-lwt-jsoo, cohttp, cohttp-lwt, cohttp-lwt-unix, c…
2f7b4b2 
…ohttp-top, cohttp-async and cohttp-mirage (4.0.0)

CHANGES:

- cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev mirage/ocaml-cohttp#752)
- Remove dependency to base (@samoht mirage/ocaml-cohttp#745)
- fix opam files and dependencies
- add GitHub Actions workflow (@smorimoto mirage/ocaml-cohttp#739)
- lwt_jsoo: Forward exceptions to caller when response is null (@mefyl mirage/ocaml-cohttp#738)
- Remove wrapped false (@rgrinberg mirage/ocaml-cohttp#734)
- Use implicit executable dependency for generate.exe (@TheLortex mirage/ocaml-cohttp#735)
- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
- [reverted] breaking changes to client and server API to use conduit
  3.0.0 (@dinosaure mirage/ocaml-cohttp#692). However, as the design discussion did not reach
  consensus, these changes were reverted to preserve better compatibility with
  existing cohttp users. (mirage/ocaml-cohttp#741,  @samoht)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mseri mseri mseri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samoht @mseri