-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove main.swift from Keystone #24383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
App Name | Jetpack | |
Configuration | Release-Alpha | |
Build Number | 26999 | |
Version | PR #24383 | |
Bundle ID | com.jetpack.alpha | |
Commit | f361df4 | |
Installation URL | 5dq90pnplaeng |
|
App Name | WordPress | |
Configuration | Release-Alpha | |
Build Number | 26999 | |
Version | PR #24383 | |
Bundle ID | org.wordpress.alpha | |
Commit | f361df4 | |
Installation URL | 0p73j4eulc1ug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Keystone will no longer compile because with the removal of main.swift, it changed how Keystone-Swift.h gets generated (Swift APIs available from Objective-C). It now includes only public APIs, and I could not find any way to change it. It is what I would expect considering that the Objective-C umbrella header works the same way – it requires headers included in it to be public.
Yeah, run in similar constraints with WordPressData. See ##24326 and #24348 and I actually have another one coming from the new #24378
Given Keystone is not currently a dependency of the app targets, the fact it does not compile should not be an issue.
0C5C46F42D98343300F2CD55 /* Exceptions for "Classes" folder in "Keystone" target */ = { | ||
isa = PBXFileSystemSynchronizedBuildFileExceptionSet; | ||
membershipExceptions = ( | ||
System/main.swift, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is says in the tin.
isa = PBXFileSystemSynchronizedRootGroup; | ||
path = WordPressAuthenticatorTests; | ||
sourceTree = "<group>"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Glad you committed this project change. I got it yesterday too but kept thinking "I'll open another PR then one for this one." I didn't want it out in a PR that did not work on the project file...
I never looked into it, but apparently the `public` access level for `@objc` is required in a framework setup for those methods to be accessible through the `<framework name>-Swift.h` generated header. We run into the same issue in Keystone, see #24383
I accidentally added it to the target together with other
Classes/
. I plan to move it outside ofClasses/
eventually.Note: Keystone will no longer compile because with the removal of
main.swift
, it changed howKeystone-Swift.h
gets generated (Swift APIs available from Objective-C). It now includes only public APIs, and I could not find any way to change it. It is what I would expect considering that the Objective-C umbrella header works the same way – it requires headers included in it to be public.To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: