WIP check upstream #26
Conversation
|
related to ocaml/opam/issues/3504 |
| @@ -692,18 +692,17 @@ module PrChecks = struct | |||
|
|
|||
| let max_items_in_post = 50 | |||
|
|
|||
| let opam_files = | |||
| Lwt.return (nv, result) | ||
| ) (opam_files files) | ||
| in | ||
| let ok = |
There was a problem hiding this comment.
hm, ça ne serait pas plus léger de faire un seul fold avec un quadruplet comme accumulateur ?
| match upstream with | ||
| | Some url -> | ||
| let open OpamProcess.Job.Op in | ||
| OpamProcess.Job.run @@ |
There was a problem hiding this comment.
I am a little afraid of seeing OpamProcess.Job.run used repeatedly here, for a few reasons:
- opam "jobs" are explicitely designed to handle parallelism at their own level, using e.g.
OpamParallel.map - we use syscalls and signals for handling sub-processes, I would certainly double-check that those don't cause bad interaction with lwt
- using this, the lwt scheduler will be blocked for the whole duration of the download, while it should be kept awake for listening to incoming requests (if just to ACK and queue them)
Note however that nested calls to OpamProcess.Job.run are supported (the outer scheduler is just freezed until the inner call terminates) ; using Lwt_preemptive (which should indeed be called Lwt_preemptable) might be enough to avoid the last issue, but requires careful use.
| | _ -> None) upstream_check | ||
| in | ||
| let title, status = | ||
| if err <> [] || fatal <> []then |
There was a problem hiding this comment.
why not display both upstream and lint warnings if we have both ? I would skip the elses here.
| in | ||
| let warn = Printf.sprintf "No checksum defined for %s" (str_pkg warn) in | ||
| let err = | ||
| OpamStd.Format.itemize ~bullet:" * " |
There was a problem hiding this comment.
I am not sure this formatting will work, itemize is suposed to start at the beginning of a line.
Maybe just add a header? (e.g. "Errors:\n" ^ OpamStd.Format.itemize ...)
Add check upstream: downloading archive & check hashes