-
Notifications
You must be signed in to change notification settings - Fork 237
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
OCamlfind is required to build the GUI with lablgtk3. #1002
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that your issues could be with "lablgtk3" and not with unison.
Don't change the makefile, it's used for builds without opam, too. Perhaps a broken lablgtk3 installation is causing your issue?
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?
src/Makefile.OCaml
Outdated
else | ||
LABLGTK3LIB=$(OCAMLLIBDIR)/lablgtk3 | ||
ifeq ($(wildcard $(LABLGTK3LIB)),$(LABLGTK3LIB)) | ||
HAS_LABLGTK3=true | ||
else | ||
LABLGTK3LIB=$(abspath $(OCAMLLIBDIR)/../lablgtk3) | ||
ifeq ($(wildcard $(LABLGTK3LIB)),$(LABLGTK3LIB)) | ||
HAS_LABLGTK3=true | ||
endif | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7ba1840
to
3076a1b
Compare
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.
Ok, then I'm sorry. Given that 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 With this change:
|
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? 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.) |
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).
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.
All this should be compatible with ocamlfind. It can be configured to search for packages in various directories. |
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. |
I agree on all this! |
3076a1b
to
eccb42a
Compare
I pushed other changes of the opam files to make them compatible with installations of ocaml that do not include |
@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. |
…Fix Opam packages when no native compiler exists. This commit does 3 things: - We no longer try to detect lablgtk3 when it's not in the standard library directory ($(OCAMLLIBDIR)) and ocamlfind is not available. The previous code was trying to detect lablgtk3 in $(OCAMLLIBDIR)/.., but then the options passed to the OCaml compiler were wrong in this situation, and the build failed. By removing this code, we make sure the build succeeds in this situation, though GUI support is dropped. If the user wants GUI support and lablgtk3 is not in $(OCAMLLIBDIR), then she can install ocamlfind which will do the job right. - We add ocamlfind as a dependency of the unison-gui Opam package, and as an optional dependency of the unison Opam package. This reflects the fact that, under Opam, ocamlfind is required to find lablgtk3. - In the Opam package, support for the NATIVE variable which, when set to false, tells the Makefile to use ocamlc instead of ocamlopt. This is to be used on Opam install that do not have ocamlopt installed.
eccb42a
to
d52f6f4
Compare
Sure, done. You're right that I was too lazy in my commit message. |
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.
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. |
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 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 So I can confirm this piece of code does not work today, and if lablgtk3 is not installed under
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.
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.
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.
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. |
I see, this makes perfect sense. No objections to removing this code.
"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.
This comment was not directed at the PR but rather the general discussion by @gdt. |
Ah, indeed, sorry, I misread. Building indeed works without opam and ocamlfind, provided lablgtk3 is installed in |
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:
|
Indeed you're right, and I'm a bit surprised that opam does otherwise then. Who knows... |
So this is ok to merge? |
Sure. |
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. |
post-release hold has expired. @jhjourdan thank you for your patience! |
Thanks! |
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:
This patch deactivates GUI compilation if ocamlfind is not installed (the build will fail anyways), and add the corresponding clauses in the opam files.