Bug 224084 - Resurrect Mac CMake build
Summary: Resurrect Mac CMake 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: Alex Christensen
URL:
Keywords: InRadar
: 203753 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-04-01 15:40 PDT by Alex Christensen
Modified: 2021-05-07 18:17 PDT (History)
30 users (show)

See Also:


Attachments
Patch (57.90 KB, patch)
2021-04-01 15:42 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (85.46 KB, patch)
2021-04-01 17:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (67.76 KB, patch)
2021-04-01 18:45 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (68.75 KB, patch)
2021-04-01 19:02 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (95.87 KB, patch)
2021-04-01 19:56 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (174.40 KB, patch)
2021-04-02 19:04 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (174.50 KB, patch)
2021-04-02 19:12 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (174.69 KB, patch)
2021-04-02 19:16 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (174.24 KB, patch)
2021-04-02 19:20 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (162.72 KB, patch)
2021-04-05 07:59 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (160.60 KB, patch)
2021-04-05 09:19 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (160.55 KB, patch)
2021-04-05 09:42 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-04-01 15:40:47 PDT
Resurrect Mac CMake build
Comment 1 Alex Christensen 2021-04-01 15:42:07 PDT
Created attachment 424954 [details]
Patch
Comment 2 Alex Christensen 2021-04-01 17:21:53 PDT
Created attachment 424965 [details]
Patch
Comment 3 Alex Christensen 2021-04-01 18:45:44 PDT
Created attachment 424972 [details]
Patch
Comment 4 Alex Christensen 2021-04-01 19:02:53 PDT
Created attachment 424973 [details]
Patch
Comment 5 Alex Christensen 2021-04-01 19:56:02 PDT
Created attachment 424981 [details]
Patch
Comment 6 Tim Horton 2021-04-02 09:26:09 PDT
Comment on attachment 424981 [details]
Patch

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

> Source/WebCore/bridge/objc/WebScriptObject.h:33
> +#if TARGET_OS_IPHONE || defined(DISABLE_LEGACY_WEBKIT_DEPRECATIONS) || defined(BUILDING_WEBKIT)

This is a public header, so this all seems pretty weird. I guess it was already in WebKitAvailability.h, but why can't it stay there?

> Source/WebCore/platform/cocoa/WebKitAvailability.h:-2
> - * Copyright (C) 2013 Apple Inc. All Rights Reserved.

It seems wildly inadvisable to delete a public header.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:274
> +#if ENABLE(GPU_PROCESS)
>          RELEASE_ASSERT(isInGPUProcess() || isInWebProcess());
> +#else

Or just make it say no?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateAVFImpl.cpp:93
> +#if USE(LIBWEBRTC)

Seems like a surprising location for an ifdef like this! What happened here?

> Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.h:32
> -#import <WebCore/PlatformView.h>
> +#import "PlatformView.h"

? This is a private header, this seems wrong.
Comment 7 Alexey Proskuryakov 2021-04-02 09:39:59 PDT
Comment on attachment 424981 [details]
Patch

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

I'm not qualified to review all changes here, but seems clear that there will be enough modification to warrant review of a revised version based on my comments.

Overall, I remain skeptical that CMake will become the build system for Mac or iOS, but the cleanup necessary for it seems like a good thing, and so does experience gained from having experimental support. So I'm supportive of this effort.

> Source/JavaScriptCore/inspector/augmentable/AugmentableInspectorController.h:32
> -#include <JavaScriptCore/AugmentableInspectorControllerClient.h>
> -#include <JavaScriptCore/InspectorBackendDispatcher.h>
> -#include <JavaScriptCore/InspectorFrontendRouter.h>
> +#include "AugmentableInspectorControllerClient.h"
> +#include "InspectorBackendDispatcher.h"
> +#include "InspectorFrontendRouter.h"

AugmentableInspectorController.h is an SPI header, so it's supposed to use framework style includes AFAIK.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/CMBaseObjectSPI.h:85
> +#ifndef CMBASECLASS_DEFINED
> +#define CMBASECLASS_DEFINED

This is a bit unusual, and this file isn't even mentioned in ChangeLog. Can you explain this change in ChangeLog?

> Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:32
> -#include <WebCore/ActiveDOMObject.h>
> -#include <WebCore/JSDOMPromiseDeferred.h>
> +#include "JSDOMPromiseDeferred.h"

ApplePaySetupWebCore.h is a private header too. It has a mix of framework and regular style includes already, but this seems like a move in the wrong direction.

> Source/WebCore/PAL/pal/spi/cf/CoreMediaSPI.h:30
> +#if PLATFORM(MAC) && ENABLE(WEB_RTC)

None of the WebRTC changes are explained in ChangeLogs. What are you trying to achieve here, is it a Mac build with WebRTC disabled? Why?

> Source/WebCore/PAL/pal/spi/cf/CoreMediaSPI.h:40
> +typedef struct OpaqueCMBaseClass *CMBaseClassID;

I suggest making all the "fix open source build" changes as a separate patch from "fix CMake build", as these are separate things that would each benefit from careful expert review, not a rubber-stamp that a huge patch calls for. It is borderline at 95K.

> Source/WebCore/PlatformMac.cmake:654
> -set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1")
> +set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1 WTF_PLATFORM_COCOA=1")

Why is this? WTF_PLATFORM_COCOA is supposed to be computed, not set by the build system.

> Source/WebCore/bridge/objc/WebScriptObject.h:133
> -+ (NSString *)webScriptNameForSelector:(SEL)selector WEBKIT_AVAILABLE_MAC(10_4);
> ++ (NSString *)webScriptNameForSelector:(SEL)selector WEBCORE_AVAILABLE_MAC(10_4);

While these changes are not explained in ChangeLog and I didn't even attempt to understand the rationale, the right thing to do here is to remove the availability declarations. ToT SDK cannot be used when targeting such ancient OS versions, so availability declarations are useless.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:272
> +#if ENABLE(GPU_PROCESS)

What is the goal here? Do we actually need to support Cocoa build with GPU_PROCESS disabled at build time?

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:26
> -#import <WebCore/AbstractPasteboard.h>
> +#import "AbstractPasteboard.h"

This is an SPI header too, so the change looks wrong to me.

> Source/WebCore/rendering/CSSFilter.cpp:311
> -#if USE(CORE_IMAGE)
> +#if USE(CORE_IMAGE) && ENABLE(CORE_IMAGE_ACCELERATED_FILTER_RENDER)

What is this change for?

> Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.h:32
> -#import <WebCore/PlatformView.h>
> +#import "PlatformView.h"

This is an SPI header too.
Comment 8 Alex Christensen 2021-04-02 19:04:19 PDT
Created attachment 425073 [details]
Patch
Comment 9 Alex Christensen 2021-04-02 19:05:13 PDT
I did things a better way with the libwebrtc stuff, and I removed all the controversial things to get it mostly working and easier to review.
Comment 10 Alex Christensen 2021-04-02 19:12:52 PDT
Created attachment 425074 [details]
Patch
Comment 11 Alex Christensen 2021-04-02 19:16:28 PDT
Created attachment 425075 [details]
Patch
Comment 12 Alex Christensen 2021-04-02 19:20:02 PDT
Created attachment 425077 [details]
Patch
Comment 13 Alexey Proskuryakov 2021-04-03 19:14:28 PDT
Comment on attachment 425077 [details]
Patch

The patch is now even larger, and several of my comments haven't been addressed in any way. Please respond, and I can take another look.
Comment 14 Alex Christensen 2021-04-05 07:53:44 PDT
(In reply to Alexey Proskuryakov from comment #7)
> > Source/WebCore/PlatformMac.cmake:654
> > -set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1")
> > +set(CSS_VALUE_PLATFORM_DEFINES "WTF_PLATFORM_MAC=1 HAVE_OS_DARK_MODE_SUPPORT=1 WTF_PLATFORM_COCOA=1")
> 
> Why is this? WTF_PLATFORM_COCOA is supposed to be computed, not set by the
> build system.
This is how it's done in the CMake build, and PlatformMac.cmake is an appropriate place to put this.

> > Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.h:32
> > -#import <WebCore/PlatformView.h>
> > +#import "PlatformView.h"
> 
> This is an SPI header too.
Since we seem to be getting hung up on this, I'll split all of these into a separate patch.
Comment 15 Alex Christensen 2021-04-05 07:59:23 PDT
Created attachment 425151 [details]
Patch
Comment 16 Alexey Proskuryakov 2021-04-05 09:07:51 PDT
Comment on attachment 425151 [details]
Patch

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

Thank you for the cleaner iteration.

I have few enough comments at this point that I'm keeping r? for someone else to review CMake parts of the patch. But please do address my comments.

> Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:32
> +#include "ActiveDOMObject.h"
>  #include "ApplePaySetupConfiguration.h"
> -#include <WebCore/ActiveDOMObject.h>
> -#include <WebCore/JSDOMPromiseDeferred.h>
> +#include "JSDOMPromiseDeferred.h"

ApplePaySetupWebCore.h is an SPI header too, so this remains a move in the wrong direction. Clearly, this isn't breaking the build yet, but why does CMake need this?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:83
> -        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));
> +        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? WebCore::TextEncoding("UTF-8") : WebCore::TextEncoding(m_response.textEncodingName()));

Why is this change needed? There is a "using namespace WebCore;" in this file, is TextEncoding conflicting with something?

> Source/WebKit/UIProcess/Media/RemoteMediaSessionCoordinatorProxy.h:88
> -    Ref<const Logger> m_logger;
> +    Ref<const WTF::Logger> m_logger;

This can't be right, Logger.h contains "using WTF::Logger;".
Comment 17 Alex Christensen 2021-04-05 09:15:25 PDT
(In reply to Alexey Proskuryakov from comment #16)
> Comment on attachment 425151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425151&action=review
> 
> Thank you for the cleaner iteration.
> 
> I have few enough comments at this point that I'm keeping r? for someone
> else to review CMake parts of the patch. But please do address my comments.
> 
> > Source/WebCore/Modules/applepay/ApplePaySetupWebCore.h:32
> > +#include "ActiveDOMObject.h"
> >  #include "ApplePaySetupConfiguration.h"
> > -#include <WebCore/ActiveDOMObject.h>
> > -#include <WebCore/JSDOMPromiseDeferred.h>
> > +#include "JSDOMPromiseDeferred.h"
> 
> ApplePaySetupWebCore.h is an SPI header too, so this remains a move in the
> wrong direction. Clearly, this isn't breaking the build yet, but why does
> CMake need this?
Using CMake with ninja doesn't use the same header finding logic as xcodebuild does, so if there's a header in the same directory as a file being built it will include it.  This causes duplicate inclusions if there is a #include "Header.h" and a #include <Framework/Header.h> sometimes because it finds different headers, each with a #pragma once.  If you oppose this patch because of this change, I can remove it to land the rest.
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:83
> > -        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));
> > +        m_decoder = TextResourceDecoder::create("application/json"_s, m_response.textEncodingName().isEmpty() ? WebCore::TextEncoding("UTF-8") : WebCore::TextEncoding(m_response.textEncodingName()));
> 
> Why is this change needed? There is a "using namespace WebCore;" in this
> file, is TextEncoding conflicting with something?
Yes.  I'd have to restore the parts I removed and rebuild to find the ambiguity.  Something in our SDK.
> 
> > Source/WebKit/UIProcess/Media/RemoteMediaSessionCoordinatorProxy.h:88
> > -    Ref<const Logger> m_logger;
> > +    Ref<const WTF::Logger> m_logger;
> 
> This can't be right, Logger.h contains "using WTF::Logger;".
Logger.h isn't included here.  There is a file that is seeing only a forward declaration.  We need either this or add include "Logger.h" which would unnecessarily increase the build time.
Comment 18 Alex Christensen 2021-04-05 09:19:53 PDT
Created attachment 425155 [details]
Patch
Comment 19 EWS Watchlist 2021-04-05 09:21:03 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 20 Alexey Proskuryakov 2021-04-05 09:36:41 PDT
> This causes duplicate inclusions if there
> is a #include "Header.h" and a #include <Framework/Header.h> sometimes
> because it finds different headers, each with a #pragma once.  If you oppose
> this patch because of this change, I can remove it to land the rest.

I don't think that this change should block landing, as this file already has wrong style includes. But we'll need to switch to header style includes at some point, so moving in the right direction is clearly preferable.

> > This can't be right, Logger.h contains "using WTF::Logger;".
> Logger.h isn't included here.  There is a file that is seeing only a forward
> declaration.  We need either this or add include "Logger.h" which would
> unnecessarily increase the build time.

The correct solution is to add "using WTF::Logger;" to that file that forward declares Logger (why does it even do that instead of including Forward.h?)
Comment 21 Alexey Proskuryakov 2021-04-05 09:38:15 PDT
(and having Forward.h declare Logger, as it doesn't now).
Comment 22 Alex Christensen 2021-04-05 09:42:02 PDT
Created attachment 425162 [details]
Patch
Comment 23 Alexey Proskuryakov 2021-04-05 13:43:42 PDT
Thank you, I have no more comments! Someone else will need to look at CMake parts of the patch.
Comment 24 EWS 2021-04-05 20:19:20 PDT
Committed r275484: <https://commits.webkit.org/r275484>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425162 [details].
Comment 25 Radar WebKit Bug Importer 2021-04-05 20:20:33 PDT
<rdar://problem/76249792>
Comment 26 Alex Christensen 2021-05-07 18:17:32 PDT
*** Bug 203753 has been marked as a duplicate of this bug. ***