Bug 233996 – [MacCatalyst] Update header search paths for ANGLE Catalyst

• Component: ANGLE
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[MacCatalyst] ANGLE is putting its public headers in the wrong place

[Myles C. Maxfield]

Created attachment 446340 [details]
Patch

[EWS Watchlist]

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

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

Looking at WTF, I see that production builds put iOSMac SDK content in /System/iOSSupport/usr/local, so there is no overlap.

[Kyle Piddington]

Created attachment 446425 [details]
Patch

[Dean Jackson]

Comment on attachment 446425 [details]
Patch

I'm surprised we didn't have to update WebCore's BaseTarget.xcconfig too

[Myles C. Maxfield]

I think Alexey's point is right - this is the wrong solution. I need to match what WTF does.

[Alexey Proskuryakov]

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?

[Kyle Piddington]

(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/;

[Kyle Piddington]

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

[Alexey Proskuryakov]

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.

[Kyle Piddington]

Created attachment 446447 [details]
Patch

[Alexey Proskuryakov]

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.

[Kyle Piddington]

Created attachment 446452 [details]
Patch

[Alexey Proskuryakov]

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?

[Kyle Piddington]

Created attachment 446457 [details]
Patch

[Kyle Piddington]

Created attachment 446461 [details]
Patch

[Alexey Proskuryakov]

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.

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/86247153>