-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Lwt 4.0.0: promises and parallelized I/O #11708
Conversation
@aantron Failures in the Travis runs:
Is there a missing dependency somewhere? |
Thanks. This looks to be due to opam's |
Working out constraints and notifications for the failed revdeps now. Interestingly, only arm64 builds seem to have run. |
The last commit above adds the constraints I was able to find from the previous revdeps build. The latest build seems to be broken due to being unable to create the build container. In addition, it is still arm64-only, so several packages (notably Please let me know if the revdeps build can be fixed. Otherwise, I believe most of the value of the revdeps build has already been extracted, so I will notify maintainers of the packages constrained, and we can merge Lwt into the repo. Travis CI passed for Lwt 4.0.0, but is broken now, with all the new constraints, because it is trying to build all those constrained packages without the proper environments that they require. |
The x86 CI was down due to a machine failure; the service has now been restored. |
Thank you :) already working on more constraints. |
The revdeps builds are now failing with "depfail" because (I guess) the basic V2.0 Build is failing at |
I excluded the constraints commits for now, so I can take a look at any extra constraints that will be found by the x86 build. |
@aantron i think it makes sense to PR the constraints in another PR, maybe even chunk that up into smaller bits so that the CI can manage. |
@hannesm That's a good idea. I propose a modified variant: I add the first, and largest, set of constraints in this PR (see above commit). This will break the CI build, but we are already pretty sure that most of these constraints are correct. We merge this PR. Even though the final CI build of this PR is broken due to the large number of packages affected, you can see that After merging this PR, I'll open a second dummy PR that twiddles only the new Lwt packages, just to trigger a new revdeps build. This will let us catch any more constraints we need to add. I'll constrain those in a third PR. The set should be much smaller here. I don't want to chunk up the initial set of constraints into multiple PRs, because it will be a lot of human time spent for learning the same information we already learned from the original revdeps build in this PR. If you agree, then I think this PR is ready for merging. |
@aantron to minimise the breakage for random people on the internet, I'd like to first merge your added constraints (thanks for adding them) into the opam-repository, and only in a second step merge the lwt 4.0 release (after CI confirmed that revdeps are indeed good now). does this sound like a plan: (a) open a second PR with your last commit here, (b) rebase this PR on master once (a) is merged? |
Yes, that sounds even better. I'll open the other PR in a couple hours when I'm back at the computer. |
* Constrain packages for Lwt 4.0.0 See #11708.
❗ opam-lint warnings 5474ec4
✅ Installability check (8665 → 8669)
|
@avsm, I am gradually accumulating a list of packages for notifying maintainers. Each package is linked to its failing build log, some of which are in the arm64 CI, and the arm64 CI site is having certificate problems. |
Also adds Lwt_ppx 1.2.0, Lwt_react 1.1.1, Lwt_log 1.1.0.
thanks @aantron for your work and constraining revdeps. I'm in favour of merging this rather sooner than later. what is the current status? what the CI is missing out on are the revdeps which list |
@hannesm fixed by avsm/mirage-ci#23 and running live on the ARM64 CI. Sorry for that. |
Looks like ocurl, mongo, markup, logs-syslog, irc-client, inotify, faraday-lwt (?), exenum, csv, containers & biocaml need constraints as well (see arm64 ci for more details) |
@kit-ty-kate Thanks for the depopts build, it is very useful :) I've added new constraints in #11741. |
Revdeps are rebuilding on the ARM64 CI. |
@kit-ty-kate Shouldn't I rebase this PR to pick everything up? Or it doesn't work that way? |
No it's fine for the ARM64 CI as it merges automatically with master, and the x86_64 CI doesn't test the depopts. |
Several builds seem to have failed with No space left on device, e.g. https://arm64.ci.ocamllabs.io/log/saved/docker-run-508421ae48926a3b4ffaedd669e6b5c4/cde7e0ce5e6dbb8040ee81d51486b40a764e60c7. I think we already know that these packages are properly constrained from earlier builds, but I wanted to point it out. |
oh god, not again! :( I'll try to fix it for good tomorrow. Thx for pointing it out. Good night \o |
Okay, awaiting ping :) |
Looks good. Most of the failures come from the fact that camlcity.org is down (ocamlnet cannot be download) so let's merge this. Thanks a lot !! :) |
Thank you! |
Maintainers! You are ccd below because your opam package has been constrained with Please see below for details. All the fixes are trivial, and involve slightly changing dependency names due to the reorganization of Lwt. Each incompatible package name is linked to a CI log displaying the error from building against Lwt 4.0.0. The following packages depend on ocamlfind package
To fix, change cc @Armael @c-cube @copy @cyberhuman @emillon @haesbaert @mfp @ygrek @zoggy The following packages depend on ocamlfind package
The easy fix is to exchange Since cc @andersfugmann @gabelevi @paurkedal The following packages expect
The easy fix is to add an opam dependency on any version of cc @anyakichi @avsm @diml @djs55 @gaborigloi @ivg @Leonidas-from-XIV @rgrinberg @toolslive @vzaliva The following packages depend on ocamlfind package
To fix this, replace cc @c-cube @djs55 @dsheets @jonludlam @pveber @seliopou @smondet @talex5 @toolslive @vasilisp The following packages use modules from
Before Lwt 4.0.0, To fix this, add the missing cc @agarwal @alinpopa @djs55 @dsheets @hannesm @lebotlan @pveber @samoht @seliopou @vbmithr |
According to the nice comment by @aantron in ocaml#11708 (comment) slacko fails because it expects lwt.log to exist without depending on the lwt_log message. This came to a surprise to me since Slacko does not do any logging at all, so constraining it does not seem to be useful. Looking in the CI log https://ci.ocamllabs.io/log/saved/docker-run-e97b0a51ae9b04229ade4297103e6c79/e87df7e1b52e41541b179ab4024a538bfd0b1484 I can see that the error came from a module in "lwt/cohttp_lwt_unix.ml" which happens when building cohttp.0.21.1. Therefore I would like to correct the dependency here. If any version of cohttp < 0.99 would be compatible with Lwt 4 (e.g. if someone does a maintenance release), so would slacko.
Congrats on shipping! I just pushed a new version of |
it seems that
|
@hhugo |
I'll send a PR with the constraints for existing versions. |
This is a breaking release of Lwt, that mainly cleans up the Lwt packaging.
The Lwt PPX is also released with minor breaking changes. Lwt_react has a bugfix. Lwt_log 1.1.0 is the first release of Lwt_log with independent packaging. The pre-existing Lwt_log 1.0.0 release was a forward-compatibility alias for Lwt. That Lwt_log 1.0.0 release has been constrained to require Lwt < 4.0.0.
From the changelog: