Bug 222346 - Fix linker warnings building gtest for macCatalyst
Summary: Fix linker warnings building gtest for macCatalyst
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-23 23:51 PST by Tim Horton
Modified: 2021-02-24 16:41 PST (History)
3 users (show)

See Also:

Attachments
Patch (2.31 KB, patch)
2021-02-23 23:52 PST, Tim Horton 		no flags Details | Formatted Diff | Diff
Patch (1.46 KB, patch)
2021-02-24 11:51 PST, Tim Horton 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.