llvm.executionengine only use ctypes, not ctypes-foreign

[kit-ty-kate]

cc @alan-j-hu This seems to be wrongly defined in llvm/bindings/ocaml/llvm/META.llvm.in, as the package itself only requires ctypes but the META file requires ctypes.foreign for some unknown reason.

Looking at the commit that introduced it https://github.com/llvm/llvm-project/commit/b1f54ff42f96893591dba930320045c4b54c533b, it looks like it's only there to use the Foreign module in the tests.

[alan-j-hu]

How do "with-test" dependencies work in the context of META? I see that LLVM 14 package has a "with-test" dependency on ounit: https://opam.ocaml.org/packages/llvm/. Is there any situation where one installs the LLVM package and runs tests?

Do you need me to take a specific action for this PR to be merged, or are you just making this PR and cc'ing me to let me know about it?

[kit-ty-kate]

How do "with-test" dependencies work in the context of META? I see that LLVM 14 package has a "with-test" dependency on ounit: https://opam.ocaml.org/packages/llvm/. Is there any situation where one installs the LLVM package and runs tests?

I'm not sure why the ounit dependency is still there. It not used anywhere in the package at the moment so it should be removed. Usually packages also define what the tests are to run them but we don't do that.

Do you need me to take a specific action for this PR to be merged, or are you just making this PR and cc'ing me to let me know about it?

Yes. This needs to be removed from the META file upstream. We're not personally using it but for people using upstream LLVM (distributions) it would be nice to have that fixed.

[kit-ty-kate]

I'm not sure why the ounit dependency is still there

done in https://github.com/kit-ty-kate/llvm-dune/commit/38ad8c62eb5f8ee15ea67b0b78d4481980a0241a

[alan-j-hu]

Okay, I tried to replace ctypes.foreign with just ctypes in the LLVM META file, and now when I ran ninja check-llvm-bindings-ocaml, the executionengine test failed. For whatever way that the CMake build system works, it seems that listing the test-only dependencies in the META file is necessary. I need to investigate further if the build system can be revised to not have this issue.

[kit-ty-kate]

Okay, I tried to replace ctypes.foreign with just ctypes in the LLVM META file, and now when I ran ninja check-llvm-bindings-ocaml, the executionengine test failed. For whatever way that the CMake build system works, it seems that listing the test-only dependencies in the META file is necessary. I need to investigate further if the build system can be revised to not have this issue.

That's expected. You need to add -package ctypes.foreign to the header of llvm/test/Bindings/Ocaml/executionengine.ml since it's apparently the one file that uses ctypes-foreign.

[alan-j-hu]

Should I try to get this change backported to the LLVM 17.x branch before LLVM 17 is released?

[kit-ty-kate]

it's always been there so i don't think it's time critical, don't worry