point GMPXX_INCLUDE_DIR to locally built gmp#2030
Conversation
| set(GMP_INCLUDE_DIR "") | ||
| set(GMP_INCLUDE_DIR ${gmp_INCLUDE_DIR}) | ||
| set(GMP_LIBRARIES gmp::gmp) | ||
| set(GMPXX_INCLUDE_DIR ${GMP_INCLUDE_DIR}) |
There was a problem hiding this comment.
I wonder if we should add ignore_package(GMPXX 5.0.1) as well just to be safe?
There was a problem hiding this comment.
Do you know what this is doing? If it makes sense to add, then happy to.
There was a problem hiding this comment.
(adding this doesn't seem to hurt)
There was a problem hiding this comment.
ignore_package() is a function I wrote in this cgal.cmake. Basically it will add an empty GMPXXConfig.cmake file, thus preventing CGAL from using its own FindGMPXX.cmake file (and subsequently adding /usr/local/include to your include paths).
|
|
||
| ignore_package(GMP 5.0.1) | ||
| set(GMP_INCLUDE_DIR "") | ||
| set(GMP_INCLUDE_DIR ${gmp_INCLUDE_DIR}) |
There was a problem hiding this comment.
This is not necessary, since our target gmp::gmp should carry the include dir already.
There was a problem hiding this comment.
I think CGAL's cmake is looking at GMPXX_INCLUDE_DIR so it needs to be explicitly set.
There was a problem hiding this comment.
Confirmed, if GMPXX_INCLUDE_DIR is set to "" then CGAL determines that CGAL_WITH_GMPXX:BOOL=OFF. By setting GMPXX_INCLUDE_DIR explicitly then we keep CGAL_WITH_GMPXX:BOOL=ON.
There was a problem hiding this comment.
If you add ignore_package(GMPXX 5.0.1), you can probably leave GMP_INCLUDE_DIR and GMPXX_INCLUDE_DIR set to "", since the fake GMPXXConfig.cmake will set GMPXX_FOUND to ON, and CGAL_WITH_GMPXX should subsequently be set to ON.
There was a problem hiding this comment.
ah, got it. Didn't realize these were related. I pushed the change.
There was a problem hiding this comment.
We're back to the state of the original PR which passes CI. If there's a more elegant way to handle GMPXX that maintains passing for Windows then let's merge it in a new PR. I'll merge this for now.
There was a problem hiding this comment.
What do you mean by why would the CI succeed? With your changes you are not preventing CGAL's FindGMPXX from executing. But because you set the GMPXX_INCLUDE_DIR variable in advance, CGAL is using those locations. On Windows, it will still go through this find_path() command and silently fail to find GMPXX. On Linux/macOS, I am not sure whether it find them or not.
In the end I suppose your setup kinda works, though you may think you're using GMPXX on Windows when you are not.
There was a problem hiding this comment.
Right. In the merge version GMPXX_INCLUDE_DIR is set to point to the gmp build for all systems, and I guess on windows cgal will realize that gmpxx should be off so it's ignored. Using the ignore_package caused windows build to fail out of the box. If there's a solution that avoids pretending that gmpxx is really there then we should merge it, but I'm not sure what that is and I end up pulling my hair out trying to debug the windows build.
There was a problem hiding this comment.
Well the solution is simple:
if(WIN32)
set(CMAKE_DISABLE_FIND_PACKAGE_GMPXX ON)
else()
ignore_package(GMPXX 5.0.1)
set(GMPXX_INCLUDE_DIR "")
set(GMPXX_LIBRARIES ${GMP_LIBRARIES})
endif()The end result should be basically the same though...
There was a problem hiding this comment.
😊 Want to PR this and merge?
Fixes #2029
gmpxx is part of gmp and we're building it via cmake. Previously we weren't telling cgal where to find it and on some machines (mine at least) it was finding it at /usr/local/include. This caused /usr/local/include to be added to include paths which caused #2029 to happen (cgal was also found there and confused).
This should tell cgal where to find gmpxx and subsequently not add /usr/local/include.