build: Use target_compile_options

[donny-dont]

Description

When using pkgconfig the value of <NAME>_CFLAGS_OTHER can contain compiler flags along with preprocessor definitions. If there is a compiler flag and its passed to target_compile_definitions then it will be treated as if it is a preprocessor definition.

Found a case where a statically compiled LibRaw with a statically compiled Little-CMS caused LibRaw_DEFINITIONS to have -pthread in its listing which was passed to the compiler as a definition, -D-pthread. This caused a build failure with auto-moc.

Add an additional parameter for the add_oiio_plugin macro to take COMPILE_OPTIONS and pass those to target_compile_options which can differentiate between compiler flags and preprocessor definitions. Pass LibRaw_DEFINITIONS to the CMake macro to prevent the above error.

Tests

Issue found while updating Little-CMS in microsoft/vcpkg#42187 and the change fixes the build issue found there. This is an attempt to upstream it so the issue is fixed.

Checklist:

• I have read the contribution guidelines.
• I have updated the documentation, if applicable. (Check if there is no
  need to update the documentation, for example if this is a bug fix that
  doesn't change the API.)
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• If I added or modified a C++ API call, I have also amended the
  corresponding Python bindings (and if altering ImageBufAlgo functions, also
  exposed the new functionality as oiiotool options).
• My code follows the prevailing code style of this project. If I haven't
  already run clang-format before submitting, I definitely will look at the CI
  test that runs clang-format and fix anything that it highlights as being
  nonconforming.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: donny-dont / name: Don Olmstead (4e6a2ac)

[lgritz]

This looks reasonable, thanks for the fix.
Can you please affix the DCO to your commit and re-push? Thanks.

[donny-dont]

This looks reasonable, thanks for the fix. Can you please affix the DCO to your commit and re-push? Thanks.

Done. Thanks for the speedy review!

[donny-dont]

Ok looks like everything is happy @lgritz

[lgritz]

LGTM, thanks for the fix!

[lgritz]

I'll backport this to the next 3.0 and 2.5 patches.

[donny-dont]

Thanks @lgritz ! I will update the vcpkg port once you get it on the 3.0 branch. You might want to look at https://github.com/microsoft/vcpkg/tree/master/ports/openimageio to see if there are any other patches that might be of interest to the project there.

[lgritz]

@donny-dont

Quick glance:

• fix-dependencies.patch : I'm not sure I understand why those find_dependency calls are needed in the exported cmake config if those packages' types or headers aren't mentioned in our public API headers. I think on Linux/MacOS, .so dependencies are transitively understood by the linker, and .a dependencies will get folded into our library. So is this a Windows-specific need? I'm also not sure exactly why the other changes here are needed.
• fix-openexr-target-missing.patch : Same as above, I'm not sure if this is still needed, since OIIO doesn't use any OpenEXR types in its public APIs.
• fix-openimageio_include_dir.patch : maybe? I'm not exactly sure I understand what that does. Did we just get the path wrong? If we never noticed the problem before, does it simply mean that the line is not needed at all?
• imath-version-guard.patch : irrelevant now that OIIO no longer support Imath 2.x.

It's hard to evaluate without understanding exactly why each of these change were made. But in general, any of them that are still relevant and necessary, I think we'd be happy to have them "upstreamed". It seems more maintainable for our build scripts to work for vcpkg out of the box, even if they have an occasional "if vcpkg" logic somewhere, than to have a lot of patches added on your end that we could break at any time without really being aware that they exist.

I'd prefer that everything be attempted to be upstreamed, and only have vcpkg-side patches for the few things that for whatever reason we aren't able to accept.

[BillyONeal]

@lgritz In general we try to avoid patching anything that would result in 'putting words in one's mouth'; build system changes that adapt how dependencies are found to the way they are deployed in vcpkg we will often just take, but if there's something that would be very visible to users or touches sources meaningfully we're going to want upstream to at least look first.

https://learn.microsoft.com/vcpkg/contributing/maintainer-guide#patching

fix-dependencies.patch : I'm not sure I understand why those find_dependency calls are needed in the exported cmake config if those packages' types or headers aren't mentioned in our public API headers. I think on Linux/MacOS, .so dependencies are transitively understood by the linker, and .a dependencies will get folded into our library. So is this a Windows-specific need? I'm also not sure exactly why the other changes here are needed.
fix-openexr-target-missing.patch : Same as above, I'm not sure if this is still needed, since OIIO doesn't use any OpenEXR types in its public APIs.

1. I don't know about whether things get 'folded into' your libraries, but that certainly isn't how it normally works for .as or static .libs. (I haven't dug into whether openimageio is doing something different or special to merge the dependent .as together. If it is doing that, we would probably have to patch that out in vcpkg because vcpkg customers might want to choose different versions of those dependencies or similar, but we wouldn't bother you trying to upstream that as we would understand that to be a specific-to-vcpkg problem)
2. CMake will want to see any targets you've target_link_libraries'd when your exported targets are loaded even if that results in no actual compiler or linker command line changes.
3. CMake bindings can export things like requirements on compiler switches unrelated to what would end up on your link line.

fix-openimageio_include_dir.patch : maybe? I'm not exactly sure I understand what that does. Did we just get the path wrong? If we never noticed the problem before, does it simply mean that the line is not needed at all?

Hmmm this should have probably been submitted upstream. I don't know exactly why it was added either in https://github.com/microsoft/vcpkg/pull/27766/files .

imath-version-guard.patch : irrelevant now that OIIO no longer support Imath 2.x.

I'm not sure I understand; as far as I can tell that OIIO no longer supports Imath 2.x is exactly why this patch is here, to warn people that that combination will explode.