Skip to content
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

Merged

Conversation

brechtvl
Copy link
Contributor

@brechtvl brechtvl commented Jan 10, 2024

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

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]>
@brechtvl
Copy link
Contributor Author

brechtvl commented Jan 10, 2024

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.

@brechtvl
Copy link
Contributor Author

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?

@meshula
Copy link
Contributor

meshula commented Jan 11, 2024

This change looks good to me.

Removing the pkgconfig detection would mean supporting only libdeflate 1.15+,

Linux distro maintainers are keen to use their own libdeflate. Debian stable is 1.14, so I think that impacts the choice.

cmake/OpenEXRSetup.cmake Outdated Show resolved Hide resolved
cmake/OpenEXRSetup.cmake Outdated Show resolved Hide resolved
@brechtvl
Copy link
Contributor Author

Thanks, that simplifies things. I've updated the PR description.

I think this PR also replaces #1572.

@cary-ilm
Copy link
Member

Thanks for the cleanup. @mandree, can you confirm this addresses #1571, so we can discard #1572?

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM

@cary-ilm cary-ilm merged commit cb94fe1 into AcademySoftwareFoundation:main Jan 22, 2024
29 checks passed
cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request Feb 13, 2024
* 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]>
cary-ilm pushed a commit that referenced this pull request Feb 16, 2024
* 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]>
@mandree
Copy link
Contributor

mandree commented Mar 30, 2024

Sorry, got distracted. v3.2.4 built fine without my local patch that gave rise to #1571. Thanks.

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.

Use CMake's find_package to include libdeflate
5 participants