Bug 231982 - [CMake] Align OptionsMac with Xcode build
Summary: [CMake] Align OptionsMac with Xcode build
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: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-19 14:16 PDT by Ross Kirsling
Modified: 2021-10-19 20:02 PDT (History)
8 users (show)

See Also:


Attachments
Patch (13.72 KB, patch)
2021-10-19 14:21 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (14.92 KB, patch)
2021-10-19 16:30 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2021-10-19 14:16:29 PDT
[CMake] Align OptionsMac with Xcode build
Comment 1 Ross Kirsling 2021-10-19 14:21:52 PDT
Created attachment 441799 [details]
Patch
Comment 2 Don Olmstead 2021-10-19 14:51:56 PDT
Comment on attachment 441799 [details]
Patch

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

LGTM overall. Just remember that any option that isn't the same as the default set in WEBKIT_OPTION_DEFINE should be explicitly set.

> Source/WebCore/PlatformMac.cmake:134
> +    "${WEBCORE_DIR}/platform/mediastream/ios"

Its my understanding iOS builds aren't supported. Does this directory have a file that is maybe in the wrong location?

> Source/WebKit/CMakeLists.txt:632
> +if (APPLE)
> +    WEBKIT_ADD_TARGET_CXX_FLAGS(WebKit -fobjc-weak)
> +endif ()

I don't know enough about ObjC to know if we should have something along the lines of a `WEBKIT_ADD_TARGET_OBJCPP_FLAGS` macro.

> Source/cmake/WebKitFeatures.cmake:-110
> -    WEBKIT_OPTION_DEFINE(ENABLE_APPLE_PAY_SESSION_V9 "Toggle Apple Pay Session V9 support" PRIVATE OFF)

These enables that are dependent on an internal SDK seem ok to remove from the listing but I'd probably want Alex to chime in.
Comment 3 Alex Christensen 2021-10-19 14:56:20 PDT
Comment on attachment 441799 [details]
Patch

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

>> Source/WebCore/PlatformMac.cmake:134
>> +    "${WEBCORE_DIR}/platform/mediastream/ios"
> 
> Its my understanding iOS builds aren't supported. Does this directory have a file that is maybe in the wrong location?

Either something is in the wrong directory or something is included on Mac that shouldn't be.  Either way, there are a few of these and we ought to fix them but it's not critical.

>> Source/WebKit/CMakeLists.txt:632
>> +endif ()
> 
> I don't know enough about ObjC to know if we should have something along the lines of a `WEBKIT_ADD_TARGET_OBJCPP_FLAGS` macro.

I don't think that's needed.  I don't think there are things we would want to add to objc++ but not to regular c++.  This looks ok to me.

>> Source/cmake/WebKitFeatures.cmake:-110
>> -    WEBKIT_OPTION_DEFINE(ENABLE_APPLE_PAY_SESSION_V9 "Toggle Apple Pay Session V9 support" PRIVATE OFF)
> 
> These enables that are dependent on an internal SDK seem ok to remove from the listing but I'd probably want Alex to chime in.

This is fine.
Comment 4 Don Olmstead 2021-10-19 16:14:49 PDT
Comment on attachment 441799 [details]
Patch

r=me with nits
Comment 5 Ross Kirsling 2021-10-19 16:30:18 PDT
Created attachment 441815 [details]
Patch for landing
Comment 6 Ross Kirsling 2021-10-19 16:31:33 PDT
(In reply to Alex Christensen from comment #3)
> >> Source/WebCore/PlatformMac.cmake:134
> >> +    "${WEBCORE_DIR}/platform/mediastream/ios"
> > 
> > Its my understanding iOS builds aren't supported. Does this directory have a file that is maybe in the wrong location?
> 
> Either something is in the wrong directory or something is included on Mac
> that shouldn't be.  Either way, there are a few of these and we ought to fix
> them but it's not critical.

Added the correct fix for the issue introduced in bug 231455; can handle more of the existing cases in a follow-up.
Comment 7 EWS 2021-10-19 17:30:39 PDT
Committed r284512 (243256@main): <https://commits.webkit.org/243256@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441815 [details].
Comment 8 Radar WebKit Bug Importer 2021-10-19 17:31:20 PDT
<rdar://problem/84441475>