Bug 223470

Summary: ANGLE is missing the explicit context headers
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebGLAssignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dino, ews-watchlist, graouts, kbr, kkinnunen, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 198948, 221664, 223250    
Attachments:
Description Flags
Patch
none
Patch none

Description Kimmo Kinnunen 2021-03-18 12:44:50 PDT
ANGLE is missing the explicit context headers
Comment 1 Kimmo Kinnunen 2021-03-18 12:49:06 PDT
Created attachment 423637 [details]
Patch
Comment 2 EWS Watchlist 2021-03-18 12:50:49 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Alexey Proskuryakov 2021-03-18 13:23:10 PDT
Comment on attachment 423637 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423637&action=review

> Source/ThirdParty/ANGLE/ChangeLog:9
> +        Add the autogenerated .inc files for explicit context
> +        API into the ANGLE public headers.

Do these new files need to be added to outputs in CMake and Xcode projects, so that the build system knows where they come from? Or are they just produced and never used by us, making it irrelevant?
Comment 4 Alexey Proskuryakov 2021-03-18 13:25:56 PDT
Comment on attachment 423637 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423637&action=review

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:1014
> +		7B0A2A632603D4040040DCEB /* gl32ext_explicit_context_autogen.inc in Headers */ = {isa = PBXBuildFile; fileRef = 7B0A2A562603D3E00040DCEB /* gl32ext_explicit_context_autogen.inc */; settings = {ATTRIBUTES = (Public, ); }; };

Where will these go? Public headers for ANGLE seem strange, and looks like existing ones go into /usr/local/include (and don't get included into public SDK).
Comment 5 Kimmo Kinnunen 2021-03-19 00:48:37 PDT
(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 423637 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423637&action=review
> 
> > Source/ThirdParty/ANGLE/ChangeLog:9
> > +        Add the autogenerated .inc files for explicit context
> > +        API into the ANGLE public headers.
> 
> Do these new files need to be added to outputs in CMake and Xcode projects,
> so that the build system knows where they come from? 

The files already exist in CMake projects.

The files are added in this patch to behave exactly any other already handled ANGLE public header file, such as, for example GLES2/gl2ext_angle.h (if you want to grep for gl2ext_angle.h to see how it is handled. 

That is to say, in Xcode currently we don't mark them in such a way that that it'd make a particular difference to the project outputs.

> Or are they just produced and never used by us, making it irrelevant?

They are in the repository, produced by the autogeneration during dev time in upstream.

They are currently not part of the Xcode project, requiring a workaround removed in this patch, thus not entirely irrelevant.

They are going to be used by us, thus not entirely irrelevant.


> Where will these go? Public headers for ANGLE seem strange, and looks like existing ones go into /usr/local/include (and don't get included into public SDK).

These added go wherever other ANGLE headers go, as they're handled as normal ANGLE public headers.

E.g.  (BUILT_PRODUCTS_DIR or DSTROOT)/usr/local/include/ANGLE/ <- flat file hierarchy.

If you are saying "public SDK" in terms of "SDK that goes to WebKit clients", then yes, ANGLE is not part of that SDK. If you mean "WebCore private framework SDK", then yes, they're not used there either, but in some cases that is a slight problem that we could fix if that'd be desired.


So currently we have a process: "something is done to ANGLE headers X,Y and Z and they're put to somewhere".

This patch tries to only add the .inc files to behave same as the X,Y,Z, without addressing "something" and "somewhere".

If you have suggestions on how to mark the Xcode project so that subsequent phases understand where the files come from, please suggest and I can file a bug.

If you have suggestions on where the ANGLE headers should go instead of /usr/local/include, please suggest and I can file a bug.
Comment 6 Kimmo Kinnunen 2021-03-19 01:01:44 PDT
wrt the /usr/local/include:

WTF is a library, not a framework. WTF has "library private" headers.
WTF headers are copied to ${DSTROOT}/${PRIVATE_HEADERS_FOLDER_PATH}

libwebrtc is a library, not a framework. libwebrtc has "library private" headers.
libwebrtc headers are copied to ${DSTROOT}${INSTALL_PATH_PREFIX%/}/${PRIVATE_HEADERS_FOLDER_PATH}

ANGLE is a library, not a framework. ANGLE has "library public" headers.
ANGLE headers are copied to ${DSTROOT}/${PUBLIC_HEADERS_FOLDER_PATH}

So I don't think there's any strangeness -- ANGLE is handled consistently?

(there's non-consistency in some using INSTALL_PATH_PREFIX and others not, but probably not significant to this discussion..)
Comment 7 Kimmo Kinnunen 2021-03-19 03:42:41 PDT
Created attachment 423714 [details]
Patch
Comment 8 Alexey Proskuryakov 2021-03-19 10:02:09 PDT
Comment on attachment 423714 [details]
Patch

> If you have suggestions on how to mark the Xcode project so that subsequent phases understand where the files come from, please suggest and I can file a bug.

Not as much as a suggestion, no. We'll need a way to clean up all script phases in Xcode projects to fully define inputs and outputs, and I don't think that tracking it for ANGLE specifically would be useful at this point.

As we track these the same as exiting .h fines, I don't have a concern.

> If you have suggestions on where the ANGLE headers should go instead of /usr/local/include, please suggest and I can file a bug.

/usr/local/include seems right, I just don't understand why PUBLIC_HEADERS_FOLDER_PATH points to that (shouldn't it be /usr/include?) Again, I have no concerns about the patch now that you explained that this is consistent with existing headers.
Comment 10 Kimmo Kinnunen 2021-03-23 03:20:19 PDT
Comment on attachment 423714 [details]
Patch

As far as I can verify, .inc files don't cause problems.
Comment 11 EWS 2021-03-23 03:29:40 PDT
Committed r274856: <https://commits.webkit.org/r274856>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423714 [details].
Comment 12 Radar WebKit Bug Importer 2021-03-23 03:30:23 PDT
<rdar://problem/75731930>