Skip to content
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

Update CMake dependencies to rely on proper targets #261

Merged
merged 23 commits into from
Mar 14, 2024

Conversation

jdumas
Copy link
Contributor

@jdumas jdumas commented Mar 7, 2024

  • This PR builds upon Update ImGui to 1.90.4 #260
  • Updates glm to 0.9.9.8 using a submodule
  • Clean up CMake code to use proper target-based dependencies
  • Do not add subdirectories if a dependency already exist, making it easier to reuse deps coming from other parts of a larger project.

@jdumas jdumas changed the title Jdumas/cmake Update CMake dependencies to rely on proper targets Mar 7, 2024
@jdumas
Copy link
Contributor Author

jdumas commented Mar 14, 2024

Done cleaning up this PR after the merge of #260. Should be much easier to review @nmwsharp. Note that the main reason to update to glm 0.9.9.8 was to use their CMake target directly. Updating to 1.x versions break compilation so I refrain from doing that.

@nmwsharp
Copy link
Owner

Thanks!

I'm a little wary about unnecessarily breaking things for people who might be e.g. using Polyscope's copy of json or something (even though this technically wasn't an intended use of the build setup). I'm also worried about what I'm always worried about with CMake, which is that it's not clear to me how to write clean proper CMake scripts, but also not break everyone's ancient dusty CMake setups in their own projects!

Regardless, I tried rebuilding so of my own projects with this and it all seems to work, so I say let's do it. The change to using targets and potentially pulling them from the outer project is quite useful.

@nmwsharp nmwsharp merged commit bb03432 into nmwsharp:master Mar 14, 2024
5 checks passed
@nmwsharp
Copy link
Owner

Merged! (Also we're about to get a bunch of issues about glm being broken, I need a keyboard macro that auto-replies "run git submodule update --init --recursive" 😄 )

@nmwsharp
Copy link
Owner

By the way, can you explain more about what glm 1.x breaks? As long as we're changing the build setup, that's a pretty good time to bite the bullet and update glm if the incompatibility isn't too bad.

@jdumas jdumas deleted the jdumas/cmake branch March 14, 2024 15:59
@jdumas
Copy link
Contributor Author

jdumas commented Mar 14, 2024

Thanks for merging this quickly!

which is that it's not clear to me how to write clean proper CMake scripts

Oh man, I could go on for a while on this topic... Very briefly, I'd break it down to two items:

  1. Treat CMake code as an object-oriented language, where CMake targets are your "objects", and rely heavily on target properties rather than CMake variables. CMake variables are very brittle (between various policies, cache variables, scoping to functions/directories, etc.), whereas target and their properties are much more reliable. This was one Daniel Pfeifer's point in his 2017 talk "Effective CMake" (the first 45min of this are great, after that it gets into the weeds of packaging, which is hard).
  2. Don't be afraid to use bump CMake version to benefit from their latest features. Because of their emphasis on strong backward compatibility (via CMake policies), updating the min required CMake version will never break an old project. But leveraging their latest feature can simplify a lot of things. See this list for a highlight of the new features in recent CMake versions. E.g. I now use CMake 3.25 so I can set CMAKE_MSVC_DEBUG_INFORMATION_FORMAT on Windows, which helps my project play nicely with ccache or address sanitizer on Windows.

Anyhow if you have more specific concerns I'm always happy to answer any CMake question :D

Also we're about to get a bunch of issues about glm being broken, I need a keyboard macro that auto-replies "run git submodule update --init --recursive" 😄 )

Yep, this is why I've switched to FetchContent/CPM for dependency management in CMake...

By the way, can you explain more about what glm 1.x breaks? As long as we're changing the build setup, that's a pretty good time to bite the bullet and update glm if the incompatibility isn't too bad.

Hmm I encountered some compilation errors at the time, so I bumped it down to 0.9.9.8. But now that I'm trying again it seems to work right away... the only thing I had to do was define GLM_ENABLE_EXPERIMENTAL more broadly (e.g. it's now needed in some of the functions used in the MarchingCube target). So I've opened #263 if you want to update GLM to the very latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants