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

[egl] Revise wrapper and pkgconfig files for windows #28174

Merged
merged 6 commits into from
Dec 14, 2022

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Dec 5, 2022

  • What does your PR fix?

    Fixes filename case mismatch for mingw in pc file (assuming case-insensitive filesystem for Windows hosts, as usual).
    Simplifies the wrapper based on unofficial angle config.
    Fixes unintented side effect on CMAKE_REQUIRED_<variable> (cf. [baseline][osg] Prefer GLVND libraries #28089 (comment)).

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

    unchanged

  • Does your PR follow the maintainer guide?

    yes

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

    yes

@dg0yt dg0yt marked this pull request as draft December 5, 2022 07:20
Comment on lines +10 to +18
find_package(unofficial-angle CONFIG)
if(TARGET unofficial::angle::libEGL)
set(EGL_LIBRARY unofficial::angle::libEGL)
if(NOT TARGET EGL::EGL)
add_library(EGL::EGL INTERFACE IMPORTED)
set_target_properties(EGL::EGL PROPERTIES
INTERFACE_LINK_LIBRARIES unofficial::angle::libEGL
)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't depend on unofficial targets. I expect it to go away as soon as angle gets changed to actually use the upstream buildsystem and having unofficial targets be used downstream is always a migration blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This dependency is internal (within the wrapper). I consider this less harmful than external usage of unofficial packages and than redundant information about link libraries. We can always silently provide the unofficial targets for a transition period, purely forwarding to the official targets.

I'm not sure if I know enough about angle. AFAIU its native build system is gn. Where do I find the official targets it is going to provide? (I would love to know the same about skia.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is not using cmake it won't provide any official targets and you have to use find_library to setup targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is why we do use unofficial targets, internally: Encode usage requirements in the port where they come from, not where they are used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Encode usage requirements in the port where they come from, not where they are used.

Then you have to apply that logic also to the pc file.....

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Dec 5, 2022
@dg0yt dg0yt marked this pull request as ready for review December 5, 2022 11:28
@Neumann-A
Copy link
Contributor

Keep in mind that EGL could also come from mesa. I just blessed ANGLE because EGL via DX seemed more appropriate/useable than EGL via software.

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Dec 7, 2022
@dan-shaw dan-shaw merged commit 5ed17f6 into microsoft:master Dec 14, 2022
@dg0yt dg0yt deleted the egl branch December 14, 2022 03:59
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.

4 participants