Bug 233996 - [MacCatalyst] Update header search paths for ANGLE Catalyst
Summary: [MacCatalyst] Update header search paths for ANGLE Catalyst
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-08 02:11 PST by Myles C. Maxfield
Modified: 2021-12-08 18:37 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2021-12-08 02:16 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (4.48 KB, patch)
2021-12-08 14:42 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2021-12-08 16:03 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2021-12-08 16:21 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (4.51 KB, patch)
2021-12-08 16:46 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (4.39 KB, patch)
2021-12-08 17:05 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-12-08 02:11:36 PST
[MacCatalyst] ANGLE is putting its public headers in the wrong place
Comment 1 Myles C. Maxfield 2021-12-08 02:16:27 PST
Created attachment 446340 [details]
Patch
Comment 2 EWS Watchlist 2021-12-08 02:17:33 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Alexey Proskuryakov 2021-12-08 06:31:28 PST
Comment on attachment 446340 [details]
Patch

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

> Source/ThirdParty/ANGLE/ChangeLog:3
> +        [MacCatalyst] ANGLE is putting its public headers in the wrong place

It looks like both native and macCatalyst builds will be producing the same SDK header paths with this patch. Which won’t work, projects shouldn’t be overwriting files that other projects already created. 

To be clear, I’m talking about final SDK paths here, not ones that you posted in the ChangeLog below. 

Am I missing something?
Comment 4 Alexey Proskuryakov 2021-12-08 08:25:40 PST
Looking at WTF, I see that production builds put iOSMac SDK content in /System/iOSSupport/usr/local, so there is no overlap.
Comment 5 Kyle Piddington 2021-12-08 14:42:33 PST
Created attachment 446425 [details]
Patch
Comment 6 Dean Jackson 2021-12-08 14:50:32 PST
Comment on attachment 446425 [details]
Patch

I'm surprised we didn't have to update WebCore's BaseTarget.xcconfig too
Comment 7 Myles C. Maxfield 2021-12-08 15:51:33 PST Comment hidden (obsolete)
Comment 8 Alexey Proskuryakov 2021-12-08 15:53:43 PST
Comment on attachment 446425 [details]
Patch

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

> Source/WebKit/Configurations/BaseTarget.xcconfig:51
> +ANGLE_HEADER_ALTERNATE_SEARCH_PATHS = $(ANGLE_HEADER_ALTERNATE_SEARCH_PATHS_$(SDK_VARIANT));
> +ANGLE_HEADER_ALTERNATE_SEARCH_PATHS_iosmac = $(BUILT_PRODUCTS_DIR)$(WK_ALTERNATE_FRAMEWORKS_DIR)/usr/local/include

Do we need the same for WTF?

> Source/WebKit/Configurations/BaseTarget.xcconfig:53
> +HEADER_SEARCH_PATHS = $(BUILT_PRODUCTS_DIR)/usr/local/include "$(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders" $(BUILT_PRODUCTS_DIR)/DerivedSources/WebKit "$(BUILT_PRODUCTS_DIR)/usr/local/include/pal/graphics/WebGPU" $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS) $(ANGLE_HEADER_ALTERNATE_SEARCH_PATHS) $(SRCROOT) $(HEADER_SEARCH_PATHS);

