Bug 234239 – [GTK] build fails with undefined symbol in ANGLE if USE_ANGLE_WEBGL is enabled.

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Arcady Goldmints-Orlov]

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.

[Arcady Goldmints-Orlov]

Created attachment 446992 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Chris Lord]

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?

[Arcady Goldmints-Orlov]

Created attachment 447047 [details]
Patch

[Arcady Goldmints-Orlov]

Created attachment 447048 [details]
Patch

[Fujii Hironori]

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.

[Chris Lord]

(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).

[Arcady Goldmints-Orlov]

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.

[Chris Lord]

(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.

[Don Olmstead]

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.

[Fujii Hironori]

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.

[Don Olmstead]

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.

[Arcady Goldmints-Orlov]

Created attachment 447628 [details]
Patch

[Kenneth Russell]

Comment on attachment 447628 [details]
Patch

Looks fine! r+.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86765194>