-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
WIP: Migrating OCaml binding to CMake #7254
base: master
Are you sure you want to change the base?
Conversation
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.
Overall this looks like a good start, but it needs a lot of clean-up and extensions before it will be usable in practice.
-ocamlcflags -bin-annot \ | ||
-package zarith \ | ||
-lz3 -lstdc++ -lpthread\ | ||
-lgmp \ |
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.
GMP is optional, so this should be guarded.
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.
Got it.
@@ -3712,6 +3713,31 @@ end | |||
For example: | |||
(set_global_param "pp.decimal" "true") | |||
will set the parameter "decimal" in the module "pp" to true. | |||
|
|||
Legal parameters are: |
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.
These parameters can change and nobody will update this list. We could perhaps try to get this list programmatically?
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.
Oh, this comes from my previous learning/experimenting notes so it shouldn't be included here.
I agree with your point.
set(libz3_path ${PROJECT_BINARY_DIR}) | ||
endif() | ||
|
||
add_custom_command( |
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.
When using a custom command and target, will these always rebuild, even when there are no changes to the inputs?
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.
No. add_custom_command
will check the depends
and outputs
to decide whether to run it. CMake also checks them statically to make sure these commands are valid.
VERBATIM | ||
) | ||
|
||
add_custom_command( |
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 shouldn't be necessary to copy source files around, the other targets that use them can just as well reference the source directory.
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's subtle and I prefer to copy them. The reason is ocamlc
may generate some intermediate files at the source location. It general, it can also simplify the dependencies of custom commands when some files are generated to be in the build path while some files are at the source path.
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.
Oh yes, that's right. We do have to make sure everything is generated in the build directory, and sometimes that might indeed require copying source files around if the OCaml tools don't provide commandline flags to set the output path.
if( APPLE ) | ||
set(ocaml_rpath "@executable_path/../libz3${so_ext}") | ||
elseif( UNIX ) | ||
set(ocaml_rpath "\\$ORIGIN/../libz3${so_ext}") |
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.
- Windows/Cygwin?
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.
At this moment, building on Windows/Cygwin is beyond my knowledge and I think even the support for OCaml on Windows is under heavy development. I may prefer to at least postpone it until the rest is done.
p.s. I notice there is some good news in the OCaml community.
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.
Yeah, it's always been a bit neglected, but there are users, so we should make sure it works. BTW, the Opam people are also adding a Windows CI pipeline at the moment, so it might be required for Opam packages soon.
set(ocaml_rpath "@executable_path/../../../lib${LLVM_LIBDIR_SUFFIX}") | ||
elseif( UNIX ) | ||
set(ocaml_rpath "\\$ORIGIN/../../../lib${LLVM_LIBDIR_SUFFIX}") | ||
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.
- Windows/Cygwin
# INCLUDES Path for header files for C sources. | ||
# CFLAGS Additional arguments passed when compiling C stubs. | ||
# PKG Names of ocamlfind packages this library depends on. | ||
# LLVM Names of LLVM libraries this library depends on. |
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.
Is this LLVM-only? If so, we need to add support for other compilers. If not, we should change those names.
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 will remove it. It's the starting point of this work but later I realized the requirements for ocaml-llvm and ocaml-z3 are different.
endforeach() | ||
|
||
install(FILES ${install_files} | ||
DESTINATION "${LLVM_OCAML_INSTALL_PATH}/llvm") |
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.
Not sure where this will attempt to install files to, but clearly ..../llvm
will not make sense for any other compilers.
add_custom_command(TARGET "ocaml_${name}" POST_BUILD | ||
COMMAND "${CMAKE_COMMAND}" "-E" "copy" "${install_file}" | ||
"${LLVM_LIBRARY_DIR}/ocaml/llvm/" | ||
COMMENT "Copying OCaml library component ${filename} to intermediate area" |
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.
COMMENT "Copying OCaml library component ${filename} to intermediate area" | |
COMMENT "Installing OCaml library component ${filename}" |
@@ -292,6 +292,9 @@ The following useful options can be passed to CMake whilst configuring. | |||
* ``Z3_INSTALL_JAVA_BINDINGS`` - BOOL. If set to ``TRUE`` and ``Z3_BUILD_JAVA_BINDINGS`` is ``TRUE`` then running the ``install`` target will install Z3's Java bindings. | |||
* ``Z3_JAVA_JAR_INSTALLDIR`` - STRING. The path to directory to install the Z3 Java ``.jar`` file. This path should be relative to ``CMAKE_INSTALL_PREFIX``. | |||
* ``Z3_JAVA_JNI_LIB_INSTALLDIRR`` - STRING. The path to directory to install the Z3 Java JNI bridge library. This path should be relative to ``CMAKE_INSTALL_PREFIX``. | |||
* ``Z3_BUILD_OCAML_BINDINGS`` - BOOL. If set to ``TRUE`` then Z3's OCaml bindings will be built. | |||
* ``Z3_BUILD_JULIA_BINDINGS`` - BOOL. If set to ``TRUE`` then Z3's Julia bindings will be built. | |||
* ``Z3_INSTALL_JULIA_BINDINGS`` - BOOL. If set to ``TRUE`` and ``Z3_BUILD_JULIA_BINDINGS`` is ``TRUE`` then running the ``install`` target will install Z3's Julia bindings. |
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.
Add similar option for the OCaml bindings?
Thank you so much for your careful examination. This is one reason I would like to make this immature draft PR. I will check your changes one by one later. |
It would be absolutely great to cmake-ify the ocaml bindings for uniform build support and allow to retarget release processes to use the more portable cmake system. |
Aha, I was occupied by some random things after the previous effect on this issue in July. I hope to pick this up this month. |
It's my WIP working on migrating the building of OCaml binding to CMake.
I think the majority of the work is done. I started by mimicking the other bindings and some llvm cmake for ocaml, but then I just manually wrote enough custom commands and dependencies. The actual building commands should have the same result as the previously generated python scripts, though I think I simplified some unnecessary dependencies and compiler flags.
tldr;
It should just work. I also build and run the
ml_examples.ml
for both bytecode and native code in the cmake to make sure it works. I observe all z3 bindings are using shared library so I wonder maybe I don't even need to make a static version for the ocaml-binding.I also add one argument
-DZ3_BUILD_OCAML_EXTERNAL_LIBZ3=/path/to/libz3.so
to support building ocaml building with an external z3 library that is often installed by some other distro package managers.Now the remaining work is :
p.s. for ocaml users, I also have a post for discussion.