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

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Elliott Williams]

Similar to https://bugs.webkit.org/show_bug.cgi?id=238213, we'll need to use a build rule + file references to replace the Makefile that implements WebCore -> WebKitLegacy migration logic. This will be unique, however, in that we need to run tapi-reexport on the migrated WebCore headers, and ensure that WebKitLegacy's exported symbols do not change.

[Radar WebKit Bug Importer]

<rdar://problem/90869551>

[Elliott Williams]

Created attachment 455826 [details]
Patch

[Elliott Williams]

Created attachment 455924 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 455924 [details]
Patch

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

Marking r- because this breaks the build.

> Source/WebKitLegacy/ChangeLog:25
> +        multicore machine it should be faster than blocking WebKitLegacy's build process on the

This matches my instincts, yet I'm curious if you measured the difference.

> Source/WebKitLegacy/mac/ChangeLog:11
> +        USE_RECURSIVE_SCRIPT_INPUTS_IN_SCRIPT_PHASES, so that the "Generate Export Files" phase can

I didn't know this existed!

> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:38
> +// WebEventRegion.h is migrated when building for an iOS family target with an internal SDKâ¦
> +INCLUDED_MIGRATED_HEADERS_YES_cocoatouch = WebEventRegion.h;
> +// â¦or when --ios-touch-events was passed to build-webkit manually.

I'm guessing these are ellipses? Not sure how I feel about them, would probably use ASCII.

> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:42
> +// The "Generate Export Files" build pahse needs this setting to rerun when any migrated header changes:

typo: pahse.

> Tools/ChangeLog:11
> +        script should check PrivateHeaders/ too.

Not sure if this comment matches what the change does. The change switches from "Headers" to "PrivateHeaders", so it's not obvious why it's a "too".

[Elliott Williams]

Created attachment 456034 [details]
Build fixes + review feedback

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 455924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455924&action=review
> 
> Marking r- because this breaks the build.
> 
> > Source/WebKitLegacy/ChangeLog:25
> > +        multicore machine it should be faster than blocking WebKitLegacy's build process on the
> 
> This matches my instincts, yet I'm curious if you measured the difference.

I did some back-of-napkin calculations earlier, but here's some actual data (measuring CPU time in seconds) over 6 builds. These were generated using "Build with Timing Summary" in Xcode:

Style       Build time (Median) RuleScriptExecution (Median)    PhaseScriptExecution (Median)
Build rule  34.8                14.754                          4.213
Make        33.6                10.844                          4.715

On my 12-core machine, the extra 3.91 sec in RuleScriptExecution should be 0.32 sec of CPU time, which is less than the 0.50 sec execution time of the build phase. In practice, the actual build time was slightly higher for the rule phases, which may be explained by the build system having additional overhead or the script phase being able to execute while other targets in the workspace are building. Still, it seems like this is a scalable approach.


> > Source/WebKitLegacy/mac/ChangeLog:11
> > +        USE_RECURSIVE_SCRIPT_INPUTS_IN_SCRIPT_PHASES, so that the "Generate Export Files" phase can
> 
> I didn't know this existed!

Neither did I! It's documented in https://developer.apple.com/documentation/xcode-release-notes/xcode-11-release-notes but not surfaced in the build settings UI.

[Elliott Williams]

Created attachment 456071 [details]
Patch

[Elliott Williams]

Created attachment 456141 [details]
Patch

[Elliott Williams]

Comment on attachment 456141 [details]
Patch

I think I'm narrowing down the cause of EWS build failures. They are related to the hacky "(Legacy) Install Headers" build phase, which is extra tricky here because we have to invoke clang (via tapi) while running the header build rule.

By not setting SYMROOT as I'd done before in WebKit, WTF, and PAL, the inferior xcodebuild was replacing WebKitLegacy.framework with a symlink to /tmp/WebKitLegacy.dst/WebKitLegacy.framework, causing linker failures.

Now, we give the inferior xcodebuild its own SYMROOT directory, but create symlinks for all the existing build products from the superior's SYMROOT so that tapi can run successfully.

Apologies for the EWS noise. I look forward to deleting these scripts soon :)

[Elliott Williams]

Created attachment 456153 [details]
Patch

[Elliott Williams]

Created attachment 456185 [details]
Patch

[Elliott Williams]

Comment on attachment 456185 [details]
Patch

ios-wk2 test failures <https://ews-build.webkit.org/#/builders/68/builds/11945> look unrelated. There were a few tests that crashed but no launch issues that could be caused by a build system change.

Alexey, I think this is ready to go!

[Alexey Proskuryakov]

Comment on attachment 456185 [details]
Patch

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

> Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj:697
> +			filePatterns = "*/Source/WebCore/*.h";

As discussed in person, I'm confused about how this worked before. WebKitLegacy couldn't pull headers directly from Source/WebCore until semi-recently.

> Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj:706
> +				"$(HEADER_OUTPUT_DIR)/$(INPUT_FILE_NAME)",
> +				"$(DERIVED_FILE_DIR)/WebCore/$(INPUT_FILE_NAME)",

Not excited about creating two files with the same name, as always.

[Elliott Williams]

Created attachment 456255 [details]
Fix typo, pre-approved

[Elliott Williams]

Comment on attachment 456255 [details]
Fix typo, pre-approved

