cmake: Install the OCaml libraries into a more correct path

[mgorny]

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.

2. 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

[whitequark]

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]

In D24354#537352, @whitequark wrote:

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.

[jpdeplaix]

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

[mgorny]

In D24354#537445, @jpdeplaix wrote:

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?

[jpdeplaix]

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]

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.

[jpdeplaix]

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.

[jpdeplaix]

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]

In D24354#537571, @jpdeplaix wrote:

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?

[jpdeplaix]

In D24354#537592, @mgorny wrote:

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]

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]

bindings/ocaml/CMakeLists.txt

2 ↗

(On Diff #70803)

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

[whitequark]

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

[jpdeplaix]

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 ?

[mgorny]

In D24354#539322, @jpdeplaix wrote:

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]

In D24354#539322, @jpdeplaix wrote:

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
[...]

[mgorny]

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

[mgorny]

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 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.

[whitequark]

@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]

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]

cmake/modules/AddOCaml.cmake

214 ↗

(On Diff #71923)

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

[mgorny]

@jpdeplaix, ping.

[mgorny]

@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]

LGTM

[mgorny]

Thanks a lot!

llvm/trunk/bindings/ocaml/backends/CMakeLists.txt

add_ocaml_library(llvm_${TARGET}
     LLVM     ${TARGET}
     NOCOPY)
 
   configure_file(
     "${CMAKE_CURRENT_SOURCE_DIR}/META.llvm_backend.in"
     "${LLVM_LIBRARY_DIR}/ocaml/META.llvm_${TARGET}")
 
   install(FILES "${LLVM_LIBRARY_DIR}/ocaml/META.llvm_${TARGET}"
-          DESTINATION lib/ocaml)
+          DESTINATION "${LLVM_OCAML_INSTALL_PATH}")
 endforeach()

llvm/trunk/bindings/ocaml/backends/META.llvm_backend.in

 name = "llvm_@TARGET@"
 version = "@PACKAGE_VERSION@"
 description = "@TARGET@ Backend for LLVM"
 requires = "llvm"
 archive(byte) = "llvm_@TARGET@.cma"
 archive(native) = "llvm_@TARGET@.cmxa"
-directory = "."
+directory = "llvm"

llvm/trunk/bindings/ocaml/llvm/CMakeLists.txt

 add_ocaml_library(llvm
   OCAML llvm
   C     llvm_ocaml
   LLVM  core support)
 
 configure_file(
   "${CMAKE_CURRENT_SOURCE_DIR}/META.llvm.in"
   "${LLVM_LIBRARY_DIR}/ocaml/META.llvm")
 
 install(FILES "${LLVM_LIBRARY_DIR}/ocaml/META.llvm"
-        DESTINATION lib/ocaml)
+        DESTINATION "${LLVM_OCAML_INSTALL_PATH}")

llvm/trunk/bindings/ocaml/llvm/META.llvm.in

 name = "llvm"
 version = "@PACKAGE_VERSION@"
 description = "LLVM OCaml bindings"
 archive(byte) = "llvm.cma"
 archive(native) = "llvm.cmxa"
-directory = "."
+directory = "llvm"
 
 package "analysis" (
     requires = "llvm"
     version = "@PACKAGE_VERSION@"
     description = "Intermediate representation analysis for LLVM"
     archive(byte) = "llvm_analysis.cma"
     archive(native) = "llvm_analysis.cmxa"
 )

llvm/trunk/cmake/config-ix.cmake

else()
     if( OCAML_VERSION VERSION_LESS "4.00.0" )
       message(STATUS "OCaml bindings disabled, need OCaml >=4.00.0.")
     else()
       find_ocamlfind_package(ctypes VERSION 0.4 OPTIONAL)
       if( HAVE_OCAML_CTYPES )
         message(STATUS "OCaml bindings enabled.")
         find_ocamlfind_package(oUnit VERSION 2 OPTIONAL)
         set(LLVM_BINDINGS "${LLVM_BINDINGS} ocaml")
+
+        set(LLVM_OCAML_INSTALL_PATH "${OCAML_STDLIB_PATH}" CACHE STRING
+            "Install directory for LLVM OCaml packages")
       else()
         message(STATUS "OCaml bindings disabled, need ctypes >=0.4.")
       endif()
     endif()
   endif()
 endif()
 
 string(REPLACE " " ";" LLVM_BINDINGS_LIST "${LLVM_BINDINGS}")

llvm/trunk/cmake/modules/AddOCaml.cmake

function(add_ocaml_library name)
   endif()
   if( HAVE_OCAMLOPT )
     list(APPEND ocaml_outputs
          "${bin}/${name}.cmxa"
          "${bin}/${name}${CMAKE_STATIC_LIBRARY_SUFFIX}")
   endif()
 
   set(ocaml_flags "-lstdc++" "-ldopt" "-L${LLVM_LIBRARY_DIR}"
-                  "-ccopt" "-L\\$CAMLORIGIN/.."
-                  "-ccopt" "-Wl,-rpath,\\$CAMLORIGIN/.."
+                  "-ccopt" "-L\\$CAMLORIGIN/../.."
+                  "-ccopt" "-Wl,-rpath,\\$CAMLORIGIN/../.."
                   ${ocaml_pkgs})
 
   foreach( ocaml_dep ${ARG_OCAMLDEP} )
     get_target_property(dep_ocaml_flags "ocaml_${ocaml_dep}" OCAML_FLAGS)
     list(APPEND ocaml_flags ${dep_ocaml_flags})
   endforeach()
 
   if( NOT BUILD_SHARED_LIBS )
function(add_ocaml_library name)
 
   set(ocaml_params)
   foreach( ocaml_input ${ocaml_inputs} ${c_outputs})
     get_filename_component(filename "${ocaml_input}" NAME)
     list(APPEND ocaml_params "${filename}")
   endforeach()
 
   if( APPLE )
-    set(ocaml_rpath "@executable_path/../../lib")
+    set(ocaml_rpath "@executable_path/../../../lib${LLVM_LIBDIR_SUFFIX}")
   elseif( UNIX )
-    set(ocaml_rpath "\\$ORIGIN/../../lib")
+    set(ocaml_rpath "\\$ORIGIN/../../../lib${LLVM_LIBDIR_SUFFIX}")
   endif()
   list(APPEND ocaml_flags "-ldopt" "-Wl,-rpath,${ocaml_rpath}")
 
   add_custom_command(
     OUTPUT ${ocaml_outputs}
     COMMAND "${OCAMLFIND}" "ocamlmklib" "-o" "${name}" ${ocaml_flags} ${ocaml_params}
     DEPENDS ${ocaml_inputs} ${c_outputs}
     COMMENT "Building OCaml library ${name}"
     VERBATIM)
 
   add_custom_command(
     OUTPUT "${bin}/${name}.odoc"
     COMMAND "${OCAMLFIND}" "ocamldoc"
             "-I" "${bin}"
-            "-I" "${LLVM_LIBRARY_DIR}/ocaml/"
+            "-I" "${LLVM_LIBRARY_DIR}/ocaml/llvm/"
             "-dump" "${bin}/${name}.odoc"
             ${ocaml_pkgs} ${ocaml_inputs}
     DEPENDS ${ocaml_inputs} ${ocaml_outputs}
     COMMENT "Building OCaml documentation for ${name}"
     VERBATIM)
 
   add_custom_target("ocaml_${name}" ALL DEPENDS ${ocaml_outputs} "${bin}/${name}.odoc")
 
if( NOT (ext STREQUAL ".cmo" OR
              ext STREQUAL CMAKE_SHARED_LIBRARY_SUFFIX) )
       list(APPEND install_files "${ocaml_output}")
     elseif( ext STREQUAL CMAKE_SHARED_LIBRARY_SUFFIX)
       list(APPEND install_shlibs "${ocaml_output}")
     endif()
   endforeach()
 
   install(FILES ${install_files}
-          DESTINATION lib/ocaml)
+          DESTINATION "${LLVM_OCAML_INSTALL_PATH}/llvm")
   install(FILES ${install_shlibs}
           PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE
                       GROUP_READ GROUP_EXECUTE
                       WORLD_READ WORLD_EXECUTE
-          DESTINATION lib/ocaml)
+          DESTINATION "${LLVM_OCAML_INSTALL_PATH}/llvm")
 
   foreach( install_file ${install_files} ${install_shlibs} )
     get_filename_component(filename "${install_file}" NAME)
     add_custom_command(TARGET "ocaml_${name}" POST_BUILD
       COMMAND "${CMAKE_COMMAND}" "-E" "copy" "${install_file}"
-                                             "${LLVM_LIBRARY_DIR}/ocaml/"
+                                             "${LLVM_LIBRARY_DIR}/ocaml/llvm/"
       COMMENT "Copying OCaml library component ${filename} to intermediate area"
       VERBATIM)
+    add_dependencies("ocaml_${name}" ocaml_make_directory)
   endforeach()
 endfunction()
 
+add_custom_target(ocaml_make_directory
+  COMMAND "${CMAKE_COMMAND}" "-E" "make_directory" "${LLVM_LIBRARY_DIR}/ocaml/llvm")
 add_custom_target("ocaml_all")
-set_target_properties(ocaml_all PROPERTIES FOLDER "Misc")
+set_target_properties(ocaml_all PROPERTIES FOLDER "Misc")
 

llvm/trunk/test/lit.cfg

 
 # Set up OCAMLPATH to include newly built OCaml libraries.
 llvm_lib_dir = getattr(config, 'llvm_lib_dir', None)
 if llvm_lib_dir is None:
     if llvm_obj_root is not None:
         llvm_lib_dir = os.path.join(llvm_obj_root, 'lib')
 
 if llvm_lib_dir is not None:
-    llvm_ocaml_lib = os.path.join(llvm_lib_dir, 'ocaml')
+    top_ocaml_lib = os.path.join(llvm_lib_dir, 'ocaml')
+    llvm_ocaml_lib = os.path.join(top_ocaml_lib, 'llvm')
     if llvm_ocaml_lib is not None:
+        ocamlpath = os.path.pathsep.join((llvm_ocaml_lib, top_ocaml_lib))
         if 'OCAMLPATH' in os.environ:
-            ocamlpath = os.path.pathsep.join((llvm_ocaml_lib, os.environ['OCAMLPATH']))
-            config.environment['OCAMLPATH'] = ocamlpath
-        else:
-            config.environment['OCAMLPATH'] = llvm_ocaml_lib
+            ocamlpath = os.path.pathsep.join((ocamlpath, os.environ['OCAMLPATH']))
 
         if 'CAML_LD_LIBRARY_PATH' in os.environ:
             caml_ld_library_path = os.path.pathsep.join((llvm_ocaml_lib,
                                         os.environ['CAML_LD_LIBRARY_PATH']))
             config.environment['CAML_LD_LIBRARY_PATH'] = caml_ld_library_path
         else:
             config.environment['CAML_LD_LIBRARY_PATH'] = llvm_ocaml_lib