Bug 238213

<rdar://problem/90172142>

Created attachment 455427 [details]
WebKit: Use native headers + build rules for migrated headers

Comment on attachment 455427 [details]
WebKit: Use native headers + build rules for migrated headers

This first patch is _only_ the changes to convert WebKit's migrated headers phase, not WebKitLegacy's. We can land them separately.

Comment on attachment 455427 [details]
WebKit: Use native headers + build rules for migrated headers

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

There are a few instances of WebKit.xcodeproj referring to files in sibling project directories (../WebKitLegacy and ../WebCore). I know this has historically been unsupported for production builds, is that still the case? See below:

> Source/WebKit/Scripts/migrate-headers-rule.sh:11
> +    SCRIPT_INPUT_FILE="${SCRIPT_OUTPUT_FILE_0}" "${SRCROOT}/../WebKitLegacy/scripts/postprocess-header-rule"

If this one is a problem, I could make WebKitLegacy install this script to the private SDK.

Alternatively, I could copy-paste the relevant portions of this script to WebKit's `postprocess-header-rule` script. The reason we have to run both of them is because WebKitLegacy's script does different transformations.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:6741
> +		DDA0A17C27E55E24005E086E /* DOMCSSImportRule.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DOMCSSImportRule.h; path = ../WebKitLegacy/mac/DOM/DOMCSSImportRule.h; sourceTree = "<group>"; };

All files in the "Migrated Headers" group refer to sibling directories. If that's a problem, I could change WebKit's `installsrc` recipe to copy these over.

> I know this has historically been unsupported for production builds, is that still the case?

This works now. It makes it harder to reason about code though (e.g. one could be forgiven for only searching WebKitLegacy for how WebKitLegacy/scripts/postprocess-header-rule is used).

(In reply to Alexey Proskuryakov from comment #5)
> > I know this has historically been unsupported for production builds, is that still the case?
> 
> This works now. It makes it harder to reason about code though (e.g. one
> could be forgiven for only searching WebKitLegacy for how
> WebKitLegacy/scripts/postprocess-header-rule is used).

Great! Not having to worry about headers referencing sibling projects is ideal [1]. I'll see about refactoring the build rule script, though. I already don't like how wordy it is given that it gets interpreted once per header.

[1]: While I was working on this, I tried using references to the headers from build products instead of their source locations (i.e. WebKit would refer to WebKitLegacy.framework/PrivateHeaders/foo.h instead of Source/WebKitLegacy/foo.h). The problem with this is that the path to the headers changes when we build for macOS due to the `Versions/A/` bundle prefix. It also makes searching for these headers in Xcode more difficult because it adds a different, editable file with the same name.

Created attachment 455510 [details]
WebKit: Use native headers + build rules for migrated headers r2

Comment on attachment 455510 [details]
WebKit: Use native headers + build rules for migrated headers r2

This should fix the open-source iOS build error regarding WebEventRegion.h, and it refactors WebKit's postprocess rule to handle all the transformation it needs for migrated headers, without calling into WebKitLegacy scripts.

Comment on attachment 455510 [details]
WebKit: Use native headers + build rules for migrated headers r2

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

Very excited about these improvements. Haven’t been able to carefully review everything yet, but it seems like we’re on the right track.

> Source/WebKit/Shared/API/Cocoa/WebKitLegacy.h:29
> +#if defined(__has_include) && __has_include(<WebKitLegacy/WebKit.h>)

It’s really unpleasant to use __has_include in a public header. Is there any way to avoid this?

(In reply to Darin Adler from comment #9)
> Comment on attachment 455510 [details]
> WebKit: Use native headers + build rules for migrated headers r2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455510&action=review
> 
> Very excited about these improvements. Haven’t been able to carefully review
> everything yet, but it seems like we’re on the right track.
Thank you!

> > Source/WebKit/Shared/API/Cocoa/WebKitLegacy.h:29
> > +#if defined(__has_include) && __has_include(<WebKitLegacy/WebKit.h>)
> 
> It’s really unpleasant to use __has_include in a public header. Is there any
> way to avoid this?
This is what we already do in the non-Mac SDKs, and I've been trying to get this patch to match the SDK headers exactly. That said, perhaps we could switch it to:

    #if defined(USE_APPLE_INTERNAL_SDK) && USE_APPLE_INTERNAL_SDK
    #import <WebKitLegacy/WebKit.h>
    #endif

…which will get removed by unifdef in the public SDK.

I am not confident enough about the implications of changing a public header in this way to make the call. What do you think?

Created attachment 455542 [details]
WebKit: Use native headers + build rules for migrated headers r3

Comment on attachment 455542 [details]
WebKit: Use native headers + build rules for migrated headers r3

This revision incorporates some changes to WebKit's postprocess-header-rule that Alexey and I discussed. The key change is that it incorporates the transformations that WebKitLegacy and the MigrateHeaders* makefiles were doing.

To make it easier to understand, it's refactored so that all the special cases only control which flags are passed to a single pipeline. It does less disk I/O overall, and is slightly faster to execute.

Created attachment 455597 [details]
WebKit: Use native headers + build rules for migrated headers r4

Comment on attachment 455597 [details]
WebKit: Use native headers + build rules for migrated headers r4

Pass configuration thru to child xcodebuild to fix open-source iOS builds

Created attachment 455609 [details]
WebKit: Use native headers + build rules for migrated headers r5

Comment on attachment 455609 [details]
WebKit: Use native headers + build rules for migrated headers r5

Pass $(ARCHS) through to the child xcodebuild, too. WebKit.xcodeproj defaults to $(ARCHS_STANDARD_32_64_BIT) in the Debug and Release configurations, which is not supported on Intel platforms because Xcode no longer builds for i386. Since build-webkit provides its own ARCHS override, just ensure it's passed down.

(In reply to Elliott Williams from comment #10)
> That said, perhaps we could switch it to:
>
>     #if defined(USE_APPLE_INTERNAL_SDK) && USE_APPLE_INTERNAL_SDK
>     #import <WebKitLegacy/WebKit.h>
>     #endif
>
> …which will get removed by unifdef in the public SDK.
>
> I am not confident enough about the implications of changing a public header
> in this way to make the call. What do you think?

This technique seems sound to me. Not sure about how easy it will be to execute correctly without causing headaches, but it’s where we’d want to end up.

One thing we need is a way to *verify* that the the unifdef'd versions of public headers don’t contain anything inappropriate for a public header. We want to do that soon after people edit header files at their desks, before a patch is even reviewed. We do not want to find out later, when we do a production build.

We don’t want to end up with a PLATFORM(XXX) or other macro check that is fine inside WebKit to creep into a header used outside the WebKit frameworks. It’s really hard for people to remember to not make that kind of mistake, when they are focused on their task and most of the code they are editing is inside the WebKit world.

I would like to see something like the above as our long term solution. We want to optimize for the cleanliness, simplicity, and clarity of the actual public headers as included in SDKs (and similarly for private headers, for builds within Apple).

Comment on attachment 455609 [details]
WebKit: Use native headers + build rules for migrated headers r5

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

> Source/WebKit/ChangeLog:36
> +        * mac/replace-webkit-additions-includes.py: Refactored to use stdin and stdout rather than

Doesn't need to be part of this patch, yet I can't help but notice that this script has a python2 shebang. Seems like it's always invoked with python3 though.

Comment on attachment 455609 [details]
WebKit: Use native headers + build rules for migrated headers r5

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

Elliott filed bug 238331 to investigate unifdef further. Sounds the rest is uncontroversial, so r=me.

> Source/WebKit/Scripts/postprocess-header-rule:47
> +# TODO: rdar://90704735 (Run unifdef more uniformly on all WebKit.framework headers)

Please reference bug 238331. Also, WebKit coding style calls for FIXME, although there are a lot of TODOs that creeped in over the years, too.

(In reply to Darin Adler from comment #17)
> 
> One thing we need is a way to *verify* that the the unifdef'd versions of
> public headers don’t contain anything inappropriate for a public header. We
> want to do that soon after people edit header files at their desks, before a
> patch is even reviewed. We do not want to find out later, when we do a
> production build.
>
> We don’t want to end up with a PLATFORM(XXX) or other macro check that is
> fine inside WebKit to creep into a header used outside the WebKit
> frameworks. It’s really hard for people to remember to not make that kind of
> mistake, when they are focused on their task and most of the code they are
> editing is inside the WebKit world.
We have the "Check For Framework Include Consistency" and "Check For Inappropriate Macros in External Headers" script phases which put some guard rails up here. That said, it's too bad that the postprocessed headers are mostly invisible to someone working in Xcode. Maybe there's a way we can tie into the Counterparts system so that you could see the unifdef'd header side-by-side with the source.

> I would like to see something like the above as our long term solution. We
> want to optimize for the cleanliness, simplicity, and clarity of the actual
> public headers as included in SDKs (and similarly for private headers, for
> builds within Apple).
As Alexey noted, I think this dovetails with https://bugs.webkit.org/show_bug.cgi?id=238331 and the associated radar. I'll post a patch there that cleans this up.

Created attachment 455656 [details]
r5 + code review fixups

Comment on attachment 455656 [details]
r5 + code review fixups

s/python/python3 and s/TODO/FIXME. Landing per Alexey's r+

Committed r291809 (248836@main): <https://commits.webkit.org/248836@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455656 [details].

Created https://bugs.webkit.org/show_bug.cgi?id=238409 to track the equivalent work in WebKitLegacy.