Recorded Alexey's r+, ready to land.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #13)
> Comment on attachment 456185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456185&action=review
> 
> > Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj:697
> > +			filePatterns = "*/Source/WebCore/*.h";
> 
> As discussed in person, I'm confused about how this worked before.
> WebKitLegacy couldn't pull headers directly from Source/WebCore until
> semi-recently.

Previously, the "Migrate Headers" makefile copied from WebCore's product headers, in WebCore.framework/PrivateHeaders. It used the locally-built WebCore in BUILT_PRODUCTS_DIR or SDKROOT, for engineering or production builds respectively. The logic to pick which location to get WebCore headers from was in migrate-headers.sh:

    # If we didn't build WebCore, use the production copy of the headers
    if [ ! -d "${WEBCORE_PRIVATE_HEADERS_DIR}" ]; then
        export WEBCORE_PRIVATE_HEADERS_DIR="${WEBCORE_PRIVATE_HEADERS_DIR_Production}"
    fi
    

Now, we pull migrated headers directly from other projects' source roots, because it's difficult to get Xcode to store file references to product headers, since their paths change depending on the platform we're building for. For reference, here's where we discussed doing the same thing in modern WebKit: https://bugs.webkit.org/show_bug.cgi?id=238213#c4

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 456255 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Elliott Williams]

Created attachment 456282 [details]
[fast-cq] Rebase with trunk

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 456282 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Ryan Haddad]

Comment on attachment 456282 [details]
[fast-cq] Rebase with trunk

Marking cq+ due to false positive related to infrastructure issue.

[EWS]

Committed r292183 (249086@main): <https://commits.webkit.org/249086@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 238639

[Ryan Haddad]

Reverted in https://commits.webkit.org/r292188 as this appears to have caused the following build failure: https://build.webkit.org/#/builders/29/builds/11637

[Elliott Williams]

Created attachment 456415 [details]
Fix symlinks modifying BUILT_PRODUCTS_DIR

[Elliott Williams]

Comment on attachment 456415 [details]
Fix symlinks modifying BUILT_PRODUCTS_DIR

This was quite the build bug. Here's my analysis:

As explained in the patch's changelog, the "(Legacy) Install Headers" script phase creates symlinks into BUILT_PRODUCTS_DIR for the inferior (child) xcodebuild:

    for p in "${BUILT_PRODUCTS_DIR}"/*; do
        ln -sf "${p}" "${INFERIOR_BUILT_PRODUCTS_DIR}/$(basename "${p}")"   # WRONG
    done
    
On a clean build, this works as intended. But if "${p}" already exists, and it points to a directory, `ln` enters the directory and creates the symlink inside it. This leads to situations where a framework bundle contains a symlink to itself:

    > ls -l WebKitBuild/Debug/WebKitLegacy.framework
    total 0
    lrwxr-xr-x  1 emw  admin   31 Apr  1 15:24 PrivateHeaders -> Versions/Current/PrivateHeaders
    lrwxr-xr-x  1 emw  admin   26 Apr  1 15:24 Resources -> Versions/Current/Resources
    drwxr-xr-x  4 emw  admin  128 Apr  1 15:24 Versions
    lrwxr-xr-x  1 emw  admin   29 Apr  1 15:24 WebKitLegacy -> Versions/Current/WebKitLegacy
    lrwxr-xr-x  1 emw  admin   72 Apr  1 15:33 WebKitLegacy.framework -> WebKitBuild/Debug/WebKitLegacy.framework  # WRONG
    lrwxr-xr-x  1 emw  admin   34 Apr  1 15:24 WebKitPluginAgent -> Versions/Current/WebKitPluginAgent
    lrwxr-xr-x  1 emw  admin   37 Apr  1 15:24 WebKitPluginHost.app -> Versions/Current/WebKitPluginHost.app

Most of the time, this is benign, because nothing tries to read the symlink. But on Mac, this symlink violates bundle structure, because the top-level bundle directory should only contain `Info.plist` and `Contents`. Bundles that are codesigned have their structure validated. In our case, `com.apple.WebKit.WebAuthn.xpc` is the unlucky bundle that signs first, failing with:

    /usr/bin/codesign --force --sign - --entitlements WebKitBuild/WebKit.build/Release/WebAuthn.build/com.apple.WebKit.WebAuthn.xpc.entitlements --timestamp=none 
    WebKitBuild/Release/com.apple.WebKit.WebAuthn.xpc: unsealed contents present in the bundle root
    
WebAuthn is part of WebKit, which builds after WebKitLegacy. Because of this, it takes at least _two builds_ to create the build-breaking symlink.

The fix is to pass a flag to tell `ln` to overwrite an existing symlink:

    for p in "${BUILT_PRODUCTS_DIR}"/*; do
        ln -sfhv "${p}" "${INFERIOR_BUILT_PRODUCTS_DIR}/$(basename "${p}")"  # RIGHT
    done

-h prevents `ln` from traversing the symlink when it's replacing it. -v makes it print the links it's creating, so that mistakes like this will be evident in build logs.

[Alexey Proskuryakov]

Comment on attachment 456415 [details]
Fix symlinks modifying BUILT_PRODUCTS_DIR

We usually just do "ln -sfh" exactly because of this; I should have noticed the lack of the usual flag.

[EWS]

Committed r292292 (249190@main): <https://commits.webkit.org/249190@main>

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