-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[angle] Update to chromium/5414, improve build system (and future updates) #27444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have modified or added at least one vcpkg.json where you should check the license
field.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/angle/vcpkg.json
Valid values for the license field can be found in the documentation
ports/angle/maintainer-notes.md
Outdated
# The ANGLE vcpkg port | ||
|
||
The ANGLE port's buildsystem is based off of [WebKit's approach](https://github.com/WebKit/WebKit/tree/main/Source/ThirdParty/ANGLE) to converting ANGLE's buildsystem to CMake. | ||
|
||
Details: | ||
|
||
- `cmake-buildsystem/CMakeLists.txt` | ||
- This is an augmented version of WebKit's [CMakeLists.txt](https://github.com/WebKit/WebKit/blob/main/Source/ThirdParty/ANGLE/CMakeLists.txt), with vcpkg edits and additions. | ||
- `cmake-buildsystem/*.cmake` | ||
- These are configuration files based on [WebKit's approach](https://github.com/WebKit/WebKit/tree/main/Source/ThirdParty/ANGLE), with some minor vcpkg edits (and renaming `PlatformGTK` to `PlatformLinux`) | ||
- `cmake-buildsystem/generated/*.cmake` | ||
- These are generated from the upstream ANGLE .gni files using WebKit's [gni-to-cmake.py](https://github.com/WebKit/WebKit/blob/main/Source/ThirdParty/ANGLE/gni-to-cmake.py) conversion script (see below) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering that port skia
and crashpad
are already invoking gn
and can be used I don't see why we should still use CMake to build angle
. The main thing somebody has to find out is how to get from vcpkg-get-cmake-vars
to a proper toolchain with the compiler and everything setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And support community triplet quirks, and handle a bunch of chromium/standalone-specific stuff and flags, which the existing (and this revised) CMake build system do (or otherwise bypass having to worry about), etc.
In lieu of spending time trying to add support for another build system (with extremely limited port use), I figured this would be a good improvement over the status quo, based on a widely-used CMake build system for the port.
(And updating a long-outdated port, fixing numerous security issues.)
Longer-term, if someone wants to go through the effort and get a gn
build setup working for windows, osx, linux, mingw, other community triplets, etc - I will be happy to test. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And updating a long-outdated port, fixing numerous security issues.)
The reason it is outdated is because it uses a CMakeLists.txt
instead of the upstream buildsystem. Just saying...
if(TARGET EGL) | ||
set_target_properties(EGL PROPERTIES EXPORT_NAME libEGL) | ||
endif() | ||
if(TARGET GLESv2) | ||
set_target_properties(GLESv2 PROPERTIES EXPORT_NAME libGLESv2) | ||
endif() | ||
set_target_properties(ANGLE PROPERTIES EXPORT_NAME libANGLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(even though this is the wrong section to comment on)
The OUTPUT_NAME
of the libraries are in conflict with glvnd on linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port currently exports libGLESv2.dll
, libEGL.dll
, (etc), that's what the upstream ANGLE setup builds, and that's what other projects are expecting the port to export.
(Example: SDL2)
So there's no change here (unless I'm missing something?), and any such conflict is pre-existing and expected. (ANGLE, by definition, provides an implementation of libGLESv2 and libEGL.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port currently exports libGLESv2.dll, libEGL.dll, (etc), that's what the upstream ANGLE setup builds, and that's what other projects are expecting the port to export.
I am not talking about windows where those name are in conflict with mesa ;) I just accept angle as superior on windows since it redirects to D3D instead of using SWR. That being said you also want a egl.pc
file on windows.
On Linux however you don't want to link the angle implementation. You want to link libglvnd
which will redirect to the correct egl/glx/gles vendor implementation on a per window basis. So you need libEGL_angle.so
and an ICD manifest for it in the correct folder.
I had enough problems with trying to link against it in #26693
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port currently exports libGLESv2.dll, libEGL.dll, (etc), that's what the upstream ANGLE setup builds, and that's what other projects are expecting the port to export.
I am not talking about windows where those name are in conflict with mesa ;) I just accept angle as superior on windows since it redirects to D3D instead of using SWR. That being said you also want a
egl.pc
file on windows.On Linux however you don't want to link the angle implementation. You want to link
libglvnd
which will redirect to the correct egl/glx/gles vendor implementation on a per window basis. So you needlibEGL_angle.so
and an ICD manifest for it in the correct folder.
My understanding is that would not match the upstream build system (at least, it’s a change from the current port), but if we want to diverge then contributions are welcome.
I had enough problems with trying to link against it in #26693
I thought Qt6 no longer included ANGLE?
(Reference: https://doc.qt.io/qt-6/opengl-changes-qt6.html )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought Qt6 no longer included ANGLE?
It doesn't but it still links egl. And if ANGLE is installed with libEGL it will pickup ANGLE from vcpkg instead of using the system version from libglvnd
.
FindEGL
or FindOpenGL
are stupid. They will just search by name.
36773dd
to
d284fa2
Compare
d284fa2
to
3980fe0
Compare
3980fe0
to
dcb2552
Compare
Could someone please trigger a re-run of the x64_linux CI pass? Looks like an unrelated failure:
|
I agree being able to use the upstream build system would be better, but IMO this is an improvement over the status quo so I'm inclined to merge it. However, I want to confirm with other maintainers. |
@ras0219-msft @JavierMatosD @markle11m @vicroms @AugP @valeriaconde @dan-shaw, and I discussed this today. I noted that I am happy with this approach. @ras0219-msft asked that the instructions and scripts (like We are concerned about potential licensing impact of copying this much Angle and WebKit code into vcpkg's repo and would prefer reducing that if possible. For example, running the python script and including a patch file with the edits. Should we just try to use WebKit's fork directly? |
Makes sense to me.
This is partially why I did not include the I can look into fetching certain files from a pinned commit from the WebKit repo, and using those to generate the files under A few files needed to be extensively modified (or largely re-written), so I'll evaluate to see which ones offer the easiest path for reductions.
Two issues that come immediately to mind:
So I'd lean much more towards your first proposal of reducing what needs to be in the vcpkg repo, fetching selected files, running |
@past-due, could you please fix the conflicting files?, then we can merge this PR, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!
PRs must add only one version and must not modify any published versions
When making any changes to a library, the version or port-version in vcpkg.json
or CONTROL
must be modified.
error: checked-in files for angle have changed but the version was not updated
version: chromium_5414
old SHA: 0b94de610273c3ac6a4a8e9aeef0d1176dce1462
new SHA: 4d21d50c6a69b88182c328b4867a27909fa34b01
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***
@JonLiu1993: Done 👍 |
Looks like this now installs the headers to the wrong location? e.g. However, it seems like the intention of an earlier commit was that the angle headers should be copied to: |
angle_commit.h
maintainer-notes.md
Describe the pull request
Update the
angle
port (libANGLE) tochromium/5249
(corresponding to the current Chromium Stable release).(This follows the recommendations in the libANGLE "Choosing an ANGLE branch" guide, matching the current Chromium Stable 106.0.5249.119.)
Also, refactor the build system based on WebKit's CMake build system for libANGLE (including scripts and instructions to ease updating this port in the future).
Which triplets are supported/not supported? Have you updated the CI baseline?
Same triplets as existing
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, waiting to see how CI checks out