Bug 222346

Summary: Fix linker warnings building gtest for macCatalyst
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Tim Horton 2021-02-23 23:51:54 PST
Fix linker warnings building gtest for macCatalyst
Comment 1 Tim Horton 2021-02-23 23:52:14 PST
Created attachment 421388 [details]
Patch
Comment 2 Tim Horton 2021-02-23 23:52:18 PST
<rdar://problem/74405116>
Comment 3 Alexey Proskuryakov 2021-02-24 09:54:07 PST
Comment on attachment 421388 [details]
Patch

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

r=me too, with comments.

> Source/ThirdParty/gtest/xcode/Config/FrameworkTarget.xcconfig:22
> +INSTALL_PATH_SDK_VARIANT_macos = $(MACOS_UNZIPPERED_TWIN_PRIVATE_FRAMEWORK_INSTALL_PATH);
> +INSTALL_PATH_SDK_VARIANT_iosmac = $(IOS_UNZIPPERED_TWIN_PRIVATE_FRAMEWORK_INSTALL_PATH);

This is not how we do this elsewhere (cf. WK_ALTERNATE_FRAMEWORKS_DIR). Do we need to modernize all other projects to use these constants? Do they exist on all OS versions we need?

> Source/ThirdParty/gtest/xcode/Config/General.xcconfig:11
> +#include "<DEVELOPER_DIR>/AppleInternal/XcodeConfig/PlatformSupport.xcconfig"

Shouldn't this be "include?"? It is in other projects, and obviously not everyone has it.
Comment 4 Alexey Proskuryakov 2021-02-24 09:56:28 PST
Also, I don't imagine that removing INSTALL_PATH altogether fixes the problem - or does it?
Comment 5 Tim Horton 2021-02-24 10:18:56 PST
Comment on attachment 421388 [details]
Patch

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

>> Source/ThirdParty/gtest/xcode/Config/FrameworkTarget.xcconfig:22
>> +INSTALL_PATH_SDK_VARIANT_iosmac = $(IOS_UNZIPPERED_TWIN_PRIVATE_FRAMEWORK_INSTALL_PATH);
> 
> This is not how we do this elsewhere (cf. WK_ALTERNATE_FRAMEWORKS_DIR). Do we need to modernize all other projects to use these constants? Do they exist on all OS versions we need?

They do exist where we need them, and I don't think any of the other parts of this matter?

>> Source/ThirdParty/gtest/xcode/Config/General.xcconfig:11
>> +#include "<DEVELOPER_DIR>/AppleInternal/XcodeConfig/PlatformSupport.xcconfig"
> 
> Shouldn't this be "include?"? It is in other projects, and obviously not everyone has it.

Yes, this is a good point.
Comment 6 Tim Horton 2021-02-24 10:19:18 PST
(In reply to Alexey Proskuryakov from comment #4)
> Also, I don't imagine that removing INSTALL_PATH altogether fixes the
> problem - or does it?

I have not tried that!
Comment 7 Tim Horton 2021-02-24 11:45:41 PST
(In reply to Tim Horton from comment #6)
> (In reply to Alexey Proskuryakov from comment #4)
> > Also, I don't imagine that removing INSTALL_PATH altogether fixes the
> > problem - or does it?
> 
> I have not tried that!

It does!
Comment 8 Tim Horton 2021-02-24 11:51:01 PST
Created attachment 421435 [details]
Patch
Comment 9 Alexey Proskuryakov 2021-02-24 12:58:09 PST
Comment on attachment 421435 [details]
Patch

Yay.
Comment 10 EWS 2021-02-24 13:13:30 PST
Committed r273430: <https://commits.webkit.org/r273430>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421435 [details].
Comment 11 Alexey Proskuryakov 2021-02-24 16:41:09 PST
> They do exist where we need them, and I don't think any of the other parts of this matter?

Doesn't matter for landing the original patch, definitely doesn't matter for the final patch, matters for understanding what future cleanup is ahead of us. Using multiple ways to do the same thing forever wouldn't have been great.