After the recent update of ANGLE, enabling USE_ANGLE_WEBGL on Linux platforms results in the build failing with "undefined reference to 'angle::DrmFourCCFormatToGLInternalFormat(int, bool*)'" because the Source/ThirdParty/ANGLE/src/common/linux/ directory doesn't get built.
Created attachment 446992 [details] Patch
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment on attachment 446992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446992&action=review LGTM, with one noted change. > Source/ThirdParty/ANGLE/GL.cmake:237 > +if (is_android OR is_linux OR is_chromeos) > + list(APPEND _gl_backend_sources > + "src/common/linux/dma_buf_utils.cpp" > + "src/common/linux/dma_buf_utils.h" > + ) > +endif() Why not add these to the existing block at line 127?
Created attachment 447047 [details] Patch
Created attachment 447048 [details] Patch
Comment on attachment 447048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447048&action=review > Source/ThirdParty/ANGLE/GL.cmake:156 > + "src/common/linux/dma_buf_utils.h" I think you shouldn't edit GL.cmake because it is genereated by gni-to-cmake.py.
(In reply to Fujii Hironori from comment #6) > Comment on attachment 447048 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447048&action=review > > > Source/ThirdParty/ANGLE/GL.cmake:156 > > + "src/common/linux/dma_buf_utils.h" > > I think you shouldn't edit GL.cmake because it is genereated by > gni-to-cmake.py. It's hand-edited after the generation, I think given the coarseness of this tool and that this file is already hand-edited (re-running the listed command at the top of the file generates a different file), it's reasonable to add this manually. The current script isn't sophisticated enough to interpret the build file in src/common/linux (in fact, it currently just strips out that definition entirely, so using the script generates an empty file).
I guess what I had in the back of my mind when I added the stuff in a separate block is that the other blocks were auto-generated, and this one is manually added and thus easier to merge with the auto-generated stuff. Maybe I should put a comment in to that effect as well? I honestly am not that familiar with the process of importing ANGLE and converting the build scripts so I am not sure what the right approach is and I am glad we are having this discussion.
(In reply to Arcady Goldmints-Orlov from comment #8) > I guess what I had in the back of my mind when I added the stuff in a > separate block is that the other blocks were auto-generated, and this one is > manually added and thus easier to merge with the auto-generated stuff. Maybe > I should put a comment in to that effect as well? I honestly am not that > familiar with the process of importing ANGLE and converting the build > scripts so I am not sure what the right approach is and I am glad we are > having this discussion. That sounds reasonable to me - I guess if we really don't want to touch the auto-generated files, it could be a separate file or included in PlatformGTK. These build files and the process of generating them doesn't really seem to be so involved that we should be worrying too much about this, imho.
Comment on attachment 447048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447048&action=review >>> Source/ThirdParty/ANGLE/GL.cmake:156 >>> + "src/common/linux/dma_buf_utils.h" >> >> I think you shouldn't edit GL.cmake because it is genereated by gni-to-cmake.py. > > It's hand-edited after the generation, I think given the coarseness of this tool and that this file is already hand-edited (re-running the listed command at the top of the file generates a different file), it's reasonable to add this manually. > > The current script isn't sophisticated enough to interpret the build file in src/common/linux (in fact, it currently just strips out that definition entirely, so using the script generates an empty file). I looked at those dma_buf_util files and in the GN its actually its own library which is probably why it isn't being picked up. I didn't write that gni-to-cmake.py script and I don't think it ran successfully on Windows if I'm remembering correctly. Ideally this should be generated in the GLESv2.cmake file NOT GL.cmake. You'll notice that the backend CMake files only append sources in `src/libANGLE/renderer/${name}`. I'm okay with this change happening in GLESv2.cmake with a big ol FIXME with an actual bug associated with it so next time there's a roll someone hopefully notices it. I'm also okay with just adding it in the PlatformGTK.cmake in the root but I do think there should be a bug about it either way.
Comment on attachment 447048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447048&action=review >>>> Source/ThirdParty/ANGLE/GL.cmake:156 >>>> + "src/common/linux/dma_buf_utils.h" >>> >>> I think you shouldn't edit GL.cmake because it is genereated by gni-to-cmake.py. >> >> It's hand-edited after the generation, I think given the coarseness of this tool and that this file is already hand-edited (re-running the listed command at the top of the file generates a different file), it's reasonable to add this manually. >> >> The current script isn't sophisticated enough to interpret the build file in src/common/linux (in fact, it currently just strips out that definition entirely, so using the script generates an empty file). > > I looked at those dma_buf_util files and in the GN its actually its own library which is probably why it isn't being picked up. I didn't write that gni-to-cmake.py script and I don't think it ran successfully on Windows if I'm remembering correctly. > > Ideally this should be generated in the GLESv2.cmake file NOT GL.cmake. You'll notice that the backend CMake files only append sources in `src/libANGLE/renderer/${name}`. > > I'm okay with this change happening in GLESv2.cmake with a big ol FIXME with an actual bug associated with it so next time there's a roll someone hopefully notices it. I'm also okay with just adding it in the PlatformGTK.cmake in the root but I do think there should be a bug about it either way. It's not a library. It's a source set that is used by GL and Valkan backends. I think we should add linux.cmake, and include it from PlatformGTK.cmake and PlatformWPE.cmake, or from CMakeLists.txt.
Comment on attachment 447048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447048&action=review >>>>> Source/ThirdParty/ANGLE/GL.cmake:156 >>>>> + "src/common/linux/dma_buf_utils.h" >>>> >>>> I think you shouldn't edit GL.cmake because it is genereated by gni-to-cmake.py. >>> >>> It's hand-edited after the generation, I think given the coarseness of this tool and that this file is already hand-edited (re-running the listed command at the top of the file generates a different file), it's reasonable to add this manually. >>> >>> The current script isn't sophisticated enough to interpret the build file in src/common/linux (in fact, it currently just strips out that definition entirely, so using the script generates an empty file). >> >> I looked at those dma_buf_util files and in the GN its actually its own library which is probably why it isn't being picked up. I didn't write that gni-to-cmake.py script and I don't think it ran successfully on Windows if I'm remembering correctly. >> >> Ideally this should be generated in the GLESv2.cmake file NOT GL.cmake. You'll notice that the backend CMake files only append sources in `src/libANGLE/renderer/${name}`. >> >> I'm okay with this change happening in GLESv2.cmake with a big ol FIXME with an actual bug associated with it so next time there's a roll someone hopefully notices it. I'm also okay with just adding it in the PlatformGTK.cmake in the root but I do think there should be a bug about it either way. > > It's not a library. It's a source set that is used by GL and Valkan backends. > I think we should add linux.cmake, and include it from PlatformGTK.cmake and PlatformWPE.cmake, or from CMakeLists.txt. Ok with that route I guess we would just list the sources in a variable and then the GTK/WPE folks can just append those values to it.
Created attachment 447628 [details] Patch
Comment on attachment 447628 [details] Patch Looks fine! r+.
Committed r287312 (245464@main): <https://commits.webkit.org/245464@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447628 [details].
<rdar://problem/86765194>