Page MenuHomePhabricator

cmake: Install the OCaml libraries into a more correct path
ClosedPublic

Authored by mgorny on Sep 8 2016, 10:32 AM.

Details

Summary

Add a OCAML_INSTALL_PATH variable that can be used to control
the install path for OCaml libraries. The new variable defaults to
${OCAML_STDLIB_PATH}, i.e. the OCaml library path obtained from
the OCaml compiler. Install libraries into "llvm" subdirectory.

This fixes two issues:

  1. OCaml library directories differ between systems, and 'lib/ocaml' is

incorrect e.g. on amd64 Gentoo where OCaml is installed
in 'lib64/ocaml'. Therefore, obtain the library path from the OCaml
compiler using 'ocamlc -where' (which is already used to set
OCAML_STDLIB_PATH), which is the method used commonly in OCaml packages.

  1. The top-level directory is reserved for the standard library, and has

precedence over local directory in search path. As a result, OCaml
preferred the files installed along with previous LLVM version over the
source tree when building a new version, resulting in two versions being
mixed during the build. The new layout is used commonly by other OCaml
packages, and findlib is able to find the LLVM libraries successfully.

Bug: https://bugs.gentoo.org/559134
Bug: https://bugs.gentoo.org/559624

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 70722.Sep 8 2016, 10:32 AM
mgorny retitled this revision from to cmake: Use system-obtained OCaml library path when installing.
mgorny updated this object.
mgorny added a reviewer: whitequark.
mgorny added a subscriber: llvm-commits.
whitequark edited edge metadata.Sep 8 2016, 10:42 AM

But doesn't OCAML_STDLIB_PATH include /usr? So you would be installing to /usr/local/usr/lib/ocaml with the default config, unless I'm missing something.

mgorny updated this revision to Diff 70729.Sep 8 2016, 11:10 AM
mgorny retitled this revision from cmake: Use system-obtained OCaml library path when installing to cmake: Install the OCaml libraries into a more correct path.
mgorny updated this object.
mgorny edited edge metadata.
mgorny updated this object.

But doesn't OCAML_STDLIB_PATH include /usr? So you would be installing to /usr/local/usr/lib/ocaml with the default config, unless I'm missing something.

Yes, it does. However, CMake uses the whole path when it is an absolute path, i.e. OCAML_INSTALL_PATH=/foo/bar -> installs to /foo/bar; OCAML_INSTALL_PATH=foo/bar -> install to ${CMAKE_INSTALL_PREFIX}/foo/bar.

Where does the META files are going to be installed ? $LLVM_LIBRARY_DIR/ocaml or $OCAML_INSTALL_PATH ?

Where does the META files are going to be installed ? $LLVM_LIBRARY_DIR/ocaml or $OCAML_INSTALL_PATH ?

Oh, I see now that my newest patch misses that. I'm going to update it in a few minutes.

Do you think it would be also a good idea to move the OCAML_INSTALL_PATH declaration elsewhere?

Well, I did an opam package with a few patches if you want to know what I'd personally expect: https://github.com/ocaml/opam-repository/pull/7346

mgorny updated this revision to Diff 70740.Sep 8 2016, 12:38 PM

Updates:

  1. extended to cover all 'lib/ocaml' uses,
  2. renamed OCAML_INSTALL_PATH to LLVM_OCAML_INSTALL_PATH,
  3. moved LLVM_OCAML_INSTALL_PATH to bindings/ocaml since that seems to be the most logical top-level place for it.

This patch won't work because the META files should be installed in ${OCAML_STDLIB_PATH}.
Otherwise ocamlfind won't be able to locate them. You also have to modify the directory field in the META files accordingly.

ocamlfind is looking for a "META" file in subdirectories so META.llvm (for example) is not handled.
META files in the form "META.thing" are handled only in the base directory.

mgorny planned changes to this revision.Sep 8 2016, 1:22 PM

ocamlfind is looking for a "META" file in subdirectories so META.llvm (for example) is not handled.
META files in the form "META.thing" are handled only in the base directory.

Thanks for explaning this. I was confused because other packages installed META files in the subdirectories.

Is there a reason to use multiple META.* files then? Considering that installing the files in a subdirectory is necessary anyway, I think it might be cleaner to just combine and install one META file for all of them. This will also make the logic simpler since it would be limited to a single flexible installation path with no need to reference parent/subdirectory structure.

Of course, another option is to just make the top directory configurable, and always create 'llvm' in it. I think this is the solution you were proposing, correct?

