WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
231998
Mac CMake build should not need to include iOS headers
https://bugs.webkit.org/show_bug.cgi?id=231998
Summary
Mac CMake build should not need to include iOS headers
Ross Kirsling
Reported
2021-10-19 20:00:43 PDT
Mac CMake build should not need to include iOS headers
Attachments
Patch
(19.89 KB, patch)
2021-10-19 20:01 PDT
,
Ross Kirsling
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(20.02 KB, patch)
2021-10-19 20:17 PDT
,
Ross Kirsling
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(19.17 KB, patch)
2021-10-19 20:51 PDT
,
Ross Kirsling
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Ross Kirsling
Comment 1
2021-10-19 20:01:14 PDT
Created
attachment 441837
[details]
Patch
Ross Kirsling
Comment 2
2021-10-19 20:04:32 PDT
Comment hidden (obsolete)
Comment on
attachment 441837
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=441837&action=review
> Source/WebCore/testing/cocoa/WebViewVisualIdentificationOverlay.h:32 > -#import <WebCore/PlatformView.h> > +#import "PlatformView.h"
This line doesn't strictly need to be part of this patch, but the objection ap had expressed in
bug 231749
doesn't actually apply to this include, so I should've just kept it at that time.
Ross Kirsling
Comment 3
2021-10-19 20:17:21 PDT
Created
attachment 441839
[details]
Patch
Ross Kirsling
Comment 4
2021-10-19 20:51:23 PDT
Created
attachment 441843
[details]
Patch
Ross Kirsling
Comment 5
2021-10-19 20:56:00 PDT
Comment on
attachment 441843
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=441843&action=review
> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:30 > +#if TARGET_OS_IPHONE > #import <WebCore/AbstractPasteboard.h> > +#endif > > #if TARGET_OS_IOS
Note: This is extremely silly, but WatchOS evidently *wants* the include and does *not* want the rest of the file.
EWS
Comment 6
2021-10-20 10:22:18 PDT
Committed
r284544
(
243286@main
): <
https://commits.webkit.org/243286@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 441843
[details]
.
Radar WebKit Bug Importer
Comment 7
2021-10-20 10:23:19 PDT
<
rdar://problem/84468173
>
Alexey Proskuryakov
Comment 8
2021-10-20 22:00:03 PDT
Comment on
attachment 441843
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=441843&action=review
> Source/WebCore/ChangeLog:3 > + Mac CMake build should not need to include iOS headers
Is this true? I don’t see why, because of macCatalyst. Also, this patch affects more than CMake. I didn’t check if this broke macCatalyst build or not, still possible that it hasn’t been noticed yet.
Alex Christensen
Comment 9
2021-10-21 08:53:30 PDT
There has been a successful macCatalyst build since this change.
Ross Kirsling
Comment 10
2021-10-21 10:33:53 PDT
(In reply to Alexey Proskuryakov from
comment #8
)
> Comment on
attachment 441843
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=441843&action=review
> > > Source/WebCore/ChangeLog:3 > > + Mac CMake build should not need to include iOS headers > > Is this true? I don’t see why, because of macCatalyst. Also, this patch > affects more than CMake.
The phrasing is very specific. It's not a CMake patch, it's a patch for CMake.
Alexey Proskuryakov
Comment 11
2021-10-21 10:41:04 PDT
Comment on
attachment 441843
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=441843&action=review
>>> Source/WebCore/ChangeLog:3 >>> + Mac CMake build should not need to include iOS headers >> >> Is this true? I don’t see why, because of macCatalyst. Also, this patch affects more than CMake. >> >> I didn’t check if this broke macCatalyst build or not, still possible that it hasn’t been noticed yet. > > The phrasing is very specific. It's not a CMake patch, it's a patch for CMake.
Please answer my questions. 1. What makes you say that "Mac CMake build should not need to include iOS headers"? 2. Why is it OK to change .mm and .h files for this?
Ross Kirsling
Comment 12
2021-10-21 10:43:33 PDT
(In reply to Alexey Proskuryakov from
comment #11
)
> Comment on
attachment 441843
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=441843&action=review
> > >>> Source/WebCore/ChangeLog:3 > >>> + Mac CMake build should not need to include iOS headers > >> > >> Is this true? I don’t see why, because of macCatalyst. Also, this patch affects more than CMake. > >> > >> I didn’t check if this broke macCatalyst build or not, still possible that it hasn’t been noticed yet. > > > > The phrasing is very specific. It's not a CMake patch, it's a patch for CMake. > > Please answer my questions. > > 1. What makes you say that "Mac CMake build should not need to include iOS > headers"? > 2. Why is it OK to change .mm and .h files for this?
Because Alex requested it in
https://bugs.webkit.org/show_bug.cgi?id=231982#c3
. This is literally a goodwill patch.
Alexey Proskuryakov
Comment 13
2021-10-21 10:52:20 PDT
I see. Perhaps Alex is the right person to answer these questions then.
Alex Christensen
Comment 14
2021-10-21 10:54:29 PDT
It is a matter of good code organization for a build of WebKit on Mac to not need things from a directory named "ios". If the code is needed, then it should be in a directory named "cocoa". This level of organization has not been enforced because the Mac build and the iOS build and the macCatalyst build are all mixed together in our internal build system. It hasn't caused a problem in the past, and continues to not cause any problems. If you'd like to point to one problem-causing change in this patch, I'm happy to discuss it.
Alexey Proskuryakov
Comment 15
2021-10-21 12:39:33 PDT
> It is a matter of good code organization for a build of WebKit on Mac to not need things from a directory named "ios". If the code is needed, then it should be in a directory named "cocoa".
This doesn't seem obvious at all, and I don't think that anyone tried to reach consensus on webkit-dev. To me, keeping macCatalyst code in "ios" seems fine and maybe preferable. Although maybe you aren't including macCatalyst when you say "WebKit on Mac", despite it obviously being on Mac?
> If you'd like to point to one problem-causing change in this patch, I'm happy to discuss it.
Let's look at the very first code change in the patch: +#if TARGET_OS_IPHONE #import <WebCore/AbstractPasteboard.h> +#endif I think that there are two issues with it: 1. Even though TARGET_OS_IPHONE is actually the right macro to use for "iOS, tvOS, watchOS, macCatalyst, and simulators", the name is highly misleading, and I wouldn't be using it without a strong reason to. 2. We already have conditionals around included file content. So now we have two, both inside and outside, with is worse than one. I *think* that at some point, we had a consensus to only have the conditionals inside, because having include blocks in files peppered with #ifs is pointless and ugly. It also multiplies the number of places that need to know about which platform each file belongs to, instead of centralizing the knowledge. Of course, there are plenty of examples of not following this principle, which even makes me doubt if my recollection is true. But one way or another, there are real downsides to doing this.
Alex Christensen
Comment 16
2021-10-21 12:45:55 PDT
I agree that the first change adding TARGET_OS_IPHONE is the most questionable part of this, and I'm not a fan of TARGET_OS_IPHONE even though it has over 400 uses in WebKit. Would you be happy if we reverted it?
Alexey Proskuryakov
Comment 17
2021-10-21 15:54:52 PDT
I'm sure that I can nitpick other changes too; pretty sure that conditionalizing both inside and outside applies to many. What would make me happiest is reaching a consensus among WebKit engineers on these points. This patch doesn't do anything unique that's worth reverting for, and maybe the consensus will even be that this is the right direction. But making decisions quietly and based on what's convenient for an unofficial build system seems like a non-ideal way to go.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug