Bug 204480 - Allow for partially-built SafariServices when building WebKit
Summary: Allow for partially-built SafariServices when building WebKit
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-21 16:30 PST by Keith Rollin
Modified: 2019-12-03 21:31 PST (History)
7 users (show)

See Also:


Attachments
Patch (9.73 KB, patch)
2019-11-21 16:33 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2019-12-03 12:57 PST, Keith Rollin
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2019-11-21 16:30:43 PST
WebKit imports <SafariServices/SSReadingList.h> in a couple of places. Normally, this framework header can be found in the iOS SDK. However, in some build configurations, a local, partially-built version of SafariServices will exist. This partially-built framework will have an empty Headers directory. This empty directory will hide the SDK directory, causing the WebKit build to fail because it can't find the SSReadingList.h header. Address this by copying the bits we need from SSReadingList.h into WebKit as a fallback for the times the real header can't be found.
Comment 1 Keith Rollin 2019-11-21 16:30:59 PST
<rdar://problem/57371344>
Comment 2 Keith Rollin 2019-11-21 16:33:25 PST
Created attachment 384102 [details]
Patch
Comment 3 Keith Rollin 2019-12-03 10:39:15 PST
Adding some more people who might review this.
Comment 4 Jonathan Bedard 2019-12-03 10:47:34 PST
Comment on attachment 384102 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:56
> +#if __has_include(<SafariServices/SSReadingList.h>)

Seems like it would be better to move this check inside of SSReadingListSPI.h
Comment 5 Keith Rollin 2019-12-03 11:37:27 PST
OK, will change.
Comment 6 Keith Rollin 2019-12-03 12:57:08 PST
Created attachment 384739 [details]
Patch
Comment 7 Sam Weinig 2019-12-03 13:33:41 PST
What causes this condition to exist? What has changed to make this start happening now?
Comment 8 Tim Horton 2019-12-03 13:37:21 PST
(In reply to Sam Weinig from comment #7)
> What causes this condition to exist? What has changed to make this start
> happening now?

This is not new (not even sort of!). It happens when the SafariServices build fails or is cancelled after creating the framework but before copying the headers, which is not uncommon.
Comment 9 Sam Weinig 2019-12-03 13:52:04 PST
(In reply to Tim Horton from comment #8)
> (In reply to Sam Weinig from comment #7)
> > What causes this condition to exist? What has changed to make this start
> > happening now?
> 
> This is not new (not even sort of!). It happens when the SafariServices
> build fails or is cancelled after creating the framework but before copying
> the headers, which is not uncommon.

Duplicating a header seems like an unfortunate workaround, and one that won't scale. 

Were any alternative solutions considered? For instance, and this may be worse, running a script at the beginning of the build which removes any partially constructed frameworks from the build products? Or getting Xcode to stop doing this?
Comment 10 Keith Rollin 2019-12-03 14:12:41 PST
> What causes this condition to exist?

This condition can exist when building with XCBuild and when using a workspace-based scheme that allows all projects to be examined and built in parallel. SafariServices will start to build and will create its (empty) directory structure, but will then suspend until WebKit is done building.

> What has changed to make this start happening now?

The attempt to enable XCBuild for WebKit builds.

> Duplicating a header seems like an unfortunate workaround, and one that won't scale. 

Yeah, I don't particularly fancy it. But it was approachable with a minimum of fuss. And duplicating the bits of the header that we needed seemed feasible given that the source header doesn't change much, and was inline with the approach in our our SPI files. That said, I'm open to other suggestions. (The one you give doesn't really work given the original motivating task of supporting XCBuild.)
Comment 11 Keith Rollin 2019-12-03 14:18:26 PST
Even if r+'d, I'm going to hold off checking this in. XCBuild may have a change that can help us in Radar 57371055.
Comment 12 Jonathan Bedard 2019-12-03 16:53:55 PST
(In reply to Sam Weinig from comment #9)
> (In reply to Tim Horton from comment #8)
> > (In reply to Sam Weinig from comment #7)
> > > What causes this condition to exist? What has changed to make this start
> > > happening now?
> > 
> > This is not new (not even sort of!). It happens when the SafariServices
> > build fails or is cancelled after creating the framework but before copying
> > the headers, which is not uncommon.
> 
> Duplicating a header seems like an unfortunate workaround, and one that
> won't scale. 
> 
> Were any alternative solutions considered? For instance, and this may be
> worse, running a script at the beginning of the build which removes any
> partially constructed frameworks from the build products? Or getting Xcode
> to stop doing this?

We do this sort of header duplication pretty frequently with system headers, the only difference here is that we're dealing with a system header we own. It's a bit unfortunate, but header duplication is already a cost we seem to have accepted.
Comment 13 Tim Horton 2019-12-03 21:31:48 PST
(In reply to Jonathan Bedard from comment #12)
> (In reply to Sam Weinig from comment #9)
> > (In reply to Tim Horton from comment #8)
> > > (In reply to Sam Weinig from comment #7)
> > > > What causes this condition to exist? What has changed to make this start
> > > > happening now?
> > > 
> > > This is not new (not even sort of!). It happens when the SafariServices
> > > build fails or is cancelled after creating the framework but before copying
> > > the headers, which is not uncommon.
> > 
> > Duplicating a header seems like an unfortunate workaround, and one that
> > won't scale. 
> > 
> > Were any alternative solutions considered? For instance, and this may be
> > worse, running a script at the beginning of the build which removes any
> > partially constructed frameworks from the build products? Or getting Xcode
> > to stop doing this?
> 
> We do this sort of header duplication pretty frequently with system headers,
> the only difference here is that we're dealing with a system header we own.
> It's a bit unfortunate, but header duplication is already a cost we seem to
> have accepted.

Not the same thing. This is a public header, so the usual reasons for having a copy in the tree do not apply.