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

Adsk contrib - Add support for minimum and recommended versions for dependencies #1772

Conversation

cedrik-fuoco-adsk
Copy link
Contributor

@cedrik-fuoco-adsk cedrik-fuoco-adsk commented Feb 23, 2023

Adding support for minimum, maximum and recommended versions for OCIO dependencies.

  • Added two new macros to handle the logic: ocio_handle_dependency and ocio_install_dependency.
  • The download and install part of each OCIO dependency are now split into two files.
    • Under share/cmake/modules/FindXYZ.cmake and share/cmake/modules/install/InstallXYZ.cmake
  • Added an option OCIO_VERBOSE to print more information when CMake is calling the Find modules.
  • Refactored what gets printed when we build as well as some colours when modules are found.
  • FindOpenImageIO.cmake was removed in favour of their CMake configuration file (as recommended by OIIO). The FindOpenImageIO.cmake that we had was coming from OIIO originally, but it is not supported anymore.
  • FindOpenEXR.cmake was removed in favour of their CMake configuration files. Our FindOpenEXR.cmake was basically just doing a search for their CMake's configuration files, but we don't need a whole Find module to do that.

Here's a snippet of a build log:

image

Implemention of minimum, maximum and recommended version.
Refactor the FindOpenImageIO to be inline with the rest of the custom find module (OCIO_USE_OIIO_CMAKE_CONFIG is no longer needed).
Added a new option called OCIO_VERBOSE. It allow the user to tell OCIO to display more information when searching and building the dependencies.
Splitted the find and install part into two files - Find<pkg>.cmake and Install<pkg>.cmake.

Signed-off-by: Cédrik Fuoco <[email protected]>
Setting policy CMP0042 when building ZLIB since that project is using an old CMake version as the cmake_minimum_required and that version has no knowledge of the policy.
Fixing a potential issue in Installopenfx. Changing back the version to 1.4 as before.
Ignoring warning from OpenImageIO for imageioapphelpers. The same warning are ignored for ociodisplay, ocioconvert and ociolutimage.

Signed-off-by: Cédrik Fuoco <[email protected]>
…acOS.

Renamed FindOpenShadingLanguage to FindOSL to match the project name used the OSL's CMakefile.
Cleanup the code for FindOSL and removed duplicate warning about needed C++14.

Updated the header comments for all Find and Install modules.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Renamed ocio_install_package to ocio_install_dependency
The "Installing [...]" message from Install module is now under OCIO_VERBOSE variable. Added a message in ocio_install_dependency instead
Improve the colors usage in the logging

Signed-off-by: Cédrik Fuoco <[email protected]>
…ith the prefix of ocio_handle_dependency since they were the same.

Removed FindOpenEXR.cmake and FindOpenImageIO.cmake since they are not needed anymore (for differente reason).
Added PROMOTE_TARGET option for ocio_handle_dependency which promote the target to GLOBAL.

Signed-off-by: Cédrik Fuoco <[email protected]>
Re-worded some of the comments

Signed-off-by: Cédrik Fuoco <[email protected]>
…ting OCIO_INSTALL_EXT_PACKAGES option correctly.

Signed-off-by: Cédrik Fuoco <[email protected]>
…ib since the user can update their cmake in order to do that.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
@cedrik-fuoco-adsk
Copy link
Contributor Author

cedrik-fuoco-adsk commented Mar 8, 2023

I am working on the failure in the Linux CI. The issue comes up when OpenImageIO is found via OpenImageIOConfig.cmake.
I've identified the issue and I'm working on a solution. The issue is present in the main branch as well.

Copy link
Collaborator

@remia remia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @cedrik-fuoco-adsk!

Would be great to finally get rid of OIIO custom Find module, I think this was already tried a couple of times in the past. I would think it may be ok to bump the minimum version required if that helps in the process (for OCIO 2.3 obviously).

Thinking in terms of different compilation path now that we support more flexibility in version dependencies (tagging @Shootfast), is there something we could add to check what were the different dependencies versions from the compiled library? Not sure it's needed here for this PR but might be useful for debugging down the line. (Don't have a strong opinion about this, just raising the point. We already allowed flexible versions for Imath and OpenEXR before.)

share/cmake/modules/FindExtPackages.cmake Outdated Show resolved Hide resolved
src/libutils/imageioapphelpers/CMakeLists.txt Show resolved Hide resolved
share/cmake/modules/FindExtPackages.cmake Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
remia and others added 11 commits March 8, 2023 22:07
* Update CI workflows

Signed-off-by: Rémi Achard <[email protected]>

* Test building OpenEXR single threaded

Signed-off-by: Rémi Achard <[email protected]>

* Add comment on disabling parallel build

Signed-off-by: Rémi Achard <[email protected]>

* Update python_requires

Signed-off-by: Rémi Achard <[email protected]>

* Remove comments around CI jobs

Signed-off-by: Rémi Achard <[email protected]>

---------

