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

[new release] http, cohttp, cohttp-top, cohttp-server-lwt-unix, cohttp-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~beta 1) #24699

Closed
wants to merge 6 commits into from

Conversation

mseri
Copy link
Member

@mseri mseri commented Oct 27, 2023

CHANGES:

mseri added 2 commits October 27, 2023 14:47
CHANGES:

- cohttp-eio: Complete rewrite to follow common interfaces and behaviors. (mefyl mirage/ocaml-cohttp#984)
- cohttp-eio: Add Client.make_generic and HTTPS support. (talex5 mirage/ocaml-cohttp#1002)
Signed-off-by: Marcello Seri <[email protected]>
@hannesm
Copy link
Member

hannesm commented Oct 27, 2023

from the changelog, there's only "cohttp-eio" being changed -- why not publish only that package then?

@mseri
Copy link
Member Author

mseri commented Oct 27, 2023 via email

@hannesm
Copy link
Member

hannesm commented Oct 27, 2023

It is a very good point. Mostly because of the locked version constraints. I can double check that is only eio and modify the pr and the version locks to reflect this (it will be weird that cohttp-eio beta will use cohttp alpha but life is such I guess :P)

that's what I did in tls-eio 0.15.5 #22366 and tls-eio 0.17.2 #24515 ... looks weird, but it saves quite some computation on CI machines and opam clients (I understand this only partially fits this PR, as you mark it as avoid-version (which works for opam 2.1.0+ clients)).

@mseri
Copy link
Member Author

mseri commented Oct 27, 2023

I actually cannot do it. There are some changes in cohttp to make the client and server interfaces generic and suitable for all packages. The change should be backward compatible though. I edited the message and will add a mention also in cohttp's CHANGELOG

@mseri
Copy link
Member Author

mseri commented Oct 27, 2023

In any case you make a very good point. I will be more careful with future PRs

@avsm
Copy link
Member

avsm commented Oct 30, 2023

A lot of 404s on the archives; did the rearrangement of the PR delete the beta1 tbzs?

@mseri
Copy link
Member Author

mseri commented Oct 30, 2023

Ah you are right. Updated

@mseri
Copy link
Member Author

mseri commented Oct 31, 2023

There seems to be a regression


#=== ERROR while compiling prometheus-app.1.2 =================================#
# context              2.2.0~alpha3~dev | linux/x86_64 | ocaml-base-compiler.4.14.1 | file:///home/opam/opam-repository
# path                 ~/.opam/4.14/.opam-switch/build/prometheus-app.1.2
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p prometheus-app -j 127
# exit-code            1
# env-file             ~/.opam/log/prometheus-app-7-51de6d.env
# output-file          ~/.opam/log/prometheus-app-7-51de6d.out
### output ###
# (cd _build/default && /home/opam/.opam/4.14/bin/ocamlc.opt -w -40 -g -bin-annot -I app/.prometheus_app.objs/byte -I /home/opam/.opam/4.14/lib/angstrom -I /home/opam/.opam/4.14/lib/asetmap -I /home/opam/.opam/4.14/lib/astring -I /home/opam/.opam/4.14/lib/base64 -I /home/opam/.opam/4.14/lib/bigstringaf -I /home/opam/.opam/4.14/lib/cohttp -I /home/opam/.opam/4.14/lib/cohttp-lwt -I /home/opam/.opam/4.14/lib/fmt -I /home/opam/.opam/4.14/lib/http -I /home/opam/.opam/4.14/lib/logs -I /home/opam/.opam/4.14/lib/lwt -I /home/opam/.opam/4.14/lib/ppx_sexp_conv/runtime-lib -I /home/opam/.opam/4.14/lib/prometheus -I /home/opam/.opam/4.14/lib/re -I /home/opam/.opam/4.14/lib/seq -I /home/opam/.opam/4.14/lib/sexplib0 -I /home/opam/.opam/4.14/lib/stringext -I /home/opam/.opam/4.14/lib/uri -I /home/opam/.opam/4.14/lib/uri-sexp -intf-suffix .ml -no-alias-deps -o app/.prometheus_app.objs/byte/prometheus_app.cmo -c -impl app/prometheus_app.ml)
# File "app/prometheus_app.ml", line 157, characters 6-27:
# 157 |       Server.respond_string ~status:`OK ~headers ~body ()
#             ^^^^^^^^^^^^^^^^^^^^^
# Error: Unbound value Server.respond_string
# (cd _build/default && /home/opam/.opam/4.14/bin/ocamlopt.opt -w -40 -g -I app/.prometheus_app.objs/byte -I app/.prometheus_app.objs/native -I /home/opam/.opam/4.14/lib/angstrom -I /home/opam/.opam/4.14/lib/asetmap -I /home/opam/.opam/4.14/lib/astring -I /home/opam/.opam/4.14/lib/base64 -I /home/opam/.opam/4.14/lib/bigstringaf -I /home/opam/.opam/4.14/lib/cohttp -I /home/opam/.opam/4.14/lib/cohttp-lwt -I /home/opam/.opam/4.14/lib/fmt -I /home/opam/.opam/4.14/lib/http -I /home/opam/.opam/4.14/lib/http/__private__/http_bytebuffer -I /home/opam/.opam/4.14/lib/logs -I /home/opam/.opam/4.14/lib/lwt -I /home/opam/.opam/4.14/lib/ppx_sexp_conv/runtime-lib -I /home/opam/.opam/4.14/lib/prometheus -I /home/opam/.opam/4.14/lib/re -I /home/opam/.opam/4.14/lib/seq -I /home/opam/.opam/4.14/lib/sexplib0 -I /home/opam/.opam/4.14/lib/stringext -I /home/opam/.opam/4.14/lib/uri -I /home/opam/.opam/4.14/lib/uri-sexp -intf-suffix .ml -no-alias-deps -o app/.prometheus_app.objs/native/prometheus_app.cmx -c -impl app/prometheus_app.ml)
# File "app/prometheus_app.ml", line 157, characters 6-27:
# 157 |       Server.respond_string ~status:`OK ~headers ~body ()
#             ^^^^^^^^^^^^^^^^^^^^^
# Error: Unbound value Server.respond_string

Please don't merge

@mseri
Copy link
Member Author

mseri commented Nov 3, 2023

@samoht do you understand why irmin is failing now? The signature of IO should not have changed in cohttp

@samoht
Copy link
Member

samoht commented Nov 4, 2023

@mseri

A diff between alpha2 and beta1 shows this:

 module type Server = sig
-  module IO : IO

This explains why this is breaking irmin-graphql

@mseri
Copy link
Member Author

mseri commented Nov 22, 2023

Seems like it is useful to have it there, what do you think? Should I try to have it back there or just add an upper bound to irmin?

@samoht
Copy link
Member

samoht commented Nov 24, 2023

I think it'd be best not to break that API if possible - enough changes in the rest already :-)

@mseri
Copy link
Member Author

mseri commented Jan 5, 2024

Closing in favour of the upcoming beta 2

@mseri mseri closed this Jan 5, 2024
@mseri mseri deleted the release-cohttp-v6.0.0_beta1 branch January 5, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants