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

[openexr] fixup pkgconfig #18123

Merged

Conversation

mcmtroffaes
Copy link
Contributor

  • What does your PR fix?

    The generated .pc file for openexr contains an absolute path. This PR fixes that.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    No changes.

  • Does your PR follow the maintainer guide?

    Yes, to the best of my knowledge.

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes.

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label May 27, 2021
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label May 31, 2021
@dg0yt
Copy link
Contributor

dg0yt commented May 31, 2021

Unfortunately, it is no enough. For debug, libraries are build with a _d suffix:

$ ls packages/openexr_x64-linux-dynamic/debug/lib
libHalf-2_5_d.so            libIlmImf-2_5_d.so.24.0.0
libHalf-2_5_d.so.24         libIlmImfUtil-2_5_d.so
libHalf-2_5_d.so.24.0.0     libIlmImfUtil-2_5_d.so.24
libIex-2_5_d.so             libIlmImfUtil-2_5_d.so.24.0.0
libIex-2_5_d.so.24          libIlmThread-2_5_d.so
libIex-2_5_d.so.24.0.0      libIlmThread-2_5_d.so.24
libIexMath-2_5_d.so         libIlmThread-2_5_d.so.24.0.0
libIexMath-2_5_d.so.24      libImath-2_5_d.so
libIexMath-2_5_d.so.24.0.0  libImath-2_5_d.so.24
libIlmImf-2_5_d.so          libImath-2_5_d.so.24.0.0
libIlmImf-2_5_d.so.24       pkgconfig

Fortunately, given the libsuffix=-2_5 line, it is probably easy to fix by substituting =-2_5 with =-2_5_d in files matching glob ${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig/*.pc.

@JonLiu1993 JonLiu1993 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels May 31, 2021
@mcmtroffaes
Copy link
Contributor Author

Many thanks for catching @dg0yt, I appreciate you looking over this. Any objection to instead simply setting CMAKE_DEBUG_POSTFIX to the empty string as in the commit I just added?

@mcmtroffaes
Copy link
Contributor Author

(Note https://github.com/AcademySoftwareFoundation/openexr/blob/master/cmake/OpenEXRSetup.cmake#L72 so it really needs to be explicitly set to the empty string.)

@dg0yt
Copy link
Contributor

dg0yt commented May 31, 2021

Any objection to instead simply setting CMAKE_DEBUG_POSTFIX to the empty string as in the commit I just added?

IMO it is fine, as long as library names match exported cmake and pkgconfig configuration. It seems hardly possibly to use these libs correctly without exported configuration. (I will know more when I add openexr support to port gdal.)

However, vcpkg policy is to not rename binaries outside the names given by upstream. Decision left to approvers.

@mcmtroffaes
Copy link
Contributor Author

Thanks, and sure, I asked explicitly because of that policy. Using an upstream configurable setting to fix issues with the upstream exported .pc configuration would seem within reasonable boundaries to me and guarantees all configurations (cmake, pkgconfig) remain in sync; it's not like renaming the libraries post-installation.

But if it's not ok, I'd be happy to try to fix the openexr .pc generation with a patch that can be upstreamed. However, I really rather go for the currently proposed and much simpler solution, not requiring any special case regexp-ing of the .pc files, and not requiring any upstreamable patches.

@Neumann-A
Copy link
Contributor

However, vcpkg policy is to not rename binaries outside the names given by upstream. Decision left to approvers.

Policy is policy. You don't know if a downstream port has a custom Find<Whatever> which depends on the upstream naming schema.

@mcmtroffaes
Copy link
Contributor Author

Sure. I've posted a patch upstream.

@mcmtroffaes
Copy link
Contributor Author

There's some non-trivial discussion upstream on how best to fix this. I'll put this back on draft until the outcome of that discussion is clear, so I can add the final upstream patch.

@mcmtroffaes mcmtroffaes marked this pull request as draft June 2, 2021 07:24
@JackBoosY
Copy link
Contributor

Please ping me if this PR is ready for review.

@mcmtroffaes mcmtroffaes marked this pull request as ready for review June 11, 2021 07:16
@mcmtroffaes
Copy link
Contributor Author

@JackBoosY Thanks, your message was perfectly timed: upstream just merged the patch! It's ready now for review.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jun 11, 2021
@strega-nil-ms
Copy link
Contributor

LGTM, thanks @mcmtroffaes :)

@strega-nil-ms strega-nil-ms merged commit 1d81331 into microsoft:master Jun 11, 2021
@mcmtroffaes mcmtroffaes deleted the feature/openexr-fixup-pkgconfig branch June 11, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants