OpenTimelineIO / raven

Raven - OpenTimelineIO Viewer Application
Apache License 2.0
81 stars 21 forks source link

Change glfw to git submodule #4

Closed markreidvfx closed 1 year ago

markreidvfx commented 1 year ago

Doing this has simplified the building experience for me on and mingw-x64 and Linux and removes an external dependency. I haven't tested this too much on macOS, but it seems to work.

jminor commented 1 year ago

Good idea. I kind of like using submodules for all the dependencies rather than CMake, since it makes it easier to handle upgrades and upstream forks.

meshula commented 1 year ago

This change loses one advantage of my script, which can be restored without too much trouble.

My script was compatible with find_package, and only downloaded glfw if it wasn't already available to cmake. I prefer to configure projects like glfw that have a lot of options, the way I like them, rather than having to poke around in other people's cmakes to tweak things

If you add a find_package(glfw3) back in, and only add the glfw subdirectory if its not found, you accrue the benefit of not having to burden your source tree with extra bulk, and would give me back the ability to have my own tweaked version of glfw.

The reason I prefer the FetchContent pattern is that the downloaded sources go in your build tree, whereas gitmodules put the downloaded sources in your source tree. I personally prefer being able to clean out extraneous junk by just deleting the build tree.

YMMV. But I do think trying find_package first would be preferable than always using a local copy of glfw. FetchContent vs submodule I don't really care about overmuch, as the whole question seems a bit subjective.

markreidvfx commented 1 year ago

I didn't realize that the cmake file was downloading glfw if it couldn't find it. I saw those prebulit MSVC libs and assumed we where linking against those like the imgui examples. We can bring it back if you'd prefer. Not sure why I was having linking issues with mingw-x64 then, might have just been the older version of imgui.

meshula commented 1 year ago

yeah, they weren't prebuilt, they were built by cmake as part of the FetchContent mechanism (the "populate" instruction does a git pull if necessary, and then runs the build). I'm not sure why mingw would particularly be an issue, because I haven't done any compiler specific configuration.

my other projects have got a bunch of scripts that all work together, so I have a hard core imgui configurator, and stuff like that. I only brought the glfw bit over here. Everything else in raven is using submodules, so there's no compelling reason to switch it back, unless we want to get into a full on portability push.

like this ~ https://github.com/meshula/LabImGui/blob/main/imgui.cmake ~ can configure every possible back end for Dear ImGui ~ but unless your project is plugged in with all this sort of thing, the benefits probably don't really accrue.

the thing I like about the approach is that I can treat these little files (another https://github.com/meshula/LabImGui/blob/main/ImPlot.cmake) like libraries. So once I've got ImGui.cmake in my system, I can just mix in ImPlot.cmake by including it in one line of code. For me, that's all a lot easier than adding lines of code here and there in an uber cmake file ~ I prefer the reusability of the separate scripts.

Again, not promoting that we switch to using my system, just can't help myself in wanting to explain it :)

jminor commented 1 year ago

Thanks for the explanation @meshula ! In your system, how do you handle cases where a specific project wants a specific version of Dear ImGui, or ImPlot, etc. which doesn't match the one found by FetchContent?

meshula commented 1 year ago

GIT_REPOSITORY has a corresponding GIT_TAG, and URL has a URL_HASH (which you may have to compute yourself) to validate a tarball.

FetchContent_Declare(
  googletest
  GIT_REPOSITORY https://github.com/google/googletest.git
  GIT_TAG        703bd9caab50b139428cea1aaff9974ebee5742e # release-1.10.0
)
FetchContent_Declare(
  myCompanyIcons
  URL      https://intranet.mycompany.com/assets/iconset_1.12.tar.gz
  URL_HASH MD5=5588a7b18261c20068beabfb4f530b87
)