| Summary: | [CMake] Align OptionsMac with Xcode build | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||
| Component: | New Bugs | Assignee: | Ross Kirsling <ross.kirsling> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, annulen, don.olmstead, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=231998 | ||||||||
| Attachments: |
|
||||||||
|
Description
Ross Kirsling
2021-10-19 14:16:29 PDT
Created attachment 441799 [details]
Patch
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 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 on attachment 441799 [details]
Patch
r=me with nits
Created attachment 441815 [details]
Patch for landing
(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. 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]. |