Bug 203059 - Add Swift modulemap for WebKit Private APIs
Summary: Add Swift modulemap for WebKit Private APIs
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-16 15:36 PDT by Dean Jackson
Modified: 2021-12-12 08:44 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.55 KB, patch)
2019-10-16 15:39 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (20.96 KB, patch)
2021-02-23 17:16 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (29.02 KB, patch)
2021-02-24 15:21 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (30.35 KB, patch)
2021-02-24 15:24 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (29.96 KB, patch)
2021-02-26 10:58 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
WIP - does not compile (31.24 KB, patch)
2021-03-29 13:33 PDT, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2019-10-16 15:36:29 PDT
Add Swift modulemap for WebKit Private APIs
Comment 1 Dean Jackson 2019-10-16 15:39:50 PDT
Created attachment 381125 [details]
Patch
Comment 2 Tim Horton 2019-10-16 15:42:00 PDT
Comment on attachment 381125 [details]
Patch

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

> Source/WebKit/Shared/API/Cocoa/WebKitPrivate.h:28
> +#import <WebKit/WKPreferencesPrivate.h>

This is still hilariously incomplete. Should we just add everything?
Comment 3 Dean Jackson 2019-10-16 15:47:56 PDT
Comment on attachment 381125 [details]
Patch

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

>> Source/WebKit/Shared/API/Cocoa/WebKitPrivate.h:28
>> +#import <WebKit/WKPreferencesPrivate.h>
> 
> This is still hilariously incomplete. Should we just add everything?

Will do separately. (even though it is a bit of a cheat that this patch is doing the single one I care about right now)
Comment 4 Radar WebKit Bug Importer 2019-10-16 15:49:39 PDT
<rdar://problem/56350290>
Comment 5 Dean Jackson 2019-10-16 15:49:57 PDT
Committed r251215: <https://trac.webkit.org/changeset/251215>
Comment 6 Dean Jackson 2021-02-23 12:08:11 PST
This got rolled out almost immediately. Time to try it again.
Comment 7 Dean Jackson 2021-02-23 17:16:40 PST
Created attachment 421377 [details]
Patch
Comment 8 Tim Horton 2021-02-23 17:35:21 PST
Comment on attachment 421377 [details]
Patch

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

> Source/WebKit/Shared/API/Cocoa/WebKit.h:2
> + * Copyright (C) 2014-2021 Apple Inc. All rights reserved.

I think this file needs a .in extension or something now?

> Source/WebKit/Shared/API/Cocoa/WebKitPrivate.h:2
> + * Copyright (C) 2014-2021 Apple Inc. All rights reserved.

Ditto
Comment 9 Adam Roben (:aroben) 2021-02-24 11:56:30 PST
Comment on attachment 421377 [details]
Patch

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

> Source/WebKit/Scripts/generate-umbrella-header.py:1
> +#!/usr/bin/python

Does this file need a license too? (Maybe even the module maps?)
Comment 10 Dean Jackson 2021-02-24 14:23:27 PST
Comment on attachment 421377 [details]
Patch

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

>> Source/WebKit/Scripts/generate-umbrella-header.py:1
>> +#!/usr/bin/python
> 
> Does this file need a license too? (Maybe even the module maps?)

