Bug 238213 – [XCBuild] WebKit's "Migrated headers" script does not emit task information

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Elliott Williams]

Our Make-based scripts which move headers from WebCore to WebKitLegacy and then to WebKit are at odds with how Xcode (and XCBuild) want to manage headers. We need to replace these scripts with a more native project-based representation, so that the build system can understand when these headers need to be (re)copied.

[Elliott Williams]

<rdar://problem/90172142>

[Elliott Williams]

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

[Elliott Williams]

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.

[Elliott Williams]

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.

[Alexey Proskuryakov]

> 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).

[Elliott Williams]

(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.

[Elliott Williams]

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

[Elliott Williams]

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.

[Darin Adler]

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?

[Elliott Williams]

(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?

[Elliott Williams]

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

[Elliott Williams]

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.

[Elliott Williams]

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

[Elliott Williams]

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

[Elliott Williams]

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

[Elliott Williams]

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.

[Darin Adler]

(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).

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Elliott Williams]

(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.

[Elliott Williams]

Created attachment 455656 [details]
r5 + code review fixups

[Elliott Williams]

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

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

[EWS]

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].

[Elliott Williams]

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