Fix lambda term work with lwt3 and lwt4 #61
Conversation
bikallem
left a comment
bikallem
left a comment
There was a problem hiding this comment.
@rgrinberg This is nice. I was experimenting with cppo but this is even cleaner and much better. 👍
| (lwt lwt_react zed | ||
| (select lTerm_sequence.ml from | ||
| (lwt.unix -> lTerm_sequence_lwt3.ml) | ||
| (lwt-unix -> lTerm_sequence_lwt4.ml)) |
There was a problem hiding this comment.
I think lwt 4.0.0 still defines lwt.unix findlib package as lwt.unix and not lwt-unix. See https://github.com/ocsigen/lwt/blob/master/src/unix/jbuild#L56. Isn't the dune (library (public_name) field the findlib package name?
Incidentally, I tried ocamlfind query lwt-unix and it returns with ocamlfind: Package `lwt-unix' not found. and when I do ocamlfind query lwt.unix it returns the correct correct path.
cc @aantron. Were you looking to rename lwt.unix to lwt-unix in lwt 4.0.0?
There was a problem hiding this comment.
Indeed, lwt.unix is correct, and we haven't renamed it.
There was a problem hiding this comment.
Ah yes, ok so we'll need a different check. Unfortunately, we might not be able to do with a select form anymore, which will complicate things.
| ignore (Lwt_unix.on_signal signum (fun _ -> send_resize ())) | ||
| with Not_found -> | ||
| ignore (Lwt_sequence.add_r send_resize Lwt_main.enter_iter_hooks) | ||
| ignore (LTerm_sequence.add_r send_resize Lwt_main.enter_iter_hooks) |
There was a problem hiding this comment.
Not sure if this will compile. Is 'a LTerm_sequence.t = 'a Lwt_sequence.t? If so, how may I ask?
There was a problem hiding this comment.
Checkout the lwt3.mli. We are making sure 'a LTerm_sequence.t = Lwt_sequence.t
|
|
||
| exception Empty | ||
|
|
||
| type 'a t = { |
There was a problem hiding this comment.
Not sure how this will work. Will this 'a t be = to 'a Lwt_sequence.t? Pls seem my comment on lTerm.ml above. How is this type equality established?
There was a problem hiding this comment.
We don't need the type equality for Lwt4 since Lwt_sequence isn't going to be present, right?
There was a problem hiding this comment.
Hmm. I think we do since we use Lwt_main.enter_iter_hooks which returns Lwt_sequence.t. My initial attempt got stuck here since I couldn't establish equality between the two types.
|
Btw, @bikallem this PR is just a POC. Feel free to take this and incorporate into your own PR. I think what's left to make this work is to have a proper way to detect lwt3 vs lwt4. I don't think jbuilder currently has a way to get the version number of external packages. That's a bit of a shame, though easily fixable in dune. Btw, testing things based on version number can be a bit brittle. I wish there was a way to simply check if the library has a certain module or not. But constructing such a capability based test is too involved at the moment unfortunately. |
|
@diml @rgrinberg Based on the comment from @aantron.(ocsigen/lwt#573 (comment)) I think the relatively straightforward approach for now is to disable the deprecation warning. We will probably have to revisit this once |
|
@bikallem The "best" course of action depends on whether If you disable the deprecation warning, I recommend doing it locally for expressions and/or declarations, not files or libraries. |
|
Thanks. Noted re: deprecating locally only. |
3 participants
cc @bikallem