Conversation
jdumas
left a comment
There was a problem hiding this comment.
The main reason I've avoided using ExternalProject_Add for gmp/mpfr on Linux/macOS is that ExternalProject_Add is really more suited for a two-stage CMake build. The typical approach to using ExternalProject_Add with CMake is as follows:
- Run a "meta" CMakeLists.txt that uses
ExternalProject_Addto download, compile & install all third-party libs into a desired target folder. - Run your own project's CMakeLists.txt and use
find_package()to import the target you have just installed in the previous step.
This requires the user to run cmake && make two times for the project, and I really don't like this approach.
The problem with trying to use ExternalProject_Add like you're doing right now, is that it's not easy to express the dependencies between building the external lib, and building the other targets that depend on it. Let's imagine for a moment that building libigl is super fast, and you attempt to link your libig_copyleft_cgal.a against gmp.a, but your external project hasn't finished building gmp. Then you're gonna get an error. Given how fast it is to build gmp/mpfr, and how slow it is to build libigl, this is not gonna happen very often. But looks like you've had similar issues with the include headers.
I'm pretty sure it's possible to express the dependency between gmp::gmp and the external project, to make sure the external lib is downloaded & compiled before anyone tries to use it. But this will require some additional logic from what I can tell.
| ExternalProject_Get_Property(gmp SOURCE_DIR) | ||
| set(gmp_LIBRARIES ${gmp_LIBRARY}) | ||
| add_library(gmp::gmp INTERFACE IMPORTED GLOBAL) | ||
| file(MAKE_DIRECTORY ${gmp_INCLUDE_DIR}) # avoid race condition |
There was a problem hiding this comment.
You can still get race condition if you try to compile a file that includes <gmp.h> before the external project is downloaded.
There was a problem hiding this comment.
This is not for a race condition at make time, but rather at cmake time. The following line:
target_include_directories(gmp::gmp INTERFACE ${gmp_INCLUDE_DIR})
will fail if that include directory doesn't exist (it doesn't matter if the actual headers are there yet. just need the path to exist).
There was a problem hiding this comment.
(this comment # avoid race condition is from a copy-paste. I don't really agree that it's a race condition. It has nothing to do with threads AFAICT. It's rather just that the directory doesn't exist yet)
|
I haven't run into that issue yet. Are you sure the dependencies aren't handled correctly? Consider this mini project https://github.com/alecjacobson/gmp-mpfr-externalproject_add If I run |
|
Ah you're right, it looks like the line |
|
Thanks. Do you have any suggestions about the build sub-directories organization? I got super confused trying to use |
|
If you |
|
fwiw, this worked successfully for my immediate use case: compile libigl-based mex functions and running them on an M1 mac running Matlab using Rosetta. |
|
Make sure you remove gmp/mpfr's install for Linux/macOS on GitHub Actions before merging this: libigl/.github/workflows/continuous.yml Line 52 in 1f44d1e libigl/.github/workflows/continuous.yml Line 60 in 1f44d1e |
Previously we relied on gmp and mpfr to be installed system wide. This (work in progress) PR uses cmake's
ExternalProject_addto download and compile gmp and mpfr from source in the build.gmp and mpfr use
configure/makestyle build which seems well supported byExternalProject_add. I've also borrowed some configure flags from homebrew's build of these libraries (otherwise they were not easy to compile following the gmp/mpfr documentation).This affects (and appears to work correctly) on Mac and Linux. A nice feature of compiling these from source, is that it's more straightforward to compile for a different architecture (e.g., compiling for x86_64 and using Rosetta on an M1 Mac).
I don't recall why we weren't compiling gmp/mpfr from source. Is the a reason besides this ExternalProject_add not existing/being written?
Windows continues to use precompiled binaries downloaded from CGAL.