bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
4.16k stars 234 forks source link

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

Open mkrupcale opened 6 months ago

mkrupcale commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

mkrupcale commented 6 months ago

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] https://github.com/ocsigen/lwt/commit/ed71a00f0d5780234c870e9f62d36f62d899f553#diff-c693279643b8cd5d248172d9c22cb7cf4ed163a3c98c8a3f69c2717edd3eacb7 [2] https://github.com/ocsigen/lwt/issues/560 [3] https://github.com/ocsigen/lwt/commit/124ad05acc0f7c7b8a9295f5a6ea2ac2d0d9d5b4

tleedjarv commented 6 months ago

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

mkrupcale commented 6 months ago

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] https://github.com/ocsigen/lwt/issues/560#issuecomment-382642533

avollmerhaus commented 6 months ago

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 :+1: