AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.76k stars 438 forks source link

The *.cmake files installs to incorrect location. #500

Closed ghost closed 6 years ago

ghost commented 6 years ago

Check the problem from the footprint in commit.

The three cmake files OpenColorIOConfig.cmake OpenColorIO-relwithdebinfo.cmake OpenColorIO.cmake are installed either under /usr or under /usr/cmake, which is incorrect. I am not familiar with cmake, but I guess the correct location should be ${CMAKE_INSTALL_LIBDIR}/cmake/opencolorio/ . Any comment on this?

dracwyrm commented 6 years ago

Actually, include( GNUInstallDirs ) ${CMAKE_INSTALL_LIBDIR}/cmake/OpenColorIO

OpenColorIO CMake files are not using GNUInstallDirs. I was going to submit a PR to convert the hard coded paths to variables:

diff -purN a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt    2017-11-29 22:51:15.000000000 +0000
+++ b/CMakeLists.txt    2017-12-27 16:49:51.461300828 +0000
@@ -59,6 +59,7 @@ endif()
 include(ParseArguments)
 include(OCIOMacros)
 include(ExternalProject)
+include(GNUInstallDirs)

 enable_language(CXX)

@@ -531,7 +532,7 @@ endif()
 configure_file(${CMAKE_SOURCE_DIR}/share/ocio/setup_ocio.sh.in
     ${CMAKE_CURRENT_BINARY_DIR}/share/ocio/setup_ocio.sh @ONLY)

-INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/share/ocio/setup_ocio.sh DESTINATION share/ocio/)
+INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/share/ocio/setup_ocio.sh DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/ocio/)

 ###############################################################################
 ### CPACK ###
@@ -646,4 +647,4 @@ file(WRITE "${CMAKE_BINARY_DIR}/OpenColo
     message(STATUS OPENCOLORIO_FOUND=\${OPENCOLORIO_FOUND})
     "
 )
-install(FILES "${CMAKE_BINARY_DIR}/OpenColorIOConfig.cmake" DESTINATION .)
+install(FILES "${CMAKE_BINARY_DIR}/OpenColorIOConfig.cmake" DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/OpenColorIO)
diff -purN a/docs/CMakeLists.txt b/docs/CMakeLists.txt
--- a/docs/CMakeLists.txt   2017-11-29 22:51:15.000000000 +0000
+++ b/docs/CMakeLists.txt   2017-12-27 16:53:28.976491353 +0000
@@ -122,7 +36,7 @@ else()
 endif()

 add_custom_target(doc ALL
-    COMMAND ${PYT_PRE_CMD} ${EXTDIST_BINPATH}/sphinx-build -b html . ${CMAKE_CURRENT_BINARY_DIR}/build-html
+    COMMAND sphinx-build -b html . ${CMAKE_CURRENT_BINARY_DIR}/build-html
     DEPENDS
         ${DEPLIBS}
         ${CMAKE_BINARY_DIR}/docs/conf.py
@@ -133,20 +47,18 @@ add_custom_target(doc ALL
     COMMENT "Building html docs"
     SOURCES ${DOCFILES})

-# note: ExternalProject will not build when added to a add_custom_target this
-# works around this problem. This seems to be fixed in the cmake ^HEAD
-add_dependencies(doc Sphinx) 
-
 install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/build-html/
-        DESTINATION ${CMAKE_INSTALL_PREFIX}/share/doc/OpenColorIO/html
+        DESTINATION ${CMAKE_INSTALL_DOCDIR}/html
         PATTERN .* EXCLUDE
 )

+if(OCIO_BUILD_PDF_DOCS)
+
 find_package(LATEX)
 if(PDFLATEX_COMPILER)

     add_custom_target(latex
-        COMMAND ${PYT_PRE_CMD} ${EXTDIST_BINPATH}/sphinx-build -b latex . ${CMAKE_CURRENT_BINARY_DIR}/build-latex
+        COMMAND sphinx-build -b latex . ${CMAKE_CURRENT_BINARY_DIR}/build-latex
         DEPENDS
             OpenColorIO
             ${CMAKE_BINARY_DIR}/docs/conf.py
@@ -156,7 +68,6 @@ if(PDFLATEX_COMPILER)
             ${RSTDOC_OUTPUT}
         COMMENT "Building latex doc"
         SOURCES ${DOCFILES})
-    add_dependencies(latex Sphinx)

     add_custom_target(pdf ALL
         COMMAND ${PDFLATEX_COMPILER} OpenColorIO.tex
@@ -166,6 +77,8 @@ if(PDFLATEX_COMPILER)
     add_dependencies(pdf latex)

     install(FILES ${CMAKE_CURRENT_BINARY_DIR}/build-latex/OpenColorIO.pdf
-            DESTINATION ${CMAKE_INSTALL_PREFIX}/share/doc/OpenColorIO/)
+            DESTINATION ${CMAKE_INSTALL_DOCDIR})

 endif()
+
+endif()

You can use that patch until I submit the PR, but it's tweaked for Gentoo Linux.

ghost commented 6 years ago

Thanks for the patch. GNUInstallDirs Module will set ${CMAKE_INSTALL_LIBDIR} to ${CMAKE_INSTALL_PREFIX}/lib64 for x86_64, and for distribution without ${CMAKE_INSTALL_PREFIX}/lib64 , that will cause files out of sync. So i think it's better not to include GNUInstallDirs, just go with install(FILES "${CMAKE_BINARY_DIR}/OpenColorIOConfig.cmake" DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/OpenColorIO) and let users define ${CMAKE_INSTALL_LIBDIR} wrt their distribution design(or FHS) when they compile OpebColorIO.

ghost commented 6 years ago

Also, there are still OpenColorIO-relwithdebinfo.cmake and OpenColorIO.cmake left under /usr/cmake, I don't know where those two come from. Do you have a patch for them as well?

dracwyrm commented 6 years ago

@rtlanceroad The distribution and OSes define what the GNUInstallDirs variables are. They should be defined as X86_64 goes to /usr/lib64 and 32 bit goes to /usr/lib even on a 64 bit system. Unless it's a distro that puts things in non GNU locations. You may need to ask your distro devs how to activate the variables for 32 bit compiles on a 64 bit system. On Gentoo, since we compile everything, our system makes sure the correct variables are always used, so GNUInstallDirs in a build system is a God send as we don't need to manually define everything. It's a complete standard, so it should work correctly on all systems, even MS Windows and MacOS.

I'll investigate those other two files. I missed those, so thank you for pointing those out.

EDIT: I found where the other files are being installed at and will update patch after a round of testing.

dracwyrm commented 6 years ago

Here is the second patch. I think everything is in order. Note that I add a variable to control PDF generation because the huge dependency on Latex. Also, I had to hand edit a hunk to make it more other distro friendly as for the Gentoo ebuild, all the bundled externals are removed and use our own installed versions. Can you check the validity of the installed .cmake files? This is the full patch I use: https://github.com/dracwyrm/gentoo-ebuilds/blob/master/media-libs/opencolorio/files/opencolorio-1.1.0-cmake-fixes-1.patch

If this works, I'll make a PR with a general distro friendly GNUInstallDirs fix. :)

diff -purN a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt    2018-01-05 01:38:27.000000000 +0000
+++ b/CMakeLists.txt    2018-01-14 09:56:38.848398729 +0000
@@ -59,6 +59,7 @@ endif()
 include(ParseArguments)
 include(OCIOMacros)
 include(ExternalProject)
+include(GNUInstallDirs)

 enable_language(CXX)

@@ -531,7 +532,7 @@ endif()
 configure_file(${CMAKE_SOURCE_DIR}/share/ocio/setup_ocio.sh.in
     ${CMAKE_CURRENT_BINARY_DIR}/share/ocio/setup_ocio.sh @ONLY)

-INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/share/ocio/setup_ocio.sh DESTINATION share/ocio/)
+INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/share/ocio/setup_ocio.sh DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/ocio/)

 ###############################################################################
 ### CPACK ###
@@ -596,7 +597,7 @@ if(TARGET OpenColorIO_STATIC)
         set(OCIO_STATIC_COMPILE_DEFINITIONS )
     endif()
 endif()
-install(EXPORT OpenColorIO DESTINATION cmake)
+install(EXPORT OpenColorIO DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/OpenColorIO)
 file(WRITE "${CMAKE_BINARY_DIR}/OpenColorIOConfig.cmake"
     "
     get_filename_component(OpenColorIO_DIR \"\${CMAKE_CURRENT_LIST_FILE}\" PATH)
@@ -608,7 +609,7 @@ file(WRITE "${CMAKE_BINARY_DIR}/OpenColo

     ## targets libraries + associated definitions
     if(NOT TARGET OpenColorIO)
-        include(\"\${OpenColorIO_DIR}/cmake/OpenColorIO.cmake\") ## thanks to imported target
+        include(\"\${OpenColorIO_DIR}/${CMAKE_INSTALL_LIBDIR}/cmake/OpenColorIO/OpenColorIO.cmake\") ## thanks to imported target
         if(TARGET OpenColorIO AND NOT OpenColorIO_USE_STATIC)
             message(STATUS \"shared target OpenColorIO : see OpenColorIO_LIBRARY\")
             set(OpenColorIO_LIBRARY         OpenColorIO)
@@ -646,4 +647,4 @@ file(WRITE "${CMAKE_BINARY_DIR}/OpenColo
     message(STATUS OPENCOLORIO_FOUND=\${OPENCOLORIO_FOUND})
     "
 )
-install(FILES "${CMAKE_BINARY_DIR}/OpenColorIOConfig.cmake" DESTINATION .)
+install(FILES "${CMAKE_BINARY_DIR}/OpenColorIOConfig.cmake" DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/OpenColorIO)
diff -purN a/docs/CMakeLists.txt b/docs/CMakeLists.txt
--- a/docs/CMakeLists.txt   2018-01-05 01:38:27.000000000 +0000
+++ b/docs/CMakeLists.txt   2018-01-14 09:40:29.103331426 +0000
@@ -132,10 +132,12 @@ add_custom_target(doc ALL 
 add_dependencies(doc Sphinx)

 install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/build-html/
-        DESTINATION ${CMAKE_INSTALL_PREFIX}/share/doc/OpenColorIO/html
+        DESTINATION ${CMAKE_INSTALL_DOCDIR}/html
         PATTERN .* EXCLUDE
 )

+if(OCIO_BUILD_PDF_DOCS)
+
 find_package(LATEX)
 if(PDFLATEX_COMPILER)

@@ -166,6 +167,8 @@ if(PDFLATEX_COMPILER)
     add_dependencies(pdf latex)

     install(FILES ${CMAKE_CURRENT_BINARY_DIR}/build-latex/OpenColorIO.pdf
-            DESTINATION ${CMAKE_INSTALL_PREFIX}/share/doc/OpenColorIO/)
+            DESTINATION ${CMAKE_INSTALL_DOCDIR})

 endif()
+
+endif()

The file listing of my installed files since I don't install the PDF:

# equery f opencolorio            
 * Searching for opencolorio ...
 * Contents of media-libs/opencolorio-1.1.0-r1:
/usr
/usr/bin
/usr/bin/ociobakelut
/usr/bin/ociocheck
/usr/bin/ocioconvert
/usr/bin/ociodisplay
/usr/bin/ociolutimage
/usr/include
/usr/include/OpenColorIO
/usr/include/OpenColorIO/OpenColorABI.h
/usr/include/OpenColorIO/OpenColorIO.h
/usr/include/OpenColorIO/OpenColorTransforms.h
/usr/include/OpenColorIO/OpenColorTypes.h
/usr/include/PyOpenColorIO
/usr/include/PyOpenColorIO/PyOpenColorIO.h
/usr/lib64
/usr/lib64/cmake
/usr/lib64/cmake/OpenColorIO
/usr/lib64/cmake/OpenColorIO/OpenColorIO-gentoo.cmake
/usr/lib64/cmake/OpenColorIO/OpenColorIO.cmake
/usr/lib64/cmake/OpenColorIO/OpenColorIOConfig.cmake
/usr/lib64/libOpenColorIO.so -> libOpenColorIO.so.1
/usr/lib64/libOpenColorIO.so.1 -> libOpenColorIO.so.1.1.0
/usr/lib64/libOpenColorIO.so.1.1.0
/usr/lib64/pkgconfig
/usr/lib64/pkgconfig/OpenColorIO.pc
/usr/lib64/python3.5
/usr/lib64/python3.5/site-packages
/usr/lib64/python3.5/site-packages/PyOpenColorIO.so
/usr/share
/usr/share/doc
/usr/share/doc/opencolorio-1.1.0-r1
/usr/share/doc/opencolorio-1.1.0-r1/ChangeLog.bz2
/usr/share/doc/opencolorio-1.1.0-r1/README.md.bz2
/usr/share/ocio
/usr/share/ocio/setup_ocio.sh
ghost commented 6 years ago

Thanks for your work. I adopted your partial patch and applied it, and it fixed my problem. You can see it in commit . About the GNUInstallDirs module, I think it's ok because I believe most of distributions have /usr/lib64 -> lib link and that keeps files not out of sync, otherwise, it needs users to override the ${CMAKE_INSTALL_PREFIX} and ${CMAKE_INSTALL_LIBDIR} variables. And PDF, hmmm, I don't know about it... Anyway go ahead and see how developers say.

dracwyrm commented 6 years ago

@rtlanceroad Glad it worked for you and thanks for testing. I'll make a PR with this fix.

scoopxyz commented 6 years ago

Thanks for looping back with the issue, and thanks @dracwyrm for the PR!

This will be reviewed for the 1.1.1 patch release

SoapGentoo commented 6 years ago

@rtlanceroad the /usr/lib64 -> lib that some distributions have (including Gentoo still) is of an historical nature. Back when amd64 hit distributions, 90% of build systems just installed stuff blindly into lib/, and creating the /usr/lib64 -> lib symlink allowed distributions to quickly make their distributions work on 64-bit systems, albeit with a crutch. CMake's GNUInstallDirs module is actually correct here, because /usr/lib should not be used as a general purpose 64-bit lib dir, as for instance also documented in the FHS: http://www.pathname.com/fhs/pub/fhs-2.3.html#LIB64 Anyhow, most distributions have hooks to explicitly set CMAKE_INSTALL_LIBDIR (which is why GNUInstallDirs is so important to us), hence most of this is not a problem really.

ghost commented 6 years ago

Since patches are merged, I'm gonna close this.