CMake generated config files should be relocatable

foundry-markf commented 2 years ago

Bug Report

Build Problem

It is build related, but only observable by the consumer of the CMake package.

The CMakeLists.txt in the project have install and export commands that generate CMake config files that are written to share/opentime and share/opentimelineio in the install step. The pairs of files in these directories both have an issue in that the paths they contain, specifically _IMPORT_PREFIX and those for C++ include directories, and the IMPORTED_LOCATION properties have absolute paths to the machine on which they are built.

For opentime: The _IMPORT_PREFIX is due to the absolute destination path in https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/main/src/opentime/CMakeLists.txt#L51-L53

The include and lib directories are due to the absolute paths in https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/main/src/opentime/CMakeLists.txt#L44-L49.

There are equivalent statements for opentimelineio.

Our usage of thirdparty libraries is to build on canonical build machines, upload binaries to a package manager, which is then served to many clients, so relocatability is important.

My recommendation, which I've applied a patch for locally, is to make all those directory specifications relative, which in turn adds code to the generated config file to dynamically determine _IMPORT_PREFIX from the location of the config file, and all other paths are then relative to that.

Here's a relevant snippet of my patch for one of the config files for your consideration, as something that worked for us:

--- a/src/opentime/CMakeLists.txt
+++ b/src/opentime/CMakeLists.txt
@@ -41,15 +41,20 @@ if(OTIO_CXX_INSTALL)
     install(FILES ${OPENTIME_HEADER_FILES} 
             DESTINATION "${OTIO_RESOLVED_CXX_INSTALL_DIR}/include/opentime")

+    # make install directories relative, so that the generated config file will be relative to the dynamic prefix
+    file(RELATIVE_PATH config_include_dir ${OTIO_RESOLVED_CXX_INSTALL_DIR} ${OTIO_RESOLVED_CXX_INSTALL_DIR}/include)
+    file(RELATIVE_PATH config_lib_dir ${OTIO_RESOLVED_CXX_INSTALL_DIR} ${OTIO_RESOLVED_CXX_DYLIB_INSTALL_DIR})
+    file(RELATIVE_PATH config_dir ${OTIO_RESOLVED_CXX_INSTALL_DIR} ${OTIO_RESOLVED_CXX_INSTALL_DIR}/share/opentime)
+
     install(TARGETS opentime
             EXPORT OpenTimeConfig
-            INCLUDES DESTINATION "${OTIO_RESOLVED_CXX_INSTALL_DIR}/include"
-            ARCHIVE DESTINATION "${OTIO_RESOLVED_CXX_DYLIB_INSTALL_DIR}"
-            LIBRARY DESTINATION "${OTIO_RESOLVED_CXX_DYLIB_INSTALL_DIR}"
-            RUNTIME DESTINATION "${OTIO_RESOLVED_CXX_DYLIB_INSTALL_DIR}")
+            INCLUDES DESTINATION "${config_include_dir}"
+            ARCHIVE DESTINATION "${config_lib_dir}"
+            LIBRARY DESTINATION "${config_lib_dir}"
+            RUNTIME DESTINATION "${config_lib_dir}")

     install(EXPORT OpenTimeConfig
-            DESTINATION "${OTIO_RESOLVED_CXX_INSTALL_DIR}/share/opentime"
+            DESTINATION "${config_dir}"
             NAMESPACE OTIO:: )
 endif()

Thanks, Mark

Expected Behavior

I would expect a CMake package generated to be relocatable to another compatible machine and still be consumable.

meshula commented 2 years ago

Is there an earlier place in the code to apply the RELATIVE_PATH keyword, such that the OTIO_RESOLVED variables are themselves relative? Now that the cmake and setup.py scripts are cleaner than they used to be, perhaps the need for fully qualified paths is gone? I'm not sure, but think it's worth looking at.

foundry-markf commented 2 years ago

It's an interesting question. At the time of investigation, I didn't look further, as I didn't want to break any assumptions there might be on other uses of OTIO_RESOLVED and friends. So I made the change as local as possible.

AcademySoftwareFoundation / OpenTimelineIO