Is there a reason to use multiple META.* files then? Considering that installing the files in a subdirectory is necessary anyway, I think it might be cleaner to just combine and install one META file for all of them. This will also make the logic simpler since it would be limited to a single flexible installation path with no need to reference parent/subdirectory structure.

Of course, another option is to just make the top directory configurable, and always create 'llvm' in it. I think this is the solution you were proposing, correct?

It is cleaner I agree, but it breaks backward compatibility. So I would say it should stay as is.
And yes I think the best solution would be to make the top directory configurable, to install the METAs in it and to install the rest in the 'llvm' subdir. That would be perfect ! :)

mgorny updated this revision to Diff 70803.Sep 9 2016, 3:16 AM
mgorny updated this object.

Hopefully this one would be 100% good ;-). Now the top directory is being configured, and META.* files are installed there. The libraries are installed unconditionally in "llvm" subdirectory, and it is also specified in META.* files.

I've tested this patch properly, and can confirm that ocamlfind sees all LLVM packages.

whitequark added inline comments.Sep 9 2016, 3:19 AM
bindings/ocaml/CMakeLists.txt
2 ↗(On Diff #70803)

OCaml; also I think this should go into config-ix.cmake

mgorny updated this revision to Diff 70808.Sep 9 2016, 3:29 AM
mgorny marked an inline comment as done.

@jpdeplaix Can you please also test the changes? Once I get ACK from you I will commit.

It works well. I updated my pull request.

It's off-topic but I also noticed that .mli are not installed. Could either one of you fix it ?

It's off-topic but I also noticed that .mli are not installed. Could either one of you fix it ?

Sure, I'll look at it. Though note that I'm not really using OCaml myself, just doing what people who use it ask me to do ;-D.

By the way, I appreciate your effort on trying to make OCaml buildable stand-alone. I'll probably end up submitting a patch similar to yours; however, I don't really like the idea of having magical variables to partially disable parts of CMake. I'm wondering how to handle this better. Maybe it'd be a good idea to make bindings/ocaml CMakeFiles.txt runnable as stand-alone project -- like clang or compiler-rt support nowadays.

mgorny added a comment.EditedSep 10 2016, 9:11 AM

It's off-topic but I also noticed that .mli are not installed. Could either one of you fix it ?

Are you sure about that, actually?

# qlist llvm | grep cmi$
/usr/lib64/ocaml/llvm/llvm.cmi
/usr/lib64/ocaml/llvm/llvm_AMDGPU.cmi
/usr/lib64/ocaml/llvm/llvm_BPF.cmi
/usr/lib64/ocaml/llvm/llvm_X86.cmi
[...]

Oh, sorry, disregard my comment. I must have trouble seeing ;-).

mgorny planned changes to this revision.Sep 20 2016, 3:59 AM

This needs more work. After some more testing, I've noticed the subdirectory change breaks tests. I need to update the output directory layout to match the new subdirectory, and update RPATHs for tests. I'll also make it respect LLVM_LIBDIR_SUFFIX.

mgorny updated this revision to Diff 71923.Sep 20 2016, 6:28 AM

Updated with a new diff that fixes output directory structure to fit META.* directory statements, and accordingly updates OCaml paths and rpaths for doc build and tests.

@jpdeplaix can you please test the updated patch also?

cmake/modules/AddOCaml.cmake
214 ↗(On Diff #71923)

""

214 ↗(On Diff #71923)

This also needs to have a reverse dependency, or else you'll break make -j.

mgorny added inline comments.
cmake/modules/AddOCaml.cmake
214 ↗(On Diff #71923)

I'm sorry but I don't understand you. What kind of a reverse dependency? The ocaml_* targets depend on ocaml_make_directory so that the directory will be created before any of the POST_BUILD commands are executed. Is there anything else that needs to be covered?

whitequark added inline comments.Sep 20 2016, 12:15 PM
cmake/modules/AddOCaml.cmake
214 ↗(On Diff #71923)

Sorry, disregard that one. I have missed that you added a dependency already.

mgorny marked 4 inline comments as done.Sep 20 2016, 1:04 PM

@whitequark, I'm sorry if I'm a little impatient but could we proceed with the review? I think @jpdeplaix isn't around for a while, and I'd like to have a clean tree to proceed with the other changes he requested ;-).

Side note: I have commit access already, so I can commit on my own.

whitequark accepted this revision.Sep 30 2016, 10:41 AM
whitequark edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 30 2016, 10:41 AM
This revision was automatically updated to reflect the committed changes.

Thanks a lot!