Signed-off-by: Rémi Achard <[email protected]>
…ATH environment variable (AcademySoftwareFoundation#1759)

* Allow PyOpenColorIO module to load DLLs from Windows PATH environment variable with an opt-out option in case the user want the default behavior of Python 3.8+.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Fixing typos in comments

Signed-off-by: Cédrik Fuoco <[email protected]>

---------

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
…here ocio_install_dependency would be used without ocio_handle_dependency.

Added a new function named ocio_check_dependency_version to check the version of a package without propagating any variables set by find_package.
Fix for Linux CI.

Signed-off-by: Cédrik Fuoco <[email protected]>
Implemention of minimum, maximum and recommended version.
Refactor the FindOpenImageIO to be inline with the rest of the custom find module (OCIO_USE_OIIO_CMAKE_CONFIG is no longer needed).
Added a new option called OCIO_VERBOSE. It allow the user to tell OCIO to display more information when searching and building the dependencies.
Splitted the find and install part into two files - Find<pkg>.cmake and Install<pkg>.cmake.

Signed-off-by: Cédrik Fuoco <[email protected]>
Setting policy CMP0042 when building ZLIB since that project is using an old CMake version as the cmake_minimum_required and that version has no knowledge of the policy.
Fixing a potential issue in Installopenfx. Changing back the version to 1.4 as before.
Ignoring warning from OpenImageIO for imageioapphelpers. The same warning are ignored for ociodisplay, ocioconvert and ociolutimage.

Signed-off-by: Cédrik Fuoco <[email protected]>
…acOS.

Renamed FindOpenShadingLanguage to FindOSL to match the project name used the OSL's CMakefile.
Cleanup the code for FindOSL and removed duplicate warning about needed C++14.

Updated the header comments for all Find and Install modules.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Renamed ocio_install_package to ocio_install_dependency
The "Installing [...]" message from Install module is now under OCIO_VERBOSE variable. Added a message in ocio_install_dependency instead
Improve the colors usage in the logging

Signed-off-by: Cédrik Fuoco <[email protected]>
…ith the prefix of ocio_handle_dependency since they were the same.

Removed FindOpenEXR.cmake and FindOpenImageIO.cmake since they are not needed anymore (for differente reason).
Added PROMOTE_TARGET option for ocio_handle_dependency which promote the target to GLOBAL.

Signed-off-by: Cédrik Fuoco <[email protected]>
Re-worded some of the comments

Signed-off-by: Cédrik Fuoco <[email protected]>
…ting OCIO_INSTALL_EXT_PACKAGES option correctly.

Signed-off-by: Cédrik Fuoco <[email protected]>
…ib since the user can update their cmake in order to do that.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
…here ocio_install_dependency would be used without ocio_handle_dependency.

Added a new function named ocio_check_dependency_version to check the version of a package without propagating any variables set by find_package.
Fix for Linux CI.

Signed-off-by: Cédrik Fuoco <[email protected]>
…rsions-for-dependencies' of https://github.com/autodesk-forks/OpenColorIO into adsk_contrib/add-support-for-minimum-and-recommended-versions-for-dependencies
Implemention of minimum, maximum and recommended version.
Refactor the FindOpenImageIO to be inline with the rest of the custom find module (OCIO_USE_OIIO_CMAKE_CONFIG is no longer needed).
Added a new option called OCIO_VERBOSE. It allow the user to tell OCIO to display more information when searching and building the dependencies.
Splitted the find and install part into two files - Find<pkg>.cmake and Install<pkg>.cmake.

Signed-off-by: Cédrik Fuoco <[email protected]>
Setting policy CMP0042 when building ZLIB since that project is using an old CMake version as the cmake_minimum_required and that version has no knowledge of the policy.
Fixing a potential issue in Installopenfx. Changing back the version to 1.4 as before.
Ignoring warning from OpenImageIO for imageioapphelpers. The same warning are ignored for ociodisplay, ocioconvert and ociolutimage.

Signed-off-by: Cédrik Fuoco <[email protected]>
…acOS.

Renamed FindOpenShadingLanguage to FindOSL to match the project name used the OSL's CMakefile.
Cleanup the code for FindOSL and removed duplicate warning about needed C++14.

Updated the header comments for all Find and Install modules.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Renamed ocio_install_package to ocio_install_dependency
The "Installing [...]" message from Install module is now under OCIO_VERBOSE variable. Added a message in ocio_install_dependency instead
Improve the colors usage in the logging

Signed-off-by: Cédrik Fuoco <[email protected]>
…ith the prefix of ocio_handle_dependency since they were the same.

Removed FindOpenEXR.cmake and FindOpenImageIO.cmake since they are not needed anymore (for differente reason).
Added PROMOTE_TARGET option for ocio_handle_dependency which promote the target to GLOBAL.

Signed-off-by: Cédrik Fuoco <[email protected]>
Re-worded some of the comments

Signed-off-by: Cédrik Fuoco <[email protected]>
…ting OCIO_INSTALL_EXT_PACKAGES option correctly.

Signed-off-by: Cédrik Fuoco <[email protected]>
…ib since the user can update their cmake in order to do that.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
…here ocio_install_dependency would be used without ocio_handle_dependency.

Added a new function named ocio_check_dependency_version to check the version of a package without propagating any variables set by find_package.
Fix for Linux CI.

Signed-off-by: Cédrik Fuoco <[email protected]>
…rsions-for-dependencies' of https://github.com/autodesk-forks/OpenColorIO into adsk_contrib/add-support-for-minimum-and-recommended-versions-for-dependencies

Signed-off-by: Cédrik Fuoco <[email protected]>
@cedrik-fuoco-adsk
Copy link
Contributor Author

Sorry, I messed up my branch while trying to rebase. I might have to do a redo.

@cedrik-fuoco-adsk
Copy link
Contributor Author

Closing in favour of #1777 (redo)

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

Successfully merging this pull request may close these issues.

4 participants