Repository navigation
Recreate pkg-config dependency targets in OpenEXRConfig.cmake - #2673
Conversation
When libdeflate, OpenJPH, or zstd is found via pkg-config rather than its CMake config, a static OpenEXRCore links to it through a PkgConfig:: imported target. OpenEXRConfig.cmake never recreated those targets, so downstream find_package(OpenEXR) failed with "The link interface of target OpenEXR::OpenEXRCore contains PkgConfig::libzstd but the target was not found". For libdeflate, the config also checked libdeflate_FOUND, which the pkg-config path does not set; for OpenJPH it called find_dependency(openjph), which requires a CMake config. For static builds, recreate the PkgConfig:: targets with pkg_check_modules; otherwise keep the existing find_dependency calls. Assisted-by: Claude Code / Claude Opus 5.5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Full disclosure: I used Claude for this, and it's truly at the very edge of my own understanding of CMake, pkgconfig, and static libraries. I definitely want another pair of critical eyes on this, because I'm not as confident in my understanding as I usually insist that people be when they are submitting LLM-assisted PRs. On the other hand, it feels spiritually equivalent to "I found this fix on Stack Overflow, it seems to fix the problem adequately, but I can't honestly say that I can explain exactly why." And I only would have felt a little sheepish about saying that a year ago about some deep cmake fix. |
|
I believe that the "CI-old" tests are failing for GitHub reasons, and not in any way related to this PR. OIIO's jobs on the old aswf-docker containers also started failing spontaneously this week, and I just got rid of the tests, since 2021/2022 is outside the support range within which I am willing to bend over backwards to make those old CentOS containers keep working. |
|
I've probably overstated the case above -- I really do understand why this solution is necessary. But I would not have been able to come up with it myself without sinking way more time into it than I'm willing to do. |
| if (NOT @BUILD_SHARED_LIBS@) | ||
| openexr_find_pkgconfig_dependency(deflate libdeflate) | ||
| endif() | ||
| elseif (@libdeflate_FOUND@) |
There was a problem hiding this comment.
| elseif (@libdeflate_FOUND@) | |
| elseif ("@libdeflate_FOUND@") |
I did some querying with Claude, and it reports that this line produces a warning when libdeflate_FOUND is false:
CMake Warning (dev) ... elseif (): ELSEIF called with no arguments, it will be skipped.
I haven't seen the warning myself, so I can't confirm, but it suggests adding "", which gives the statement a real argument.
There was a problem hiding this comment.
I don't understand what quoting it does, but as far as I'm concerned, we can sign off and apply the suggestions and see if it passes CI. If it does, I suppose there's no reason not to do it?
There was a problem hiding this comment.
Did you get the warning above, or any other warnings coming from the elseif ()? If not, let's just leave it as is.
There was a problem hiding this comment.
It's really hard for met to tell, Cary. The specific circumstance of this is in my CI where my build system does its auto-build trickery for OpenEXR to build a static library, and it swallows the openexr build output rather than clutter the logs with all the dependency builds (other than reporting if they actually fail). So, er, ... so far it's defying my attempts to discover if I'm actually hitting that warning.
But I can buy the logic. I don't think there's harm in using the quotes. Shall I accept those suggestions and let's try it?
| if (NOT @BUILD_SHARED_LIBS@) | ||
| openexr_find_pkgconfig_dependency(libzstd libzstd) | ||
| endif() | ||
| elseif (@zstd_FOUND@) |
There was a problem hiding this comment.
| elseif (@zstd_FOUND@) | |
| elseif ("@zstd_FOUND@") |
same here.
There was a problem hiding this comment.
Definitely beyond my obvious knowledge of this corner of cmake. I don't know why quoting this would be helpful. I defer to the better judgment of you and/or your robot.
cary-ilm
left a comment
cary-ilm
left a comment
There was a problem hiding this comment.
This is beyond my cmake/pkgconfig expertise but it looks fine, especially if it solves your problem.
The CI includes a couple tests to validate pkgconfig, but they don't catch this case, where it's the pkgconfig on the dependent libraries that's in question. Ideally, I'd like to add a case to to the CI to validate this case, but I also intend a massive overhaul of the CI at some point soon, since it's become quite complicated. So let's file an issue for that and deal with it later.
|
And yes, all CI-old workflows are failing now, unrelated to your change, something changed on the GitHub side. I propose to just drop those entirely since they're old and fragile. |
|
Well, my problem is solved by a separate patch I made on the OIIO side (I have to deal with the broken OpenEXR versions that already exist, after all). But other downstream projects could run into the same problem, and this is meant to solve it at the source. I don't really use pkgconfig myself, so this is all a little fuzzy to me. |
When libdeflate, OpenJPH, or zstd is found via pkg-config rather than its CMake config, a static OpenEXRCore links to it through a PkgConfig:: imported target. OpenEXRConfig.cmake never recreated those targets, so downstream find_package(OpenEXR) failed with "The link interface of target OpenEXR::OpenEXRCore contains PkgConfig::libzstd but the target was not found". For libdeflate, the config also checked libdeflate_FOUND, which the pkg-config path does not set; for OpenJPH it called find_dependency(openjph), which requires a CMake config. For static builds, recreate the PkgConfig:: targets with pkg_check_modules; otherwise keep the existing find_dependency calls. Assisted-by: Claude Code / Claude Opus 5.5 Signed-off-by: Larry Gritz <lg@larrygritz.com> Co-authored-by: Cary Phillips <seabeepea@gmail.com>
When libdeflate, OpenJPH, or zstd is found via pkg-config rather than its CMake config, a static OpenEXRCore links to it through a PkgConfig:: imported target. OpenEXRConfig.cmake never recreated those targets, so downstream find_package(OpenEXR) failed with "The link interface of target OpenEXR::OpenEXRCore contains PkgConfig::libzstd but the target was not found". For libdeflate, the config also checked libdeflate_FOUND, which the pkg-config path does not set; for OpenJPH it called find_dependency(openjph), which requires a CMake config.
For static builds, recreate the PkgConfig:: targets with pkg_check_modules; otherwise keep the existing find_dependency calls.
Assisted-by: Claude Code / Claude Opus 5.5