Bug 231998

Summary: Mac CMake build should not need to include iOS headers
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: New BugsAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, ap, don.olmstead, ews-watchlist, gyuyoung.kim, ryanhaddad, 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=231982
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Ross Kirsling 2021-10-19 20:00:43 PDT
Mac CMake build should not need to include iOS headers
Comment 1 Ross Kirsling 2021-10-19 20:01:14 PDT
Created attachment 441837 [details]
Patch
Comment 2 Ross Kirsling 2021-10-19 20:04:32 PDT Comment hidden (obsolete)
Comment 3 Ross Kirsling 2021-10-19 20:17:21 PDT
Created attachment 441839 [details]
Patch
Comment 4 Ross Kirsling 2021-10-19 20:51:23 PDT
Created attachment 441843 [details]
Patch
Comment 5 Ross Kirsling 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.
Comment 6 EWS 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].
Comment 7 Radar WebKit Bug Importer 2021-10-20 10:23:19 PDT
<rdar://problem/84468173>
Comment 8 Alexey Proskuryakov 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.
Comment 9 Alex Christensen 2021-10-21 08:53:30 PDT
There has been a successful macCatalyst build since this change.
Comment 10 Ross Kirsling 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.
Comment 11 Alexey Proskuryakov 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?
Comment 12 Ross Kirsling 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.
Comment 13 Alexey Proskuryakov 2021-10-21 10:52:20 PDT
I see. Perhaps Alex is the right person to answer these questions then.
Comment 14 Alex Christensen 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.
Comment 15 Alexey Proskuryakov 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.
Comment 16 Alex Christensen 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?
Comment 17 Alexey Proskuryakov 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.