OpenImageIO: conan 2 support, old version cleanup & version 2.4.17.0 & 2.5.6.0 added #19950
Conversation
Initial conversion to Conan 2.0 based on existing Conan 1.0 receip. Updating to OIIO 2.4.15.0 additionally to include patches integrated since last version.
Initial conversion to Conan 2.0 based on existing Conan 1.0 receip. Updating to OIIO 2.4.15.0 additionally to include patches integrated since last version.
…nter-index into oiio-v2.4.15.0-conan2.0
ghost
mentioned this pull request
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Any hint what is required for the CI to run/why it currently cannot be built? Just got approved and hour ago and repushed with the current master merged into my branch and it failed. Clicking on Details results in a "403 Forbidden" error. Btw. I saw that my quick fix for OpenColorIO requirements is not needed if #19549 is merged first which also updates OpenColorIO and looks good to me. Also a small question on the proper workflow here: I'm normally used to projects on non-public gitlab servers where the merge workflow is to have gitlab squash commits for a flat graph. How is that handled here? Squashing the pull request on merge or preparing it before merging? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hello @danimtb , @lasote @czoido @memsharded , |
recipes/openimageio/all/conanfile.py
Outdated
| @@ -63,260 +64,248 @@ class OpenImageIOConan(ConanFile): | |||
| "with_openvdb": False, # FIXME: broken on M1 | |||
| "with_ptex": True, | |||
| "with_libwebp": True, | |||
| "build_testing": False, | |||
There was a problem hiding this comment.
Any insight into why this would be needed in the recipe? The idea is to package the CCI libraries to be consumed, not tested/developed - I'm not against having the option here and will approve if we keep it, but would like some context :)
There was a problem hiding this comment.
I'm ok to completely remove it and just default the build for the tests to false internally as a fixed setting. I don't need it, was just assuming it might be wanted to have something to switch vendor cmake options to their default. Will remove it.
Would that mean in the commented thing below to set tc.variables["BUILD_TESTING"] = False or take it from tools.build:skip_test?
There was a problem hiding this comment.
Removed option and fixed to false
| tc.variables["EMBEDPLUGINS"] = True | ||
| tc.variables["USE_PYTHON"] = False | ||
| tc.variables["USE_EXTERNAL_PUGIXML"] = True | ||
| tc.variables["BUILD_MISSING_FMT"] = False |
There was a problem hiding this comment.
Is robinmap a necessary or optional lib?
The wording in https://github.com/AcademySoftwareFoundation/OpenImageIO/blob/43fea763945991d1b83f5c0bb65f12bfab09b99b/src/cmake/externalpackages.cmake#L300 makes it seem optional, and it currently is vendorized because we seem to lack that library in CCI (tracking in #22872)
There was a problem hiding this comment.
To my understanding it is needed and the code you linked allows between selecting an external one vs. having Cmake download a robinmap and build it itself. But maybe I'm missunderstanding something.
There was a problem hiding this comment.
Sorry, misunderstood that earlier. To my understanding it was by the changes and patches using tsl-robin-map as the robinmap. Not sure what the difference is if any to the MR you linked.
There was a problem hiding this comment.
So, testing is no fixed to false.
recipes/openimageio/all/conanfile.py
Outdated
| tc.variables["BUILD_MISSING_FMT"] = False | ||
|
|
||
| # The tests automatically download test images via git which is avoided by the default not to build the tests | ||
| tc.variables["BUILD_TESTING"] = self.options.build_testing |
There was a problem hiding this comment.
Should we maybe have tools.build:skip_test be reflected here?
There was a problem hiding this comment.
Maybe. I'm not fully sure how that works. I just did a best guess by looking at other related projects like openexr and the old conan 1 file. How would that best be done?
Co-authored-by: Rubén Rincón Blanco <git@rinconblanco.es>
conan-center-bot
added
Failed
Missing dependencies
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
conan-center-bot
removed
the
Missing dependencies
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 57 (
Conan v2 pipeline ✔️
All green in build 56 ( |
| - "For deveoper debugging/testing ONLY! Disable OCIO 2.2 builtin configs." OFF) | ||
| - if (OIIO_DISABLE_BUILTIN_OCIO_CONFIGS OR "$ENV{OIIO_DISABLE_BUILTIN_OCIO_CONFIGS}") | ||
| - add_compile_definitions(OIIO_DISABLE_BUILTIN_OCIO_CONFIGS) | ||
| +if (USE_OPENCOLORIO) |
There was a problem hiding this comment.
Just FYI, the checked_find_package macro itself internally checks USE_Package/USE_PACKAGE and disables if either exists and is set to 0. So I'm not sure that it was necessary to put the if (USE_PACKAGE) around all these various dependency calls.
There was a problem hiding this comment.
To be hones, I'm not sure, I just followed the existing patches for older versions.
Specify library name and version: openimageio/2.x
Main change: Conan 2 support.
Add 2.5.6.0 & 2.4.17.0, remove 2.2.7.0 & 2.2.18.0 & 2.3.7.2.
This pull request is adapting the existing openimageio recipe to Conan 2.0 for the newest version of OpenImageIO (as of starting the MR). I removed some old versions and offering 3 versions inside the 2.* major which I understand the FAQ to be requiring.