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

Fix WebP linking if CMAKE_FIND_PACKAGE_PREFER_CONFIG is ON #3863

Merged
merged 1 commit into from
Jul 13, 2023
Merged

Conversation

bebuch
Copy link
Contributor

@bebuch bebuch commented Jun 2, 2023

Description

I use CMake with CMAKE_FIND_PACKAGE_PREFER_CONFIG. WebP installs its own config files which but doesn't respect CMAKE_DEBUG_POSTFIX in WEBP_LIBRARIES. Linking to the target resolves that issue.

Tests

It would be usefull to add a test pipline where CMake is called with CMAKE_FIND_PACKAGE_PREFER_CONFIG. However this is out of scope to this PR and should be done by a separate patch. Unfortunately, I don't currently have the time to familiarize myself enough to do this myself.

Checklist:

  • I have read the contribution guidelines.
  • (not applicable) If this is more extensive than a small change to existing code, I
    have previously submitted a Contributor License Agreement
    (individual, and if there is any way my
    employers might think my programming belongs to them, then also
    corporate).
  • (not applicable) I have updated the documentation, if applicable.
  • (not applicable) I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

Actually, I think that the right solution is to edit our FindWebP.cmake, which tries to set those targets but gets the capitalization wrong, calling them WebP::WebP and WebP::WebPDemux.

If you fix our FindWebP.cmake to match the naming conventions of webp's own exported cmake configs, then in this spot, you can just replace ${WEP_LIBRARIES} with the targets, without needing the if at all.

What do you think?

@bebuch
Copy link
Contributor Author

bebuch commented Jul 12, 2023

I updated the patch accordingly :-)

@lgritz
Copy link
Collaborator

lgritz commented Jul 12, 2023

@bebuch In the time since you first submitted this, we have switched to accepting all new code under Apache-2.0 license (changing from our historical use of BSD-3-Clause) and also enabled use of DCO sign-off.

Would you mind please acknowledging here (a comment in this PR is sufficient) that you are ok submitting under Apache-2.0?

And also, could you please do the sign-off so it passes the DCO check? The easiest way to do that is:

git commit --amend -s             # the -s will add the sign-off line
git push origin patch-2 --force

…ACKAGE_PREFER_CONFIG is ON

Signed-off-by: Benjamin Buch <[email protected]>
@bebuch
Copy link
Contributor Author

bebuch commented Jul 13, 2023

I'm fine with submitting under Apache-2.0 license.

@lgritz lgritz merged commit a27b55a into AcademySoftwareFoundation:master Jul 13, 2023
22 of 23 checks passed
lgritz pushed a commit to lgritz/OpenImageIO that referenced this pull request Jul 15, 2023
…ademySoftwareFoundation#3863)

Our own FindWebP.cmake was setting up targets that didn't
match the capitalization of the cmake configs that come with WebP itself.

Signed-off-by: Benjamin Buch <[email protected]>
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.

2 participants