RenderKit / ospray

An Open, Scalable, Portable, Ray Tracing Based Rendering Engine for High-Fidelity Visualization
http://ospray.org
Apache License 2.0
985 stars 180 forks source link

Superbuild - glm not built only copied - no glmConfig.cmake installed #478

Closed ARSanderson closed 2 years ago

ARSanderson commented 3 years ago

The OS particulars:

OS X Big Sur 11.2 Xcode 12.4 Compiler clang : Apple land version 12.0.0 CMake 3.19.5 OSPRay 2.5 release

The cmake command: %cmake -DCMAKE_CXX_FLAGS="-stdlib=libc++" -DCMAKE_OSX_DEPLOYMENT_TARGET=10.9 ../scripts/superbuild

With the above the OSPRay apps will be built by default. As such, glm is required by apps/CMakeLists.txt

However, the scripts/superbuild/dependencies/dep_glm.cmake only does an "install" which is a copy of the source dir.

As such, there is no glmConfig.cmake file in the glm install dir and the glm is required by apps/CMakeLists.txt fails so cmake fails.

Workaround - do not build the apps.

Fix - scripts/superbuild/dependencies/dep_glm.cmake need to be updated to configure, build, and install glm properly.

paulmelis commented 3 years ago

Yup, ran into this as well with a superbuild on Linux

paulmelis commented 3 years ago

Fix - scripts/superbuild/dependencies/dep_glm.cmake need to be updated to configure, build, and install glm properly.

Btw, it might not involve all these steps, as GLM is a header-only library, so no configuring and building needed

paulmelis commented 3 years ago

Seems the glmConfig.cmake file isn't generated by default GLM. For the Arch package I see it is added as part of the Arch build (https://github.com/archlinux/svntogit-community/blob/packages/glm/trunk/PKGBUILD), with a remark that the install target was removed by the GLM devs for uncertain reasons

paulmelis commented 3 years ago

Ah, @Twinklebear is probably aware of this issue, given https://github.com/g-truc/glm/pull/966 :wink:

paulmelis commented 3 years ago

Wait, so the additions by @Twinklebear were merged in GLM on March 5th, 2020, but are no longer present in the master branch? Instead, they got superseded by https://github.com/g-truc/glm/pull/1054 on March, 8th 2021. Which mentions:

This PR should hopefully clear up some of the confusion about how CMake should be finding GLM. It should also make it easier for package maintainers with the packages being generated by CPack.

CMake should now be able to use its internal templates to generate appropriate glmConfig.cmake and glmConfigVersion.cmake files. Those files should be installed by the install target, as well as included in the CPack packages.

Just tested with a master checkout of GLM. A make install (after cmake configuring) will indeed generate the config files and store them in <dest>/lib/cmake/glm.

So indeed, @ARSanderson's "Fix - scripts/superbuild/dependencies/dep_glm.cmake need to be updated to configure, build, and install glm properly." is needed. Sorry for the noise :)

paulmelis commented 3 years ago

The machine I ran into this is not my usual development machine so I don't have a forked repo there setup for an easy pull request. So here's a quick patch that seems to fix the issue for me:

diff --git a/scripts/superbuild/dependencies/dep_glm.cmake b/scripts/superbuild/dependencies/dep_glm.cmake
index 5d7be4bd7..405190d0e 100644
--- a/scripts/superbuild/dependencies/dep_glm.cmake
+++ b/scripts/superbuild/dependencies/dep_glm.cmake
@@ -15,13 +15,11 @@ ExternalProject_Add(${COMPONENT_NAME}
   SOURCE_DIR ${COMPONENT_NAME}/src
   BINARY_DIR ${COMPONENT_NAME}
   URL "https://github.com/g-truc/glm/archive/master.zip"
-  CONFIGURE_COMMAND ""
-  BUILD_COMMAND ""
-  INSTALL_COMMAND "${CMAKE_COMMAND}" -E copy_directory
-    <SOURCE_DIR>
-    ${COMPONENT_PATH}
-  BUILD_ALWAYS OFF
+  CMAKE_ARGS
+    -DCMAKE_INSTALL_PREFIX:PATH=${COMPONENT_PATH}
+  BUILD_COMMAND ${DEFAULT_BUILD_COMMAND}
+  BUILD_ALWAYS ${ALWAYS_REBUILD}
 )

-list(APPEND CMAKE_PREFIX_PATH ${COMPONENT_PATH}/cmake/glm)
+list(APPEND CMAKE_PREFIX_PATH ${COMPONENT_PATH}/lib/cmake/glm)
 string(REPLACE ";" "|" CMAKE_PREFIX_PATH "${CMAKE_PREFIX_PATH}")
jeffamstutz commented 3 years ago

FWIW, this was fixed on devel by instead pointing to the glm release that had the non-install based config in it: https://github.com/ospray/ospray/commit/0278e6811504bb36ce6a8c0e697634d260800780

Edit: I'm no longer in a position to strongly state which approach is "preferred" though :)

johguenther commented 3 years ago

Yes, that commit fixed the superbuild in devel, but I'm not sure whether the original issue is solved with that fix, because of it "build but is not copied". Is it?

Twinklebear commented 3 years ago

Unfortunately at some point my glmConfig stuff got replaced by the "make install" style use they had before/now, so now glm has to be built and installed to generate the glmConfig files. That would probably need a fix to the library search path that we use and to actually make and install the glm library to generate the glmConfig.cmake as in @paulmelis 's diff above

Edit: I think that change woudl only be needed if we keep using master for glm, but on devel we've moved to pulling a specific version that still has my older CMake code in it.

Twinklebear commented 3 years ago

Checking on this now, the change in https://github.com/ospray/ospray/commit/0278e6811504bb36ce6a8c0e697634d260800780 does fix the superbuild, and it does install GLM in the installation directory. So I think until we move to a new release of glm that removes the cmake stuff what we have now is ok.

Later if we update to a newer glm version we'll need to build and install it again as in @paulmelis 's diff, but it's probably more stable to track actual releases of glm instead of master directly.

johguenther commented 2 years ago

We now use the latest stable GLM release, where the issue is not present yet. Once we update to a later GLM we need to apply above patch to make sure that GLM is installed for the CMake config to be generated.