CMake refactor by jdumas · Pull Request #1805 · libigl/libigl

jdumas · 2021-05-10T02:27:24Z

Alright, here it is. As part of the efforts towards v3.0.0 described in #1696, this refactors libigl's CMake system to be more modern and maintainable. The new setup is inspired by Lagrange, e.g. how include() + FetchContent is used to download and handle third-party dependencies.

I wrote an upgrade guide > here <, which I will update with final information once we merge this PR.

Highlights

CGAL and Boost are now built entirely from source via CMake. This means the CGAL/Cork module will work out of the box under Windows -- no need for a conda virtual environment. On Linux/macOS, the only dependency to install are GMP+MPFR.
CMake targets and options have been renamed based on the module category: regular/copyleft/nonfree. This eliminates long module names (igl::open_glfw_imgui) that didn't bring much to the table, in favor of three root classes based on available licenses for each module.
There is now a LibiglOptions.cmake sample file at the root of the project repository, which should make it easier to document/update available CMake options.
There is now a unique entry point for the project, which is the root CMakeLists.txt. This facilitates maintenance, as before either tutorial/CMakeLists.txt and cmake/libigl.cmake could serve as potential entry points.
The default behavior for module compilation is:
- When compiling libigl on its own, all modules are enabled by default.
- When compiling libigl as a subproject, all modules are disabled by default.
- On Linux/macOS, if GMP/MPFR is not installed on the system, shows a warning and disable CGAL/Cork by default.
- If the user explicitly enables those modules, but they do not have GMP/MPFR installed on the system, CMake configuration will still fail.
Big cleanup of the tutorial CMake definitions. With the new setup adding a new tutorial is even easier!
Cleaned up dependency setup for imgui/stb (among others)
Unit tests have been split into different CMake target, based on which module is tested.
Add support for Hunter + install() for the igl::core module (should be the same as before).
Use a more generic igl_copy_dll() function to copy GMP/MPFR dlls on Windows.
Cleaned up our .gitignore file.
~~Tested on macOS/Windows.~~ ~~Ok looks like I have some issues left to sort out on Windows, shouldn't be to bad.~~
--> Looks like we're having compilations issues with Cork on Windows. Cork depends on gmpxx, which is a C++ wrapper around gmp types. The CGAL precompiled gmp libs only come with gmp itself. Thus supporting Cork with gmpxx on Windows would require integrating the mpir fork of gmp. Because this is a lot of work for little value, I vote that we drop support for the cork files altogether. After all we know that there are better alternatives nowadays.

TODO

Matlab/Mosek modules are currently missing, because I do not have them setup on my machine/have no way of testing them via Github Actions.
OSXFixDylibReferences.cmake is also not used, for the same reason as above (although I am on macOS, I dot not have a setup that needs it).
@rbsheth you wrote the initial hunterization of libigl. Could you let me know if you have any concern wrt to this new CMake setup.

alecjacobson · 2021-05-27T14:59:30Z

How dependent are all these changes on each other? Each time I flip through this I get overwhelmed by the number of changes. I'm a bit worried that if we keep it as one larger PR it will never get merged. These changes may have more success if broken down into independent parts.

jdumas · 2021-05-27T15:07:12Z

Well the goal of this PR was to refactor and cleanup our CMake build system. There are two parts to this 1) pulling the dependencies, and 2) cleaning up how modules are defined, as well as tutorials/unit tests. It may be a lot of changes, but it's really confined to the CMake scripts (I avoided doing changes to the .cpp unless I had to). I'd rather not do "half-cleanup" for this. Better look at it as a rewrite of the whole CMake build system where everything is new.

alecjacobson

Some initial comments. I think this will take a very long time to sift through everything.

alecjacobson · 2021-05-27T15:00:45Z

CMakeLists.txt

+endif()
+
+# Check for gmp + mpfr and enable cork / cgal modules accordingly
+set(LIBIGL_DEFAULT_CGAL_CORK ${LIBIGL_TOPLEVEL_PROJECT})


Should this LIBIGL_DEFAULT_CGAL_CORK be something like LIBIGL_DEFAULT_GMP_MPFR ? so it's more clear what it's controlling?

(I'm also not sure what DEFAULT means here.)

We don't have an option LIBIGL_DEFAULT_GMP_MPFR. Here LIBIGL_DEFAULT_CGAL_CORK controls the default value of the options for the CGAL and Cork modules. If GMP and MPFR are not available on the target machine, then I disable the CGAL/Cork modules by default, since CGAL/Cork depend on GMP+MPFR.

Right, and I'm commenting that the name LIBIGL_DEFAULT_CGAL_CORK is confusing since it seems to primarily refer to the presence of gmp/mpfr.

The value of LIBIGL_DEFAULT_CGAL_CORK is set based on the presence of gmp/mpfr, but what it is used for is to set the default option for the CGAL and Cork modules. But hey I'm happy to rename it if you prefer.

One thing though: if libigl is not built as a subproject, LIBIGL_DEFAULT_CGAL_CORK will be OFF. Also on Windows, LIBIGL_DEFAULT_CGAL_CORK will always be ON by default for toplevel project. So if I rename the variable into LIBIGL_GMP_MPFR_AVAILABLE, this would be somewhat misleading (e.g. GMP/MPFR might be available, but because libigl is built as a subproject, it will be set to OFF).

I see. Maybe it makes sense to have a variable that tracks whether GMP_MPFR has been found and then the existing variables for wether building the cork / cgal modules.

a variable that tracks whether GMP_MPFR has been found

What would you use it for? To track whether GMP/MPFR have been found, I would test for the existence of the imported targets gmp::gmp and mpfr::mpfr. Using variables in CMake is kinda brittle so I like to keep it to the bare minimum. But if you want to amend this part of the script to your liking feel free to give it a shot, since I'm not sure what you have in mind.

I don't think I can amend it because I don't understand it. What does LIBIGL_DEFAULT_CGAL_CORK mean? LIBIGL I can guess means this is one of our variables, DEFAULT means this is a flag On or Off for a default setting. CGAL_CORK why both? Is this variable the same as LIBIGL_DEFAULT_CGAL AND LIBIGL_DEFAULT_CORK?

I mean it's just a variable name. The "correct" name should be:

LIBIGL_DEFAULT_VALUE_FOR_CMAKE_OPTION_CGAL_AND_CORK_MODULES_BASED_ON_WHETHER_GMP_AND_MPFR_ARE_AVAILABLE

Then I just use this variable twice to set the default value for the following CMake options:

LIBIGL_COPYLEFT_WITH_CGAL

LIBIGL_COPYLEFT_WITH_CORK

So really this should be invisible to the user. LIBIGL_DEFAULT_CGAL_CORK is just a "local variable", it's not a proper CMake option that the user has to concern themselves with. If the user provides their own value for LIBIGL_COPYLEFT_WITH_CGAL or LIBIGL_COPYLEFT_WITH_CORK, the user-provided option will take precedence.

alecjacobson · 2021-05-27T15:02:16Z

cmake/find/FindGMP.cmake

@@ -0,0 +1,76 @@
+# Try to find the GNU Multiple Precision Arithmetic Library (GMP)


Somehow from previous discussions with @jdumas I was under the impression that writing custom Find*.cmake was a no-no these days. This PR adds a lot of custom cmake. Is this really what's expected for a modern cmake build system? Or is it a special case because GMP/MPFR are a pain?

GMP/MPFR are a pain yes. We are not building them from source since they do not use a CMake build system. So the only option is to install via a package manager on macOS/Linux, or download a pre-built dll on Windows (which this find_package() will find).

alecjacobson · 2021-05-27T15:04:10Z

cmake/igl/modules/copyleft/cgal.cmake

@@ -0,0 +1,32 @@
+# 1. Define module


These cmake/igl/modules//.cmake files all look roughly the same. Perhaps this can be abstracted into a function rather than copy-pasting files with minor changes.

I am wary of making everything into a function. This file still separates the different parts of defining a libigl module based on:

Module name

Include headers

Target sources

Target dependencies

Unit test definitions

Imho abstracting away some of these steps would make it too much look like "cmake magic" (e.g. where does the include header definition come from?).

CMakeLists.txt

LibiglOptions.cmake.sample

alecjacobson · 2021-05-27T15:18:48Z

include/igl/opengl/glfw/imgui/ImGuiMenu.cpp

  if (ImGui::CollapsingHeader("Workspace", ImGuiTreeNodeFlags_DefaultOpen))
  {
-    float w = ImGui::GetContentRegionAvailWidth();
+    float w = ImGui::GetContentRegionAvail().x;


candidate for independent PR

I have added the IMGUI_DISABLE_OBSOLETE_FUNCTIONS define macro when updating our imgui dependency, which required updating the code accordingly. I could make a separate PR for this but I don't think it's worth the effort (adding IMGUI_DISABLE_OBSOLETE_FUNCTIONS to our current CMake would create conflict, and otherwise the change is not needed).

jdumas · 2021-05-29T22:06:20Z

Alright I have renamed nonfree -> restricted + added details in comments, and updated the description in the ".sample" CMake file. Once this is merged I will update instructions on the website accordingly.

rbsheth · 2021-06-25T18:32:04Z

I haven't got a chance to review the PR in its entirety but happy to help test as needed.

jdumas · 2021-06-27T22:36:29Z

I haven't got a chance to review the PR in its entirety but happy to help test as needed.

Thanks! I just want to make sure this is not breaking existing Hunter integration. I don't have much experience packaging libraries with Hunter, and I'm not sure if you use the upstream repo or a clone for the Hunter package. If you have any insight please let me know.

melMass · 2021-09-05T21:48:00Z

Hey, I'm interested in adding Conan support (#1634) and it seems this PR is almost done and awaits review?

Thanks

jdumas · 2021-09-05T21:56:14Z

Yes I'm waiting for green light from @alecjacobson and @danielepanozzo, but not sure when it's gonna happen...

melMass · 2021-09-25T20:55:35Z

Thanks I'll wait for @alecjacobson and @danielepanozzo to chime in!

alecjacobson · 2022-02-10T06:13:33Z

🎉

libigl introduced breaking changes in libigl/libigl#1805 This fixes the OSS-Fuzz set up.

jdumas and others added 15 commits May 2, 2021 10:00

Start CMake refactoring.

52c14ee

WIP on instlal().

b5942a9

Handle install + hunter setup.

90e0900

Handle module options + igl_include helper.

a3e7370

Build all tutorials again.

d0a9be2

Update CMake for unit tests.

ddbfa78

Update triangle lib.

3970a96

Merge remote-tracking branch 'origin/main' into jdumas/cmake-refactor

4cc455f

Update Hunter integration.

6367112

Fix for header-only mode.

ac42b6d

Fix Windows compilation + add GMP/MPFR find_package + set VS folders.

56d8617

Fix CGAL build.

34cee78

Add copy dll for Windows + improve find gmp/mpfr.

66dc99e

Update Github Actions.

adf2524

Update README.

bdfd7a2

jdumas added compilation cmake labels May 10, 2021

jdumas added this to the v3.0.0 milestone May 10, 2021

jdumas requested review from alecjacobson, danielepanozzo and qnzhou May 10, 2021 02:27

jdumas referenced this pull request in libigl/triangle May 24, 2021

cmake version way ahead of libigl (3.1); probably typo

3ee6cac

This was referenced May 25, 2021

Add conan support #1634

Closed

bump tetgen commit hash #1815

Merged

alecjacobson reviewed May 27, 2021

View reviewed changes

jdumas added 3 commits May 27, 2021 12:30

Fixes for CMake 3.18

aae9440

Fix include option.

bc4b9db

Rename libigl_imgui_front to avoid conflicts.

f543129

jdumas added 3 commits May 29, 2021 14:54

Update triangle and tetgen versions.

0342125

Merge remote-tracking branch 'origin/main' into jdumas/cmake-refactor

87ee094

Fix compilation issue.

fec2d43

ngodber mentioned this pull request Jun 19, 2021

Deprecate third party builds and utilize apt-get | conda-forge | port PyMesh/PyMesh#302

Open

jdumas mentioned this pull request Sep 20, 2021

Bump CGAL → Boolean + remesh_*intersections performance boost #1895

Merged

jdumas mentioned this pull request Nov 8, 2021

Cannot open include file: 'triangle.h': No such file or directory #1635

Closed

3 tasks

jdumas and others added 8 commits January 5, 2022 11:38

Update continuous.yml

659d00d

Merge remote-tracking branch 'origin/main' into jdumas/cmake-refactor

039a73d

remove cork

e762413

finding matlab with default

f663883

mosek module compiles; default detected; osx dylib hack

0143e9c

Reduce build parallelism.

02a588d

Fix find GMP/MPFR on Windows + cleanup CGAL changes.

fac1b79

Update readme.

ce4fde8

jdumas removed this from the v2.4.0 milestone Feb 10, 2022

jdumas changed the title ~~CMake refactor for v3.0.0~~ CMake refactor Feb 10, 2022

jdumas merged commit b0fd49d into main Feb 10, 2022

jdumas deleted the jdumas/cmake-refactor branch February 10, 2022 02:58

alecjacobson mentioned this pull request Feb 10, 2022

Module reorganization and major change suggestions #1696

Closed

DavidKorczynski added a commit to DavidKorczynski/oss-fuzz that referenced this pull request Feb 19, 2022

libigl: fix build following upstream major update

a8be712

libigl introduced breaking changes in libigl/libigl#1805 This fixes the OSS-Fuzz set up.

DavidKorczynski mentioned this pull request Feb 19, 2022

libigl: fix build following upstream major update google/oss-fuzz#7305

Merged

AdamKorcz pushed a commit to google/oss-fuzz that referenced this pull request Feb 19, 2022

libigl: fix build following upstream major update (#7305)

3f886e7

libigl introduced breaking changes in libigl/libigl#1805 This fixes the OSS-Fuzz set up.

MartinPetkov pushed a commit to MartinPetkov/oss-fuzz that referenced this pull request Aug 15, 2022

libigl: fix build following upstream major update (google#7305)

c906f07

libigl introduced breaking changes in libigl/libigl#1805 This fixes the OSS-Fuzz set up.

		@@ -0,0 +1,76 @@
		# Try to find the GNU Multiple Precision Arithmetic Library (GMP)

Conversation

jdumas commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Highlights

TODO

Uh oh!

alecjacobson commented May 27, 2021

Uh oh!

jdumas commented May 27, 2021

Uh oh!

alecjacobson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdumas commented May 29, 2021

Uh oh!

rbsheth commented Jun 25, 2021

Uh oh!

jdumas commented Jun 27, 2021

Uh oh!

melMass commented Sep 5, 2021

Uh oh!

jdumas commented Sep 5, 2021

Uh oh!

melMass commented Sep 25, 2021

Uh oh!

alecjacobson commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jdumas commented May 10, 2021 •

edited

Loading