Skip to content

Fix OpenImageIO dependency specification - #2479

Closed
marcin-serwin wants to merge 2 commits into
AcademySoftwareFoundation:mainfrom
marcin-serwin:push-mxvuwzryoowp
Closed

marcin-serwin wants to merge 2 commits into
AcademySoftwareFoundation:mainfrom
marcin-serwin:push-mxvuwzryoowp

Conversation

@marcin-serwin

Copy link
Copy Markdown

The cmake module in External/OpenImageIO was deleted in 1.39.0 version.

Since the OpenImageIO package is a link time dependency it needs to be present as a dependency when MaterialX is used as a library.

Signed-off-by: Marcin Serwin <marcin@serwin.dev>
Signed-off-by: Marcin Serwin <marcin@serwin.dev>
@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@jstone-lucasfilm

Copy link
Copy Markdown
Member

@marcin-serwin This proposed change looks very reasonable to me, and there are instructions in the comment above on how to address the CLA issue:

#2479 (comment)

Let us know how it goes!

@marcin-serwin

Copy link
Copy Markdown
Author

I'm not signing the CLA. I release this patch into the public domain, so you can do with it what you please.

@jstone-lucasfilm

Copy link
Copy Markdown
Member

@marcin-serwin Alas, I don't believe we're allowed to accept contributions without a CLA, but I understand your point of view. Perhaps you can encourage another developer to contribute this change on your behalf?

@marcin-serwin

Copy link
Copy Markdown
Author

Feel free to contribute it yourself.

@jstone-lucasfilm

Copy link
Copy Markdown
Member

Thanks for this original suggestion, @marcin-serwin, and I believe we can now close this PR in favor of #2495.

@marcin-serwin

Copy link
Copy Markdown
Author

The other PR does not fix the issue with downstream dependencies needing to link to OIIO. I still get the following error when building OpenUSD with MaterialX taken from main:

CMake Error at /nix/store/ngk8fkfn34k9ahvfd3mq5gi17rlbz59k-python3.11-materialx-1.39.3/lib/cmake/MaterialX/MaterialXTargets.cmake:222>
  The link interface of target "MaterialXRender" contains:

    OpenImageIO::OpenImageIO

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /nix/store/ngk8fkfn34k9ahvfd3mq5gi17rlbz59k-python3.11-materialx-1.39.3/lib/cmake/MaterialX/MaterialXConfig.cmake:36 (include)
  cmake/defaults/Packages.cmake:300 (find_package)
  CMakeLists.txt:23 (include)

set_and_check(MATERIALX_PYTHON_DIR "@PACKAGE_CMAKE_INSTALL_PREFIX@/python")
endif()

if(@MATERIALX_BUILD_OIIO@)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @marcin-serwin , I suspect this should be
if(@MATERIALX_BUILD_OIIO@ AND NOT @MATERIALX_BUILD_SHARED_LIBS@)
because static libraries do need to declare transitive dependencies.

Thanks for showing us the way to declare transitive link dependencies. That is a part of CMake that was unknown to me.

MaterialX libraries do not require declaring dependencies when they are built as shared libraries. You will notice that OiioImageLoader.h does not leak OIIO symbols, same thing for OcioColorManagementSystem.h which does not leak OCIO symbols. It is only when linking against a static MaterialXRender.a that a downstream project needs to know about OIIO.

I will do some experiments and eventually repost that fix (with OCIO included) in order to satisfy the CLAs once I have examined the generated CMake config this produces.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you, I missed that it's building static libs by default. I wonder why it wasn't necessary to link OIIO downstream before 1.39 even with static libs.

@JGamache-autodesk

Copy link
Copy Markdown
Contributor

Hi @marcin-serwin I have repackaged your fix in #2497. Can you code review please?

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.

3 participants