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-
Patch (20.02 KB, patch)
2021-10-19 20:17 PDT, Ross Kirsling
ews-feeder: commit-queue-
Patch (19.17 KB, patch)
2021-10-19 20:51 PDT, Ross Kirsling
no flags
Ross Kirsling
Comment 1 2021-10-19 20:01:14 PDT
Ross Kirsling
Comment 2 2021-10-19 20:04:32 PDT Comment hidden (obsolete)
Ross Kirsling
Comment 3 2021-10-19 20:17:21 PDT
Ross Kirsling
Comment 4 2021-10-19 20:51:23 PDT
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
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.