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

[baseline][opengl,mesa] Resolve ambiguities #28160

Merged
merged 11 commits into from
Jan 12, 2023
Merged

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Dec 4, 2022

  • What does your PR fix?

    Fix a CI baseline regression for x64-windows-static-md, port osg.
    Fixes #include <GL/gl.h> is ambiguous on case-insensitve file systems #28087, by no longer copying libs and headers out of the Windows SDK at all (ultimately) / using wrong case and by changes to mesa.

    I fail to see a robust benefit from the copying of SDK headers. AFAIU package opengl's ABI hash already doesn't depend on the Windows SDK, i.e. rebuilds of the port will be triggered independently of changes to the active version of the Windows SDK, picking what they will find at this time. And even just using exported artifacts on another machine should demand using a compatible (i.e. controlled) version of the SDK.

    Somewhat related, this PR moves the opengl32 lib generated by mesa to manual-link, and drops all mesa headers which are available from opengl-registry.

    Fixes [egl] Build failure #28688

    Revise mesa dependencies.
    Installs a proper copyright file for mesa.

    Drafts of these changes were tested and discussed in [baseline][osg] Prefer GLVND libraries #28089.

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

    unchanged, yes

  • 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

github-actions[bot]
github-actions bot previously approved these changes Dec 4, 2022
Comment on lines 141 to 146
file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/lib/manual-link")
file(RENAME "${CURRENT_PACKAGES_DIR}/lib/opengl32${VCPKG_TARGET_IMPORT_LIBRARY_SUFFIX}" "${CURRENT_PACKAGES_DIR}/lib/manual-link/opengl32${VCPKG_TARGET_IMPORT_LIBRARY_SUFFIX}")
if(NOT VCPKG_BUILD_TYPE)
file(MAKE_DIRECTORY "${CURRENT_PACKAGES_DIR}/debug/lib/manual-link")
file(RENAME "${CURRENT_PACKAGES_DIR}/debug/lib/opengl32${VCPKG_TARGET_IMPORT_LIBRARY_SUFFIX}" "${CURRENT_PACKAGES_DIR}/debug/lib/manual-link/opengl32${VCPKG_TARGET_IMPORT_LIBRARY_SUFFIX}")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary. The way this works is just to make the linker happy about the opengl symbols.
Which DLL actually gets loaded depends on load order. the opengl32.dll provided by mesa is a drop in replacement providing full opengl software rendering.

Copy link
Member

Choose a reason for hiding this comment

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

If the symbols exported by this lib are identical to the Windows SDK ones, the user should get the Windows SDK ones (which we require to be present anyway) and the ones in manual-link are pointless (the Windows SDK ones will always be found first).

If the set of symbols differs we need to come up with something else...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot check the Windows SDK files. But AFAIU mesa explicitly claims to provide a drop-in replacement for Windows opengl32.dll, so the import lib cannot be far away, and any bug is theirs. FTR mesa def template is here:
https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/targets/libgl-gdi/opengl32.def.in

@Cheney-W Cheney-W added 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 labels Dec 5, 2022
@mrowrpurr
Copy link

Friendly ping. This is blocking #28126

@dan-shaw dan-shaw added the depends:vcpkg-team This PR depends on the vcpkg team to solve something label Dec 14, 2022
@dan-shaw
Copy link
Contributor

I don't think we can remove the SDK libs from opengl, as it would break msbuild automatic linkage (#99). We may need to skip testing mesa in CI.

@Neumann-A
Copy link
Contributor

We may need to skip testing mesa in CI.

no

as it would break msbuild automatic linkage

If you need opengl in a windows project you should not really use vcpkg auto linking.....

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 16, 2022

Can't msbuild use the Windows SDK without copying the needed import libs or DLLs?
And there is no single port in vcpkg which breaks when they are not present in installed?

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Dec 20, 2022

Some libraries from the Windows SDK are automatically linked by the compiler (kernel32.lib), and some are not (opengl32.lib). vcpkg intends to provide:

  1. A complete description of the direct dependencies of projects
  2. (within a single build graph) Unique ownership for each library and symbol
  3. Automatic availability of headers and linking of needed libraries for MSBuild

With those points:

Can't msbuild use the Windows SDK without copying the needed import libs or DLLs?

This could be possible by implementing an opengl32.lib-specific hack, but does not solve ownership -- see below. The file conflicts are not the root cause of the problem, they are a symptom of the true cause: unclear ownership over opengl32.lib and GL/gl.h.

And there is no single port in vcpkg which breaks when they are not present in installed?

OSS libraries (so far) are either 1) CMake, which needs to explicitly find_library & link anyway or (rarely) 2) MSBuild projects independent of vcpkg, which will directly list opengl32.lib as an additional linker input file. It does not surprise me that we do not (yet) have a port that requires the automatic linkage. We know we have users that use it -- they've filed issues early on in the life of vcpkg that were fixed by this.

We may need to skip testing mesa in CI.

We do not need to skip testing mesa in CI. However, our opengl must route to the unique owner of opengl -- either the Windows SDK, or system packages, or mesa. This is another case of alternative implementations of a common interface.

This means we need to move compilation and deployment of opengl32.lib / .dll from Mesa into an off-by-default feature so that, across the tree, the Windows SDK is the default owner of opengl. We may need to make similar adjustments to the opengl-registry port.

Then, the same as with openssl, blas, or any of the other interfaces that have multiple providers, users can replace the WinSDK default implementation of opengl via and overlay that instead depends on mesa[opengl]. This is the only way to ensure that every port consuming opengl will be correctly sequenced after mesa/opengl-registry and use the deterministic and correct libs and headers.

As an aside: @dg0yt is spot on with the observation that we need to better capture the Windows SDK dependency here.

@Neumann-A
Copy link
Contributor

@ras0219-msft:

It does not surprise me that we do not (yet) have a port that requires the automatic linkage.

That probably will never happen if #26370 gets merged since that will always explicitly link libraries in msbuild. As mentioned countless times the current MSBuild integration is flawed by design. Another approach will be needed in the future to avoid further issues with it.

unclear ownership over opengl32.lib and GL/gl.h.

ownership of opengl32.lib isn't required (more later). Ownership of GL/gl.h should be opengl-registry because it is the correct upstream. Also I want to point out this is only a Windows issue. On Unix everything needs to move through opengl-registry (headers) and system libglvnd (dispatch) and then mesa (implementation) and there is no selection.

This is another case of alternative implementations of a common interface.
This means we need to move compilation and deployment of opengl32.lib / .dll from Mesa into an off-by-default feature so that,

It is another case but it is different from ssl/blas/lapack in that regard that opengl32 can only be dynamically linked and you generally DON'T want to deploy the windows version because that will prohibit your graphics driver from taking over and you will be stuck in an ancient opengl version (1.1) probably not able to run your application. If you want to deploy SW rendering you are going to want to deploy the mesa llvmpipe version.
Again since it is always a dynamic libraries opengl32.lib is just a way to tell the linker: I have these symbols please don't error on me. It really doesn't matter if it is even in the installed tree. It could be generated by a def file without being associated with either the WindowsSDK or Mesa as long as the list of symbols is correct. As such ownership is really not the issue here.
Furthermore opengl functions are always loaded dynamically https://www.khronos.org/opengl/wiki/Load_OpenGL_Functions
by testing the functionality of the driver which makes the real owner of the symbols the driver itself. This makes this case clearly different from the ssl/blas/lapack ports.

As such I am still voting for leaving the WindowsSDK out of vcpkg. (Since the alternative would be to have the whole SDK in vcpkg). If you want to have working MSBuild autolinking with opengl32 just add a #pragma comment(lib, "opengl32") into the GL/gl.h header for MSVC.

@dan-shaw dan-shaw removed the info:reviewed Pull Request changes follow basic guidelines label Dec 22, 2022
@dg0yt dg0yt changed the title [baseline][opengl,mesa] Stop copying from Windows SDK, resolve ambiguities [baseline][opengl,mesa] Resolve ambiguities Dec 29, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 29, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 29, 2022

I reverted the last commit. SDK headers and import lib are installed as before.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 30, 2022

@ras0219-msft @BillyONeal This should be good enough now to move forward from the baseline regression.

@dg0yt dg0yt mentioned this pull request Jan 3, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 5, 2023

Ping @ras0219-msft @BillyONeal.

@dan-shaw dan-shaw added info:reviewed Pull Request changes follow basic guidelines and removed depends:vcpkg-team This PR depends on the vcpkg team to solve something labels Jan 12, 2023
Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

This LGTM as a stopgap solution.

@vicroms vicroms merged commit ce70fb4 into microsoft:master Jan 12, 2023
@dg0yt dg0yt mentioned this pull request Mar 15, 2023
11 tasks
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.

[egl] Build failure #include <GL/gl.h> is ambiguous on case-insensitve file systems
8 participants