Skip to content
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

Explicit extension safety flag #866

Merged
merged 5 commits into from
Jan 5, 2023

Conversation

lickel
Copy link
Contributor

@lickel lickel commented Jan 5, 2023

This explicitly declares that ReactiveSwift only uses extension safe APIs (and/or APIs in an extension-safe way).
The code was already extension safe; this just forward declares it and enforces it at compile time.

If I can enforce this here, then I can enforce it in Workflow which is heavily dependent on this library.
Without this explicit conformance, it is not straightforward to write CI validation that these libraries remain extension safe.

There is zero harm in explicit conformance. Non-extension applications can call into extension-safe libraries without any restrictions.

Checklist

  • Updated CHANGELOG.md.

@@ -1512,6 +1512,7 @@
isa = XCBuildConfiguration;
baseConfigurationReference = D047262919E49FE8006002AA /* Debug.xcconfig */;
buildSettings = {
APPLICATION_EXTENSION_API_ONLY = YES;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which target did you set this on? It's already YES on each of the 4 targets.
Screenshot 2023-01-05 at 10 13 16

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I didn't have the XCConfig submodule setup correctly. I've reverted this.

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@
*Please add new entries at the top.*

1. Bumped deployment target to iOS 11, tvOS 11, watchOS 4, macOS 10.13, per Xcode 14 warnings
1. Explicitly declare `APPLICATION_EXTENSION_API_ONLY`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, maybe mention that this is specific to the CocoaPods installation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@lickel
Copy link
Contributor Author

lickel commented Jan 5, 2023

@NachoSoto once this lands, would you be willing to publish a new release?

That way I would be unblocked in making/landing the change to that other repo I mentioned.
And, thanks!

@NachoSoto NachoSoto merged commit 618b802 into ReactiveCocoa:master Jan 5, 2023
@NachoSoto
Copy link
Member

I don't think I have privileges to create a CocoaPods release, sorry. But hopefully somebody else can help with that.

@lickel
Copy link
Contributor Author

lickel commented Jan 5, 2023

CC @mluisbrown @andersio re: the possibility of publishing a new release

@lickel lickel deleted the extension-safety branch January 5, 2023 19:38
@mluisbrown
Copy link
Contributor

I can create a release. I will do it tomorrow 👍

@mluisbrown
Copy link
Contributor

mluisbrown commented Jan 6, 2023

@lickel 7.1.1 has been released. I did notice (too late to do anything about it) that the Podspec still has iOS 10 as the minimum deployment target, so you will still get warnings about that.

@lickel
Copy link
Contributor Author

lickel commented Jan 6, 2023

Thanks! And ugh whoops sorry about missing that one.

@lickel lickel mentioned this pull request Jan 6, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants