Add Swift modulemap for WebKit Private APIs

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Add Swift modulemap for WebKit Private APIs

[Dean Jackson]

Created attachment 381125 [details]
Patch

[Tim Horton]

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?

[Dean Jackson]

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)

[Radar WebKit Bug Importer]

<rdar://problem/56350290>

[Dean Jackson]

Committed r251215: <https://trac.webkit.org/changeset/251215>

[Dean Jackson]

This got rolled out almost immediately. Time to try it again.

[Dean Jackson]

Created attachment 421377 [details]
Patch

[Tim Horton]

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

[Adam Roben (:aroben)]

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

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 421468 [details]
Patch

[Dean Jackson]

Created attachment 421471 [details]
Patch

[Alexey Proskuryakov]

Is Python code here tested with both Python 2 and Python 3?

[Dean Jackson]

(In reply to Alexey Proskuryakov from comment #13)
> Is Python code here tested with both Python 2 and Python 3?

Yes.

[Dean Jackson]

These failures are because the bot needs a clean build.

[Alexey Proskuryakov]

Is there a way to not require a clean build? We are way conditioned to allowing ourselves that, but it's never correct behavior.

[Dean Jackson]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

If I'm missing something and it is acceptable, please fell encourage to r? again.

[Alexey Proskuryakov]

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

[Adam Roben (:aroben)]

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.

[Dean Jackson]

Created attachment 421677 [details]
Patch

[Dean Jackson]

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.

[Adam Roben (:aroben)]

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.

[Dean Jackson]

-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

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 424572 [details]
WIP - does not compile

[Ian Anderson]

Pull request: https://github.com/WebKit/WebKit/pull/7674

[EWS]

Committed 262032@main (db17513feb3f): <https://commits.webkit.org/262032@main>

Reviewed commits have been landed. Closing PR #7674 and removing active labels.