Should ANGLE_HEADER_ALTERNATE_SEARCH_PATHS come first, to override the versions in /usr/local?
Comment 9 Kyle Piddington 2021-12-08 15:54:01 PST
(In reply to Dean Jackson from comment #6)
> Comment on attachment 446425 [details]
> Patch
> 
> I'm surprised we didn't have to update WebCore's BaseTarget.xcconfig too

WebCore already has this (Configurations/Webcore)

ANGLE_HEADER_SEARCH_PATHS = $(BUILT_PRODUCTS_DIR)$(WK_ALTERNATE_FRAMEWORKS_DIR)/usr/local/include/ $(SDKROOT)$(WK_ALTERNATE_WEBKIT_SDK_PATH)/usr/local/include/;
Comment 10 Kyle Piddington 2021-12-08 15:55:34 PST
(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 446425 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446425&action=review
> 
> > Source/WebKit/Configurations/BaseTarget.xcconfig:51
> > +ANGLE_HEADER_ALTERNATE_SEARCH_PATHS = $(ANGLE_HEADER_ALTERNATE_SEARCH_PATHS_$(SDK_VARIANT));
> > +ANGLE_HEADER_ALTERNATE_SEARCH_PATHS_iosmac = $(BUILT_PRODUCTS_DIR)$(WK_ALTERNATE_FRAMEWORKS_DIR)/usr/local/include
> 
> Do we need the same for WTF?
> 
> > Source/WebKit/Configurations/BaseTarget.xcconfig:53
> > +HEADER_SEARCH_PATHS = $(BUILT_PRODUCTS_DIR)/usr/local/include "$(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders" $(BUILT_PRODUCTS_DIR)/DerivedSources/WebKit "$(BUILT_PRODUCTS_DIR)/usr/local/include/pal/graphics/WebGPU" $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS) $(ANGLE_HEADER_ALTERNATE_SEARCH_PATHS) $(SRCROOT) $(HEADER_SEARCH_PATHS);
> 
> Should ANGLE_HEADER_ALTERNATE_SEARCH_PATHS come first, to override the
> versions in /usr/local?

Good call, yes.
Comment 11 Alexey Proskuryakov 2021-12-08 15:55:39 PST
Comment on attachment 446425 [details]
Patch

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

>> Source/WebKit/Configurations/BaseTarget.xcconfig:51
>> +ANGLE_HEADER_ALTERNATE_SEARCH_PATHS_iosmac = $(BUILT_PRODUCTS_DIR)$(WK_ALTERNATE_FRAMEWORKS_DIR)/usr/local/include
> 
> Do we need the same for WTF?

Given that we effectively apply to to everything, probably shouldn't have ANGLE in the name anyway, just ALTERNATE_HEADER_SEARCH_PATHS.
Comment 12 Kyle Piddington 2021-12-08 16:03:32 PST
Created attachment 446447 [details]
Patch
Comment 13 Alexey Proskuryakov 2021-12-08 16:07:26 PST
Comment on attachment 446447 [details]
Patch

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

> Source/WebKit/Configurations/BaseTarget.xcconfig:53
> +HEADER_SEARCH_PATHS = $(ALTERNATE_HEADER_SEARCH_PATHS) $(BUILT_PRODUCTS_DIR)/usr/local/include "$(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders" $(BUILT_PRODUCTS_DIR)/DerivedSources/WebKit "$(BUILT_PRODUCTS_DIR)/usr/local/include/pal/graphics/WebGPU" $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS)  $(SRCROOT) $(HEADER_SEARCH_PATHS);

Please don't add two spaces on this line.

I think that I was not precise enough. ALTERNATE_HEADER_SEARCH_PATHS should probably come after SRCROOT still, but before HEADER_SEARCH_PATHS. The current order seems very inconsistent, with it being the very first.

> Tools/TestWebKitAPI/Configurations/Base.xcconfig:40
> +HEADER_SEARCH_PATHS = $(ALTERNATE_HEADER_SEARCH_PATHS) $(BUILT_PRODUCTS_DIR)/usr/local/include "$(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders" $(BUILT_PRODUCTS_DIR)/DerivedSources/WebKit "$(BUILT_PRODUCTS_DIR)/usr/local/include/pal/graphics/WebGPU" $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS)  $(SRCROOT) $(HEADER_SEARCH_PATHS);

Ditto.
Comment 14 Kyle Piddington 2021-12-08 16:21:02 PST
Created attachment 446452 [details]
Patch
Comment 15 Alexey Proskuryakov 2021-12-08 16:24:22 PST
Comment on attachment 446452 [details]
Patch

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

> Source/WebKit/Configurations/BaseTarget.xcconfig:53
> +HEADER_SEARCH_PATHS = $(BUILT_PRODUCTS_DIR)/usr/local/include "$(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders" $(BUILT_PRODUCTS_DIR)/DerivedSources/WebKit "$(BUILT_PRODUCTS_DIR)/usr/local/include/pal/graphics/WebGPU" $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS) $(SRCROOT) $(ALTERNATE_HEADER_SEARCH_PATHS) $(HEADER_SEARCH_PATHS);

I'm sorry for my carelessness in review.

ALTERNATE_HEADER_SEARCH_PATHS is actually parallel to $(BUILT_PRODUCTS_DIR)/usr/local/include, not to the original value of $(HEADER_SEARCH_PATHS), so it should be first in fact?
Comment 16 Kyle Piddington 2021-12-08 16:46:47 PST
Created attachment 446457 [details]
Patch
Comment 17 Kyle Piddington 2021-12-08 17:05:25 PST
Created attachment 446461 [details]
Patch
Comment 18 Alexey Proskuryakov 2021-12-08 17:23:51 PST
Comment on attachment 446461 [details]
Patch

Looks good to me! I'm going to clear your r? flag, because otherwise commit queue would fail - you already have Dean as the reviewer in ChangeLogs.
Comment 19 EWS 2021-12-08 18:36:26 PST
Committed r286757 (244998@main): <https://commits.webkit.org/244998@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446461 [details].
Comment 20 Radar WebKit Bug Importer 2021-12-08 18:37:21 PST
<rdar://problem/86247153>