Bug 231982 – [CMake] Align OptionsMac with Xcode build

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

[CMake] Align OptionsMac with Xcode build

[Ross Kirsling]

Created attachment 441799 [details]
Patch

[Don Olmstead]

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.

[Alex Christensen]

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.

[Don Olmstead]

Comment on attachment 441799 [details]
Patch

r=me with nits

[Ross Kirsling]

Created attachment 441815 [details]
Patch for landing

[Ross Kirsling]

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

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/84441475>