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

document that unison's lwt is not an old bundled version of the newer thing also called lwt #1032

Open
mkrupcale opened this issue May 5, 2024 · 8 comments
Labels
effort-low issue is likely resolvable with <= 5h of effort enhancement issue is a request for a feature, and not a defect impact-low low importance

Comments

@mkrupcale
Copy link

Hello,

I am a Fedora packager and am looking to re-package unison in Fedora. It is currently under review [1], and the reviewer pointed out that there is a very old copy of ocaml-lwt [2] which is bundled in the unison src/lwt directory. Where possible, it is preferred for packages to rely on already-packaged library dependencies [3].

I am a total OCaml newbie, so I'm not sure how difficult of a task this is, but I played around with the Makefiles a bit and was able to get unison at least trying to build with the system ocaml-lwt, and this is the first error I encountered:

ocamlopt -g -I ubase -I system -I /usr/lib64/ocaml/lwt -I /usr/lib64/ocaml/lwt/unix -I +unix -I +str -I system/generic -c /builddir/build/BUILD/unison-2.53.4/src/terminal.ml
File "/builddir/build/BUILD/unison-2.53.4/src/terminal.ml", line 211, characters 57-76:
211 |                                               (fun () -> Lwt_unix.close fdIn)));
                                                               ^^^^^^^^^^^^^^^^^^^
Error: This expression has type unit Lwt.t
       but an expression was expected of type unit

Is it feasible to move unison to latest upstream ocaml-lwt?

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2277254#c2
[2] https://src.fedoraproject.org/rpms/ocaml-lwt
[3] https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

@gdt gdt added defect unison fails to meet its specification (but doesn't crash; see also "crash") effort-high issue is likely to require >20h of effort, perhaps much more effort-medium issue is likely resolvable with <= 20h of effort labels May 5, 2024
@gdt gdt changed the title Unbundling and using latest upstream Lwt unison has bundled lwt and it is old May 5, 2024
@gdt
Copy link
Collaborator

gdt commented May 5, 2024

This is a really good question. I am pretty sure the answer is yes, it's feasible, and no it is not going to happen really soon.

@mkrupcale
Copy link
Author

I am pretty sure the answer is yes, it's feasible

I'm glad to hear that!

and no it is not going to happen really soon.

Okay, would you be accepting of patches if I were to work on this myself? Again, knowing next to nothing about OCaml, I have no idea if I can manage it, but I'm willing to give it a go.

@tleedjarv
Copy link
Contributor

I don't think it is feasible. Lwt was originally created for Unison, then extracted as a separate project. Since then, the Lwt project has been rewritten a couple of times and diverged significantly from the original. The basic concepts are no longer the same.

Given this, the lwt code in Unison should not be considered a bundled version of ocaml-lwt package; they are entirely different and only same by name. This fact should resolve the package review comment.

@gdt
Copy link
Collaborator

gdt commented May 5, 2024

Probably then we should resolve it by having a README in the lwt directory that explains this relatively fully, so that those newly coming to the sources will end up with the right impression. It's certainly fair of people to see this and think something is wrong.

@gdt gdt changed the title unison has bundled lwt and it is old document that unison's lwt is not an old bundled version of the newer thing also called lwt May 5, 2024
@gdt gdt added effort-low issue is likely resolvable with <= 5h of effort impact-low low importance enhancement issue is a request for a feature, and not a defect and removed defect unison fails to meet its specification (but doesn't crash; see also "crash") effort-high issue is likely to require >20h of effort, perhaps much more effort-medium issue is likely resolvable with <= 20h of effort labels May 5, 2024
@mkrupcale
Copy link
Author

Lwt was originally created for Unison, then extracted as a separate project.

That's interesting, thanks for the history. Given this, who actually holds the copyright for the src/lwt contents, and what is its license? Looking at the history of ocaml-lwt, its original (2008) license appears to be LGPL-2.1-or-later [1], which was changed (2018) to MIT [2,3] with the permission of the copyright holders that they could identify at the time. I have no idea though if they were aware of this history or the original copyright holder(s).

I don't think it is feasible. [...] Since then, the Lwt project has been rewritten a couple of times and diverged significantly from the original. The basic concepts are no longer the same.

Gotcha.

Given this, the lwt code in Unison should not be considered a bundled version of ocaml-lwt package; they are entirely different and only same by name. This fact should resolve the package review comment.

Okay, I will convey this to the reviewer, and I suspect this will be sufficient justification.

[1] ocsigen/lwt@ed71a00#diff-c693279643b8cd5d248172d9c22cb7cf4ed163a3c98c8a3f69c2717edd3eacb7
[2] ocsigen/lwt#560
[3] ocsigen/lwt@124ad05

@tleedjarv
Copy link
Contributor

I don't know about the copyright and license. The original author is Jérôme Vouillon.
Perhaps @bcpierce00 can clarify?

@mkrupcale
Copy link
Author

The original author is Jérôme Vouillon.

Okay, it looks like he was involved in the license change process and approved the change [1], so that's good.

[1] ocsigen/lwt#560 (comment)

@avollmerhaus
Copy link

I've been compiling my own unison on Fedora for quite some time now, thanks for taking the time to look into this.
It's very much appreciated.
I've been trying to convince some of my Fedora-using friends who are less-inclined to compile stuff themselves to become unison users for quite some time now, this will lower the barrier somewhat 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-low issue is likely resolvable with <= 5h of effort enhancement issue is a request for a feature, and not a defect impact-low low importance
Projects
None yet
Development

No branches or pull requests

4 participants