[Build] Build issue with modern cmake targets

[LazyDodo]

Describe the bug

I'm doing library upgrades for blender and have updated both our OIIO (to 2.2.15.1) and OSL (to 1.11.14.1) dependencies.

Leading to an odd build issue: ever since #2957 the OpenImageIO::OpenImageIO target inside OpenImageIOTargets.cmake gets populated as follows (abbreviated for clarity):

set_target_properties(OpenImageIO::OpenImageIO PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "OIIO_STATIC_DEFINE=1"
  INTERFACE_INCLUDE_DIRECTORIES [correct stuff here]
  INTERFACE_LINK_LIBRARIES [correct stuff here];\$<LINK_ONLY:PNG::PNG>;\$<LINK_ONLY:ZLIB::ZLIB>;[more correct stuff here]
)

Which is causing some issues for OSL, since OpenImageIOTargets.cmake makes no attempts to locate these targets, it is fine for the ZLIB target, since OSL has a checked_find_package (ZLIB REQUIRED) in its externalpackages.cmake PNG was not quite as lucky and the OSL builds fails during the cmake generation phase, since the PNG::PNG target is unknown.

Worked around the issue by adding a small patch to add checked_find_package (PNG REQUIRED) to OSL's externalpackages.cmake and adding a -DPNG_ROOT=... to our OSL build scripts.

While it solves our immediate problem, it's a "less great" solution, given it requires OSL having to know OIIO's linkage needs

To Reproduce Steps to reproduce the behavior:

1. Try building OIIO (2.2.15.1) and OSL (to 1.11.14.1) together.
2. That's it really.

Expected behavior

1. Glorious success

Evidence While configuring OSL with CMake.

  CMake Error at src/liboslcomp/CMakeLists.txt:19 (add_library):
    Target "oslcomp" links to target "PNG::PNG" but the target was not found.
    Perhaps a find_package() call is missing for an IMPORTED target, or an
    ALIAS target is missing?
[ above text 7x per target ]
  -- Generating done
  CMake Generate step failed.  Build files cannot be regenerated correctly.

Platform information:

• CMake version: 3.20.5
• OIIO branch/version: OIIO (2.2.15.1) and OSL (to 1.11.14.1)
• OS: Win10
• C++ compiler: MSVC 2017 15.9
• Any non-default build flags when you build OIIO: build config available here

[lgritz]

I don't believe that anything from PNG or ZLIB affects the public APIs, so I don't think those need to be explicitly passed to the downstream client of OIIO. So I'm not sure why those targets would end up in the exported OIIO config. But I'll look into it.

[LazyDodo]

Good point, should probably have mentioned that, we link oiio statically, so downstream needs to be informed about the linktime requirements, this is was working before #2957 but it sounds that may have been pure luck :)

[lgritz]

OK, static linkage is admitted not as thoroughly tested.

I guess that when you build OIIO, it understands that the libraries are static and so passes the targets to the downstream project... but as you point out, if the downstream project didn't already do a find_package to define the target, it wouldn't know what it means.

How does this usually work for CMake projects? What is the customary way to solve it?

[lgritz]

I think that before #2957, we used the variables like ZLIB_LIBRARIES, so it would have embedded the expansion of those, which is the full path.

I'll have to read up a bit on the cmake side of this. I thought switching from vars to targets was the modern way, but I'm not sure how that's supposed to work for static libs. Hmmm.

[LazyDodo]

I'm no CMake expert, Instinctively I want to stick a set(depname_ROOT where_ever_it_may_be) and find_package(depname) into the generated OpenImageIOTargets.cmake but I admit, my instincts are often wrong.

[LazyDodo]

Also it's good to note, for now i'm perfectly happy using the workaround I found, so there's no need to rush this, I filed the ticket more as a heads up of a potential issue. Take your time on this, there is no rush.

[lgritz]

Yeah, but I don't want that for all the dependent libraries (which do not need exposure through OIIO's public APIs) for the dynamic case. I think I have some reading to do. :-)

[lgritz]

Is there anything that still needs to be solved here?

[LazyDodo]

I no longer see this change in our patchset for oiio, so i guess this got solved somewhere down the line

[lgritz]

Fixing things without even knowing it is the best kind of fixing things!