The existing module maps did not. But yes, this should.
Comment 11 Dean Jackson 2021-02-24 15:21:28 PST
Created attachment 421468 [details]
Patch
Comment 12 Dean Jackson 2021-02-24 15:24:44 PST
Created attachment 421471 [details]
Patch
Comment 13 Alexey Proskuryakov 2021-02-24 15:28:16 PST
Is Python code here tested with both Python 2 and Python 3?
Comment 14 Dean Jackson 2021-02-25 14:24:47 PST
(In reply to Alexey Proskuryakov from comment #13)
> Is Python code here tested with both Python 2 and Python 3?

Yes.
Comment 15 Dean Jackson 2021-02-25 14:25:31 PST
These failures are because the bot needs a clean build.
Comment 16 Alexey Proskuryakov 2021-02-25 14:38:42 PST
Is there a way to not require a clean build? We are way conditioned to allowing ourselves that, but it's never correct behavior.
Comment 17 Dean Jackson 2021-02-25 15:17:03 PST
(In reply to Alexey Proskuryakov from comment #16)
> Is there a way to not require a clean build? We are way conditioned to
> allowing ourselves that, but it's never correct behavior.

Are you asking me? I have no idea why TAPI fails on a non-clean bot build, so no, I don't know how to stop requiring it.

Truitt tickled the mac bots, which shows that a clean build succeeds.
Comment 18 Alexey Proskuryakov 2021-02-25 15:29:04 PST
Comment on attachment 421471 [details]
Patch

It helps to know that it was an InstallAPI failure. Generally speaking, we cannot do much to fix incremental builds when API/SPI headers get renamed, and an old version is still present in the file system due to incremental nature of the build. We could and maybe should get into the habit of adding scripts to clean those up, but we haven't yet.

This patch makes NSURLDownloadSPI.h non-private, which appears unacceptable given its name. So InstallAPI inadvertently pointed out a real problem for once.
Comment 19 Alexey Proskuryakov 2021-02-25 15:31:02 PST
If I'm missing something and it is acceptable, please fell encourage to r? again.
Comment 20 Alexey Proskuryakov 2021-02-25 15:36:39 PST
> This patch makes NSURLDownloadSPI.h non-private, which appears unacceptable
> given its name.

Not given its name, but given that WebDownload.h is private, and imports it.
Comment 21 Adam Roben (:aroben) 2021-02-25 19:20:36 PST
Comment on attachment 421471 [details]
Patch

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

> Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj:-690
> -		E1531BD82187B954002E3F81 /* NSURLDownloadSPI.h in Headers */ = {isa = PBXBuildFile; fileRef = E1531BD72187B8F2002E3F81 /* NSURLDownloadSPI.h */; settings = {ATTRIBUTES = (Private, ); }; };

Looks like we need to undo this change since WebDownload.h imports this header and WebDownload.h is a private header.
Comment 22 Dean Jackson 2021-02-26 10:58:43 PST
Created attachment 421677 [details]
Patch
Comment 23 Dean Jackson 2021-02-26 10:59:39 PST
The problem is that if you import the system NSURLDownload you'll get a clash if you then import NSURLDownloadSPI. TestWebKitAPI hits this.

This new patch adds the same guards that WebDownload does, so it shouldn't cause a clash if you include the umbrella.
Comment 24 Adam Roben (:aroben) 2021-02-26 11:01:09 PST
Comment on attachment 421677 [details]
Patch

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

There are bunch of modularization-related warnings we could enable too:

-Watimport-in-framework-header
-Wframework-include-private-from-public
-Wincomplete-framework-module-declaration
-Wincomplete-umbrella
-Wnon-modular-include-in-framework-module
-Wnon-modular-include-in-module
-Wquoted-include-in-framework-header

> Source/WebKit/WebKit.xcodeproj/xcshareddata/xcschemes/WebKit.xcscheme:34
> -      buildConfiguration = "Release"
> +      buildConfiguration = "Debug"

You probably didn't leave to mean this change in.
Comment 25 Dean Jackson 2021-03-29 12:51:50 PDT
-Watimport-in-framework-header
No config equivalent

-Wframework-include-private-from-public
CLANG_WARN_FRAMEWORK_INCLUDE_PRIVATE_FROM_PUBLIC=YES

-Wincomplete-framework-module-declaration
No config equivalent

-Wincomplete-umbrella
No config equivalent

-Wnon-modular-include-in-framework-module
CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES=NO
(this is the default)

-Wnon-modular-include-in-module
No config equivalen

-Wquoted-include-in-framework-header
CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER=YES
Comment 26 Dean Jackson 2021-03-29 13:23:55 PDT
This is going to take a bit of cleanup in JSC and WebCore.

For example WebKit's APICustomHeaderFields.h includes WebCore's CustomHeaderFields.h which has a quoted include.
Comment 27 Dean Jackson 2021-03-29 13:33:24 PDT
Created attachment 424572 [details]
WIP - does not compile