bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
3.86k stars 224 forks source link

OCamlfind is required to build the GUI with lablgtk3. #1002

Closed jhjourdan closed 4 months ago

jhjourdan commented 4 months ago

INSTALL.md tells that ocamlfind is required for GUI support. Indeed, if lablgtk3 is installed but ocamlfind is not, build fails with the following error message:

ocamlopt -g -I lwt -I ubase -I system -I +unix -I +str -I system/generic -I lwt/generic -I +lablgtk3 -I +cairo2 -c /home/jjourdan/Software/unison/_opam/.opam-switch/build/unison.2.53.4/src/uigtk3.ml
File "/home/jjourdan/Software/unison/_opam/.opam-switch/build/unison.2.53.4/src/uigtk3.ml", line 75, characters 26-48:
75 | let fontMonospace = lazy (Pango.Font.from_string "monospace")
                               ^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound module Pango

This patch deactivates GUI compilation if ocamlfind is not installed (the build will fail anyways), and add the corresponding clauses in the opam files.

jhjourdan commented 4 months ago

I will push the updated opam files to the opam repository. This means that the opam files for 2.53.4 will be slightly bogus: the installation of the unison package will fail if lablgtk3 is installed but ocamlfind is not. This will be fixed in future releases.

jhjourdan commented 4 months ago

It seems to me that your issues could be with "lablgtk3" and not with unison.

I also find it weird that you'd need to add "ocamlfind" opam dependency for the GUI build. Doesn't that mean that package "lablgtk3" has declared its dependencies incorrectly?

lablgtk3 in itself does not depends on ocamlfind. ocamlfind is just a software used to find where libraries are installed and to pass the right corresponding options to the OCaml compiler. For that purpose, one can use other tools (e.g., dune is able to do that by itself), or write some ad-hoc scripts. In the opam repository, many packages depend on lablgtk3 but not on ocamlfind, because they use dune instead.

Don't change the makefile, it's used for builds without opam, too. Perhaps a broken lablgtk3 installation is causing your issue?

Why remove this? This branch works when not using opam. ocamlfind is not needed when lablgtk3 is already installed. I don't have ocamlfind installed and I can build the GUI just fine.

Ok, then I'm sorry. Given that INSTALL.md tells that ocamlfind is necessary, I thought that the intended way of building the GUI was by using ocamlfind.

That being said, I had a deeper look at this, and I'd still like to change the Makefile a bit, because I still think it is bogus (I've just pushed my proposed change). Here is why I think this is necessary: when ocamlfind is not installed, then the Makefile uses -I +lablgtk3 -I +cairo2 as options to tell the compiler where lablgtk3 should be found. This means that lablgtk3 should be found in the directory of the standard library. So if ocamlfind is not availbale, we should not search for it lablgtk3 in $(OCAMLLIBDIR)/../lablgtk3, because then we will not give the right options to the ocaml compiler.

With this change:

gdt commented 4 months ago

I suspect this is pretty complicated and would like to go very slow on actually doing anything.

The basic issues are that:

Overall, I lean to one of

I think the biggest question is how normal is ocamlfind in the ocaml world. Would people say that a system wiith ocamlc on which it is difficult to install ocaml-find basically broken? Or is it considered fringe? Has anyone really had trouble installing it? In pkgsrc it is trivial, and I'd expect that in other packaging systems. Philosophically I am not concerned about LTS; if people want to run old software from 2014, they can run unison from 2014 too :-) But seriously, supporting modern unison on old LTS is the responsibility of LTS distirbutors, not the unison project. It's not fair to expect us to do work because other people choose to run old software.

If one wrote "ad hoc scripts to find things", would they be re-implementing ocamlfind? I would think so.

In general I do not have a problem with requiring normal tools that many other things depend on.

Another question is: If ocamlc is installed to $prefix1, are we willing to require that lablgtk3 and others are installed to that same $prefix1? Or can they be installed to $prefix2? Is that true if unison is built to $prefix3?

(I should add that I am unhappy with the "optional ocamfind" aspect of what is in the repo now, but I merged it because I had not gotten to doing something different and I didn't want to hold things up.)

jhjourdan commented 4 months ago

First of all, the change I am proposing in the makefile is really just a bugfix. I don't think there is any system where a build used to work before this patch and fails after this patch. The reason is that the lines I'm proposing to remove detects lablgtk3 in a path where the rest of the makefile is not able to use it. This is really a small change very well in the spirit of " going very slow on actually doing anything".

Now, let's try to answer your questions, to the best of my knowledge (I'm not really an expert on these OCaml portability questions).

I think the biggest question is how normal is ocamlfind in the ocaml world. Would people say that a system wiith ocamlc on which it is difficult to install ocaml-find basically broken? Or is it considered fringe? Has anyone really had trouble installing it?

Before dune was popular, ocamlfind used to be a very, very common dependency for OCaml projects. It works on most platforms, has no dependencies apart from OCaml. I am not used to exotic platforms, so I can't say whether some people had problems installing it on some unusual platform, but it is fairly standard. lablgtk3 is a much stronger dependency, I expect that any system supporting lablgtk3 would easily support ocamlfind.

Another question is: If ocamlc is installed to $prefix1, are we willing to require that lablgtk3 and others are installed to that same $prefix1? Or can they be installed to $prefix2? Is that true if unison is built to $prefix3?

All this should be compatible with ocamlfind. It can be configured to search for packages in various directories.

gdt commented 4 months ago

Thanks for the explanation. I have come around to seeing this as a bugfix, by removing a bit of magic that gets things wrong. So I am likely to merge this after a bit in case anyone else wants to comment.

What I think I'd like to do is at least add a big scary warning that ocamlfind was not present, even if it finds labl3gtk in the ocaml libdir. And then to make the gui a requested rather than discovered option, and fail hard if not found. But that's entirely disjoint from your change, and it's not reasonable to hold it up.

What you found is exactly why I'm uncomfortable with ad hoc looking for things rather than using the standard appoaches.

jhjourdan commented 4 months ago

I agree on all this!

jhjourdan commented 4 months ago

I pushed other changes of the opam files to make them compatible with installations of ocaml that do not include ocamlopt.

gdt commented 4 months ago

@tleedjarv Are you ok with this?

@jhjourdan Could you amend the commit message to give the rationale for the change? I would like it understandable from the commit alone, because that's IMHO the right approach. Also the PR history is now complicated (a lot of that's my fault) and when we migrate from github I do not expect to retain it. The ocamlopt/native stuff could use a comment; I think it's querying how the compiler was built and building native if supported and bytecode not but it would be good for someone who really understands that to say what it does. And the discussion about how the "look for things here, but flags won't match, so this is buggy" belongs, even if it's only the one crisp sentence. Sorry to be difficult, but future me will want to read this years later when I wonder why.

jhjourdan commented 4 months ago

Could you amend the commit message to give the rationale for the change?

Sure, done. You're right that I was too lazy in my commit message.

tleedjarv commented 4 months ago

First of all, thank you @jhjourdan, for looking into making the build system more robust. As explained a bit more below, I think it is very important that the build process "just works" for as many people as possible.

That being said, I had a deeper look at this, and I'd still like to change the Makefile a bit, because I still think it is bogus (I've just pushed my proposed change). Here is why I think this is necessary: when ocamlfind is not installed, then the Makefile uses -I +lablgtk3 -I +cairo2 as options to tell the compiler where lablgtk3 should be found. This means that lablgtk3 should be found in the directory of the standard library. So if ocamlfind is not availbale, we should not search for it lablgtk3 in $(OCAMLLIBDIR)/../lablgtk3, because then we will not give the right options to the ocaml compiler.

Then I don't understand how it could have worked before. Please note that this code was added at one time precisely due to opam, see #114 . You can remove this code if you can confirm that it is no longer needed or that it really does not work. I do not think ocamlfind should be a mandatory requirement, if we can build without it. Building without ocamlfind works without opam. As I understand it, this code was added for building without ocamlfind with opam. Is that no longer relevant at all (ocamlfind is mandatory now)?

I can't comment on changes to opam files; I think here you know much better than I do. I still find it weird that ocamlfind would be a mandatory dependency, but I also think that lablgtk3 is different from lablgtk2 in that respect, so maybe that's the reason.

Not directly related to this PR, but as for the general discussion, I don't necessarily agree with @gdt. For years (decades, by now), people have been searching for pre-built binaries because they thought that the build process and/or build dependencies were complex and difficult to operate. I thought so too for many years. Only recently did I find out that building Unison was actually an extremely simple task.

Packagers have much more knowledge about the build environment (and tools) and can control the build process by way of (env) variables and build targets. Regular users should not have to deal with any of that. For them, the build process should be as automated as possible and have as few prerequisites as possible. I do acknowledge that rarely anyone still builds from source, so I'm not too opposed to changing the build system if there is clear motivation.

Additionally, not entirely sure but I think that OCaml compiler and tooling ecosystem is slowly moving towards supporting Windows as a build platform. Even though the current makefile depends on POSIX tooling, that can be easily changed. Even though there are very few Windows-native contributors right now, I wouldn't like seeing changes to build system that make it even harder or nearly impossible to natively support Windows in some unknown future.

jhjourdan commented 4 months ago

Then I don't understand how it could have worked before. Please note that this code was added at one time precisely due to opam, see https://github.com/bcpierce00/unison/pull/114 . You can remove this code if you can confirm that it is no longer needed or that it really does not work.

This code was added in #114 to detect lablgtk2 under Opam. This PR links to the other PR #95 where it is explicitly said that ocamlfind was needed at that time to build unison under opam. So, at that time, lablgtk was detected manually in $(OCAMLLIBDIR)/../lablgtk3, but ocamlfind was used to pass the right options to the compiler. That's a bit weird hybrid way of doing things, but that's the way it used to work at that time.

Nowadays, we first try to detect lablgtk3 using ocamlfind if it exists (which we did not do at the time of #114), and then use ocamlfind to pass the right options to the compiler. If ocamlfind is available, the we only search for it in $(OCAMLLIBDIR), because that's the place we assume when passing parameters manually to the compilers afterwards.

So I can confirm this piece of code does not work today, and if lablgtk3 is not installed under $(OCAMLLIBDIR), then ocamlfind is needed to build unison, and has (I think) always been needed.

I do not think ocamlfind should be a mandatory requirement, if we can build without it.

Well, we can have a more complex Makefile that does the work of ocamlfind instead of requiring it. But ocamlfind is really not an heavy dependency, especially under opam, which is going to install it without effort from the user.

Building without ocamlfind works without opam.

I think that's wrong. Building unison GUI without ocamlfind was not possible at the time of #114. The reason why ocamlfind was not an unison dependency at that time is that it was a lablgtk2 dependency, so ocamlfind was actually a transitive dependency of unison. Nowadays, ocamlfind is no longer a dependency of lablgtk (because dune may do the work of ocamlfind, so one can use lablgtk without ocamlfind), so we need to add it explicitly as a dependency of unison.

Regular users should not have to deal with any of that. For them, the build process should be as automated as possible and have as few prerequisites as possible.

Right. I think this patch goes in that direction, since build was failing in a (somewhat standard) situation, and is now succeeding. Of, in that case it does not build the GUI, but it will do if ocamlfind is installed, and ocamlfind is documented as a dependency of the GUI.

Additionally, not entirely sure but I think that OCaml compiler and tooling ecosystem is slowly moving towards supporting Windows as a build platform. Even though the current makefile depends on POSIX tooling, that can be easily changed. Even though there are very few Windows-native contributors right now, I wouldn't like seeing changes to build system that make it even harder or nearly impossible to natively support Windows in some unknown future.

I can only agree. I can't test windows support of the opam packages I'm now maintaining, but I don't see how this change can make it more difficult to build unison under Windows. Ocamlfind is supported under Windows.

tleedjarv commented 4 months ago

This code was added in #114 to detect lablgtk2 under Opam. This PR links to the other PR #95 where it is explicitly said that ocamlfind was needed at that time to build unison under opam. So, at that time, lablgtk was detected manually in $(OCAMLLIBDIR)/../lablgtk3, but ocamlfind was used to pass the right options to the compiler. That's a bit weird hybrid way of doing things, but that's the way it used to work at that time.

I see, this makes perfect sense. No objections to removing this code.

Building without ocamlfind works without opam.

I think that's wrong. Building unison GUI without ocamlfind was not possible at the time of #114.

"without opam" was the condition here. That works. I now understand that it didn't and couldn't work under opam, so your fix is fine with me.

Additionally, not entirely sure but I think that OCaml compiler and tooling ecosystem is slowly moving towards supporting Windows as a build platform. Even though the current makefile depends on POSIX tooling, that can be easily changed. Even though there are very few Windows-native contributors right now, I wouldn't like seeing changes to build system that make it even harder or nearly impossible to natively support Windows in some unknown future.

I can only agree. I can't test windows support of the opam packages I'm now maintaining, but I don't see how this change can make it more difficult to build unison under Windows. Ocamlfind is supported under Windows.

This comment was not directed at the PR but rather the general discussion by @gdt.

jhjourdan commented 4 months ago

"without opam" was the condition here. That works. I now understand that it didn't and couldn't work under opam, so your fix is fine with me.

Ah, indeed, sorry, I misread. Building indeed works without opam and ocamlfind, provided lablgtk3 is installed in $(OCAMLLIBDIR)/lablgtk3, which is a rather strong assumption. This will continue to work after this patch.

tleedjarv commented 4 months ago

provided lablgtk3 is installed in $(OCAMLLIBDIR)/lablgtk3, which is a rather strong assumption.

I think it's the case for many/most packaging systems (and this is what we're effectively talking about when saying "without opam"). I took the time to check a few package distros and see if they install lablgtk3 in the said location, and they do:

jhjourdan commented 4 months ago

Indeed you're right, and I'm a bit surprised that opam does otherwise then. Who knows...

gdt commented 4 months ago

So this is ok to merge?

jhjourdan commented 4 months ago

Sure.

gdt commented 4 months ago

Thanks. going to wait at least a week anyway because we just had a release and I like to have zero changes in case we need a new micro for a bad bug.

gdt commented 4 months ago

post-release hold has expired. @jhjourdan thank you for your patience!

jhjourdan commented 4 months ago

Thanks!