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.
<rdar://problem/90869551>
Created attachment 455826 [details] Patch
Created attachment 455924 [details] Patch
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".
Created attachment 456034 [details] Build fixes + review feedback
(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.
Created attachment 456071 [details] Patch
Created attachment 456141 [details] Patch
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 :)
Created attachment 456153 [details] Patch
Created attachment 456185 [details] Patch
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!
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.
Created attachment 456255 [details] Fix typo, pre-approved
Comment on attachment 456255 [details] Fix typo, pre-approved Recorded Alexey's r+, ready to land.
(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
Tools/Scripts/svn-apply failed to apply attachment 456255 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 456282 [details] [fast-cq] Rebase with trunk
Tools/Scripts/svn-apply failed to apply attachment 456282 [details] to trunk. Please resolve the conflicts and upload a new patch.
Comment on attachment 456282 [details] [fast-cq] Rebase with trunk Marking cq+ due to false positive related to infrastructure issue.
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].
Re-opened since this is blocked by bug 238639
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
Created attachment 456415 [details] Fix symlinks modifying BUILT_PRODUCTS_DIR
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.
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.
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].