-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support cmake config for libdeflate #1613
Support cmake config for libdeflate #1613
Conversation
When not using the internal deflate, the only option was pkgconfig which is not well supported on Windows. This adds support for using the libdeflate cmake config files when available. This change also affects the generated OpenEXR cmake config files. For a static OpenEXR library, INTERFACE_LINK_LIBRARIES now appropriately includes libdeflate along with the existing dependencies like Imath. It also turns the internal libdeflate into a target for consistency. Resolves AcademySoftwareFoundation#1588 Signed-off-by: Brecht Van Lommel <[email protected]>
d464f8a
to
bc3153d
Compare
This ended up being quite a bit more complicated than I had hoped. I tried to test all combinations of cmake config and pkgconfig, static and shared OpenEXR, static and shared libdeflate. From the CI it seems I broke the internal libdeflate case in some way. The implementation would be simpler if we could only support detecting libdeflate through cmake config files and drop the pkgconfig stuff. But I'm not sure if that would be considered too breaking. |
Signed-off-by: Brecht Van Lommel <[email protected]>
I backed out of making any changes to the internal libdeflate code for now, which fixed the CI. Removing the pkgconfig detection would mean supporting only libdeflate 1.15+, released December 5, 2022. It's not that old, maybe needs to be kept as a fallback for now? |
This change looks good to me.
Linux distro maintainers are keen to use their own libdeflate. Debian stable is 1.14, so I think that impacts the choice. |
Signed-off-by: Brecht Van Lommel <[email protected]>
Thanks, that simplifies things. I've updated the PR description. I think this PR also replaces #1572. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Support cmake config and shared library for libdeflate When not using the internal deflate, the only option was pkgconfig which is not well supported on Windows. This adds support for using the libdeflate cmake config files when available. This change also affects the generated OpenEXR cmake config files. For a static OpenEXR library, INTERFACE_LINK_LIBRARIES now appropriately includes libdeflate along with the existing dependencies like Imath. It also turns the internal libdeflate into a target for consistency. Resolves AcademySoftwareFoundation#1588 Signed-off-by: Brecht Van Lommel <[email protected]> * Back out of adding a target for internal libdeflate Signed-off-by: Brecht Van Lommel <[email protected]> * Use Requires.private for generated pkgconfig file Signed-off-by: Brecht Van Lommel <[email protected]> --------- Signed-off-by: Brecht Van Lommel <[email protected]>
* Support cmake config and shared library for libdeflate When not using the internal deflate, the only option was pkgconfig which is not well supported on Windows. This adds support for using the libdeflate cmake config files when available. This change also affects the generated OpenEXR cmake config files. For a static OpenEXR library, INTERFACE_LINK_LIBRARIES now appropriately includes libdeflate along with the existing dependencies like Imath. It also turns the internal libdeflate into a target for consistency. Resolves #1588 Signed-off-by: Brecht Van Lommel <[email protected]> * Back out of adding a target for internal libdeflate Signed-off-by: Brecht Van Lommel <[email protected]> * Use Requires.private for generated pkgconfig file Signed-off-by: Brecht Van Lommel <[email protected]> --------- Signed-off-by: Brecht Van Lommel <[email protected]>
Sorry, got distracted. v3.2.4 built fine without my local patch that gave rise to #1571. Thanks. |
When not using the internal deflate, the only option was pkgconfig which is not well supported on Windows. This adds support for using cmake config files to detect libdeflate. It also changes the pkgconfig case to use the target instead of the library, for consistency.
OpenEXR cmake config files now appropriately include libdeflate in INTERFACE_LINK_LIBRARIES, when building a static OpenEXR library.
The OpenEXR pkgconfig file now uses Requires.private for libdeflate instead of linking flags.
Resolves #1588