Add support for OCaml 4.12

[kit-ty-kate]

ocaml/ocaml#9869 added Unix.SO_REUSEPORT and made Lwt not compile with OCaml 4.12. This PR fixes the issue.

[raphael-proust]

Thanks a lot for the patch.

I'm not sure about adding COMPILER=4.12.0+trunk REPOSITORIES=--repo=default,beta=git://github.com/ocaml/ocaml-beta-repository.git, specifically the beta part, to the master branch here. The rest is all good.

[aantron]

@raphael-proust That has been the way to test on OCaml pre-releases historically. See, for example, https://github.com/ocsigen/lwt/blob/81fa268b93e334498301b6b12074e0b626f61f45/.travis.yml. There's no issue with that being in master, as it's only related to the CI setup, and, in any case, it makes the branch strictly better tested, especially when contributors open PRs without deliberately considering changes in upcoming OCaml releases.

[raphael-proust]

That makes sense.

Travis is failing because ocaml -version raises an exception. I'll look into that.

[kit-ty-kate]

Travis is failing because ocaml -version raises an exception. I'll look into that.

This is a known issue with current trunk. Here are the relevant fix in ocaml-ci-scripts (ocaml/ocaml-ci-scripts@3368e6a) and in ocaml itself (ocaml/ocaml#9798)

[kit-ty-kate]

CI will continue to fail until ocaml-migrate-parsetree and ppx_tools_versioned are updated for OCaml 4.12. I forgot to consider the dependencies of lwt_ppx, sorry.

There is no rush for now, I'm keeping this branch in opam-alpha-repository for testing purpose anyway for those who want to test packages depending on lwt quickly.

I'll remove 8fddc16 when all the required dependencies will be available to trigger the CI and at that point it might be ready to merge. I'll make it a draft in the meantime.

[aantron]

@kit-ty-kate One more option is to use allow_failures like so:

lwt/.travis.yml

Lines 26 to 33 in 81fa268

	allow_failures:
	- env: COMPILER=ocaml-variants.4.09.0+beta2 REPOSITORIES=--repositories=default,beta=git+https://github.com/ocaml/ocaml-beta-repository.git
	- env: COMPILER=ocaml-variants.4.08.1+flambda
	- env: COMPILER=4.07.1 DOCS=yes
	- env: COMPILER=4.06.1 LWT_STRESS_TEST=true
	- env: COMPILER=4.05.0
	- env: COMPILER=4.04.2
	- env: COMPILER=4.03.0

[hannesm]

what I was wondering, why does Lwt_unix copy the definition of these sum types from Unix? Can't it simply do a type alias instead (and this way not care about changes in future OCaml versions)?

[raphael-proust]

why does Lwt_unix copy the definition of these sum types from Unix? Can't it simply do a type alias instead (and this way not care about changes in future OCaml versions)?

I think the original intent might have that it let's people use the constructors directly after doing a open Lwt_unix (and we definitely don't want people to open Unix) or within the scope of a local open. A typical use might have been thought to be let foo x = let open Lwt_unix in Lwt.catch (fun () -> <some unixery> (function <pattern that uses the variants directly> -> …).

We could change it but that's a breaking change.

[hannesm]

@raphael-proust I see what you mean, sorry for the noise. Please go ahead with this very fine PR.

[talex5]

ocaml-ci is now testing 4.12 too. Feel free to add https://github.com/apps/ocaml-ci if you want to test with that (make sure to select only the lwt repository when enabling it, though).

[avsm]

ocaml-ci added to lwt

[avsm]

4.12 fails with

File "src/unix/lwt_unix.cppo.ml", line 222, characters 17-29:
Error (warning 16 [unerasable-optional-argument]): this optional argument cannot be erased.

[kit-ty-kate]

All green

[avsm]

Not blocking this PR, but let execute_job ?async_method ~job ~result ~free looks like a dodgy prototype -- we might as well make async_method a required argument, or add a unit to the end of that function.

[kit-ty-kate]

This function is exported and most importantly: deprecated. I don't think it's worth changing the interface of a deprecated function. I'm guessing it will be removed in a future major release anyway.

[aantron]

Bah thanks, I was merging master in, too :) You have the quicker draw :)

[aantron]

I'm ready to merge this after addressing CI nits.

[aantron]

Can you tweak the rows in allow_failures, so that it recognizes the 4.09 row again? We should also add 4.10 and 4.12 to allow_failures. Travis is set up to require on Linux the least and greatest OCaml release that we test (4.02.3 and 4.11.1, respectively, in this PR), and the rest are in allow_failures so that we can get build feedback faster if Travis has a build backlog.

[aantron]

I suggest using the 4.12 alpha release at this point.

[kit-ty-kate]

I would argue against. In my experience running +trunk is more stable than running any ~alpha/~beta. The changes are slow and rather incremental. If something is done in the 4.12 branch and break something here then it should be reported / fixed. But again, in my experience, once branched out, the alpha/beta branches are quite stable and contain bug fixes that are not in the ~alpha/~beta releases.

If that can help you I could add it to the allow_failures list though.

[aantron]

I agree with your argument, thanks. I think it does belong in allow_failures for how the Lwt CI is set up.

[kit-ty-kate]

All three CIs are now green.

[aantron]

Thanks!