8000 Use CMake find macro instead of pulling in googletest as a submodule by kintel · Pull Request #402 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use CMake find macro instead of pulling in googletest as a submodule #402

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

Closed
wants to merge 3 commits into from

Conversation

kintel
Copy link
Contributor
@kintel kintel commented Apr 5, 2023

Proof of concept @elalish

Prerequisites (Ubuntu): sudo apt install libgtest-dev

@elalish elalish marked this pull request as ready for review April 5, 2023 06:07
@elalish
Copy link
Owner
elalish commented Apr 5, 2023

Picking up from #400 (comment), I wonder why gtest was giving you trouble on 22.04, since that's what our CI runs on?

I'm unmarking this as a draft because I'd like to see what our CI does with it, though it'll need a prod to try running the tests.

enable_testing()
find_package(GTest REQUIRED)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me, I'm still trying to comprehend CMake - does this mean GTest maintains a file in the CMake distribution for finding it's install location on various architectures? Does this mean any project where find_package is available has some distribution on Linux, Mac, and Windows? That would be pretty convenient - I wonder how many of our submodules have that?

The part of this change I have the feeling will be trickier is updating manifold.yml to get our CI to install the required packages on each platform.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a search path which cmake will try to find the package file, similar to a compiler trying to search for header files. I think this is fine with gtest as it is widely used and is not a critical dependency for us. For other dependencies, I think the current submodule approach would be better as it allows us to control the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t-paul You tend to be good at explaining reasons for decoupling dependencies, adding you here in case you want to chip in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elalish CMake comes with a bunch of find macros. I believe these macros mostly deal with offering a way of interacting with the components of the package, namespaced library names etc. A lot of these macros will use a more structured approach, like pkg-config, to locate the actual software. Since these interact with binaries, it also doesn't matter which build system the dependencies were built with.

The caveat (there is always a caveat) is that on systems without decent options for package management, coughwindowscough, the find macros sometimes need help.

..but to get best of both worlds, build all your dependencies with separate CMake invocations, then use standard find macros (with necessary hints) to find the software, then it should be easy for anyone to build manifold binaries which suit their environment.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kintel Thanks for the explanation, that does indeed sound like the best of both worlds: basically allow use of the binary distro if available (and if it's version is compatible) and fall back to our submodule otherwise, but keep its CMake process somehow separated. I've never accomplished that - do you have examples? Preferably that demonstrate it working on a CI across Linux, Mac and Windows when the package isn't available on them all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any good examples; a lot of this comes down to strategies for software distribution vs. CI vs. development, which may all have different goals and challenges.

I expect most of the pain of build systems and package management to happen once a project grows out of the "lab" and gets external users, so you don't necessarily have to solve all problems at once.

..but spending some time to learn and experiment with CMake is always a good thing :)

Also, I'm not saying that this will make things easier for you; it's about making it easier for downstream consumers, which invariably adds more work here.

Copy link
Owner
@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, well I tried updating our manifold.yml to include libgtest-dev, but it seems to have only worked for the mac build. I have no idea why Ubuntu is angry, and I don't even know where to pull gtest for Windows. Any thoughts? Currently this is convincing me submodules is a better way to go...

@kintel
Copy link
Contributor Author
kintel commented Apr 10, 2023

For OpenSCAD, we use msys2 on Windows. It's a way of making Windows appear more friendly (i.e. Unix-like), and it comes with a package manager where we can install software in a structured fashion: https://github.com/openscad/openscad/blob/master/.github/workflows/windows.yml

I've heard things about vcpkg as well, but it's rumored to be very slow.

@kintel
Copy link
Contributor Author
kintel commented Apr 10, 2023

For the Ubuntu breakage, I cannot tell; looks like some interaction between gtest and glm

@elalish
Copy link
Owner
elalish commented Apr 25, 2023

As I say, I'd love to make our library easier to consume, and no one will know better how to do that than the users themselves. The only reason I didn't use CMake's findPackage originally was because I couldn't figure out how to set up the CI to make that work. And in fact much of our CI has been contributed by our users, eager to make sure their systems get tested on. So, this can only be a proof of concept if a solution to the various CIs is found. I don't know how to accomplish it, but msys2 sounds like a good idea. Anyway, if you can prove this system works in this PR, I'll be happy to try and extend the approach to our other dependencies.

@pca006132
Copy link
Collaborator

I am revisiting this. What do you think about using system provided package when available, and use CMake FetchContent when the package is not available?

@pca006132
Copy link
Collaborator

Implemented in #523.

@pca006132 pca006132 closed this Aug 7, 2023
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.

3 participants
0