[llvm] [cmake] Fix finding modern ounit2

[mgorny]

Summary

Apparently modern versions of ounit2 can only be found as "ounit2"
rather than "oUnit" version 2. Update the CMake check to support both
variants. This makes the OCaml tests run again with ounit2-2.2.4.

[mgorny]

@jberdine, you seem to be the only person touching OCaml "recently". Any chance you'd be able to review this?

[jberdine]

I am not so familiar with cmake and ocamlfind, what do you think @arbipher? @vaivaswatha?

[vaivaswatha]

This change looks fine to me.

[jberdine]

Thanks both!

[mgorny]

Thanks!

[arbipher]

Hi all, I am happy to help with OCaml-related problems for this one and the future.
It seems I am a bit late to reply. I saw the email but didn't find a time to check this immediately at that moment until today.

I played with the ocaml binding several times before but not with the test.
@mgorny could you share the steps to run these ocaml tests against ounit2-2.2.4?

I tried opam install llvm --with-test, which uses llvm 13.0.1 and ounit2-2.2.5 on opam and it works.
This installation shows the unpatched cmake file works well with the ounit2 2.2.5. I would like to know the non-working scenario with your 2.2.4 without the patch.

The renaming of ounit2 occurs at 2.2.0 in 2019. See the release note.
I am thinking maybe we could just support ounit2 in the trunk.

[mgorny]

I'm not really using OCaml — I'm just maintaining LLVM in Gentoo, and since someone before me added the OCaml bindings, I just keep them working.

I was using ounit2 installed from Gentoo packages — perhaps opam is doing something to preserve backwards compatibility that we aren't doing.

I personally don't mind removing support for old versions but I don't think I should be the one to decide that.

[arbipher]

I see. Thanks for the explanation. I was opam-minded before and now the problem looks more clear to me.

I am ok with the patch.

The following is some of my understanding.

The library ounit now contains two packages:

• ounit2 is the canonical one and its name on opam is ounit2.
• oUnit is for transition and its name on opam is ounit.

ocamlfind/findlib is a package-format tool and library.
opam is a package-distribution tool and repo. It's widely used in the ocaml community.
Packages can also be managed by other package managers, as shown in your Gentoo cases. It varies that how package managers are dealing with the name for ounit2 in other linux distributions.
For this aspect, I don't have a better solution.

As for llvm's ocaml binding, I don't see how ounit is actually used. I don't find it in the source of the binding, nor in the test llvm/test/Bindings/OCaml/.
llvm/test/Bindings/OCaml/lit.local.cfg refers to config.root.have_ocaml_ounit but no further used in it. I can revisit this issue and make another PR after realizing its usage later.

[mgorny]

That entry in lit.local.cfg.py makes lit mark all OCaml binding tests as unsupported if ounit is not present.

[arbipher]

I saw that. My concern is the tests themselves don't use ounit.

In my checking, the tests llvm/test/Bindings/OCaml/<test>.ml uses the ocaml binaries ocamlc ... ocamlopt ... but no ounit.
There is also no ounit tests in the binding's source code (llvm/bindindings/ocaml) (checked with the ocaml community but we can wait for some time)

btw, I think it's fine to keep it as it is. I will need more time to investigate it.

[mgorny]

Heh, this possibility didn't even occur to me. I suppose the easiest way to check would be to remove ounit2, remove the lit restriction and see if tests still pass.

[jberdine]

Heh, yeah, it seems that the best fix would be to just remove the traces of ounit. I also don't see where they are used, so just removing and seeing if the tests pass in a context with no ounit installed seems like a good plan.

[mgorny]

In D119079#3322700, @jberdine wrote:

Heh, yeah, it seems that the best fix would be to just remove the traces of ounit. I also don't see where they are used, so just removing and seeing if the tests pass in a context with no ounit installed seems like a good plan.

I've just uninstalled ounit2 and all its dependencies, removed it from cmake/lit and it seems that all tests pass. I'll prepare a merge request.

[arbipher]

Sure. It also sounds good to me.

Could you share how you run the OCaml binding tests inside the LLVM?

I may work on a new release on llvm-ocaml binding on opam later. I am thinking to re-run these testing after installing the opam package.
Since there are some patches for linking in the installation so the files are not the same. I haven't prepared all the necessary scripts for that.

[mgorny]

It's basically https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-ml/llvm-ocaml/llvm-ocaml-15.0.0.9999.ebuild#n41.

Note that I'm building it on top of already-installed LLVM. It involves some hacks already present in CMake to avoid building other parts of LLVM etc. Plus a hack to symlink system .so files into build dir's lib/.

Then it's just:

make ocaml_all
make check-llvm-bindings-ocaml
DESTDIR="${D}" cmake -P "${BUILD_DIR}"/bindings/ocaml/cmake_install.cmake

Replace make with ninja as appropriate. I think modern cmake has some wrapper for calling these two, if your prefer.

[arbipher]

Thanks! I will learn from your script and test which flags help much in the opam building.

I saw a note in it on _TODO: figure out a way to pass -L to ocaml..._.
I once worked very hard to solve a similar question in ocaml-z3. I am thinking I could be able to solve it but just need some free time.

llvm/cmake/config-ix.cmake

if( NOT OCAML_FOUND )
     message(STATUS "OCaml bindings disabled.")
   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(ounit2 OPTIONAL)
+        if ( HAVE_OCAML_OUNIT2 )
+          set(HAVE_OCAML_OUNIT TRUE)
+        else()
           find_ocamlfind_package(oUnit VERSION 2 OPTIONAL)
+        endif()
         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()