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

OpenColorIO library CMakeLists.txt: Sets up GNUInstallDirs for non-win but completely ignores it in install(TARGETS... #1296

Closed
hobbes1069 opened this issue Jan 31, 2021 · 7 comments

Comments

@hobbes1069
Copy link
Contributor

if(NOT WIN32)
# Install the pkg-config file.
include(GNUInstallDirs)
set(prefix ${CMAKE_INSTALL_PREFIX})
set(exec_prefix "\${prefix}")
set(libdir "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}")
set(includedir "\${exec_prefix}/${CMAKE_INSTALL_INCLUDEDIR}")
configure_file(res/OpenColorIO.pc.in ${CMAKE_CURRENT_BINARY_DIR}/OpenColorIO.pc @ONLY)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/OpenColorIO.pc DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)
endif()

and then:

install(TARGETS OpenColorIO
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
ARCHIVE DESTINATION lib
)

Why go to the trouble of setting up the right directories and then just not use them?

@hobbes1069
Copy link
Contributor Author

I see this is still a problem in 2.1.0. Hardcoding install paths is BAD.

Here's my fix:

$ cat ocio-install.patch
Index: OpenColorIO-2.1.0/src/OpenColorIO/CMakeLists.txt
===================================================================
--- OpenColorIO-2.1.0.orig/src/OpenColorIO/CMakeLists.txt
+++ OpenColorIO-2.1.0/src/OpenColorIO/CMakeLists.txt
@@ -293,7 +293,7 @@ endif()
 
 install(TARGETS OpenColorIO
 	EXPORT ${PROJECT_NAME}_EXPORTED_TARGETS
-	LIBRARY DESTINATION lib
-	RUNTIME DESTINATION bin
-	ARCHIVE DESTINATION lib
+    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
 )

@remia
Copy link
Collaborator

remia commented Sep 1, 2021

Thanks for the suggestion @hobbes1069, I implemented that, among other things, in #1471 would be great if you could have a look.

@hobbes1069
Copy link
Contributor Author

LGTM!

@remia
Copy link
Collaborator

remia commented Sep 1, 2021

@hobbes1069 Would be curious if you have an example where the current hardcoded script fails or produce unexpected results, so that we are aware of what kind of changes we can expect for existing users? Thanks.

@hobbes1069
Copy link
Contributor Author

Specifically in my case as a Fedora Linux packager, libraries go into either /usr/lib or /usr/lib64 depending on if they're 32 or 64bit. Debian based systems have a different but equivalent file hierarchy. There's also a chance someone will want to build OCIO as a subproject, such as cross compiling from Linux to Windows using MinGW, and set some sort of custom path.

@remia
Copy link
Collaborator

remia commented Sep 1, 2021

Thanks, that makes sense, I was expecting Windows differences related to the build configuration but forgot about these architecture issues. Agree that the less we hardcode the better, especially when CMake override are available.

@hodoulp hodoulp reopened this Sep 1, 2021
@hodoulp
Copy link
Member

hodoulp commented Sep 1, 2021

I reopened the issue because the pull request is not yet merged.

@hodoulp hodoulp closed this as completed Oct 14, 2021
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

No branches or pull requests

3 participants