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

Provide the ability for use without UIKit #919

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

nickdowell
Copy link
Contributor

@nickdowell nickdowell commented Dec 1, 2020

Goal

Some types of app extensions have such severe memory limits (e.g. 30MB for a file provider extension) that linking UIKit is undesirable.

These changes make it possible to use Bugsnag without linking UIKit.

Design

The approach used here to avoid the use of any UIKit symbols - i.e. Objective-C classes, function names or symbols like NSNotification names.

We can still #import <UIKit/UIKit.h> because Clang's autolinking works on the basis of the symbols referenced by code.

Note: this is contrary to the Quick Help Summary show in Xcode:

Automatically link SDK frameworks that are referenced using #import or #include. This feature requires also enabling support for modules. This build setting only applies to C-family languages.

Even with CLANG_MODULES_AUTOLINK = YES (the default) otool -L shows that removing the use of all UIKit symbols from our code also removes the linker dependency.

We can also still send messages to UIKit objects in the normal way as this does not introduce any link-time dependencies.

Changeset

  • CLANG_MODULES_AUTOLINK has been set to NO in the top-level Bugsnag project - this will cause a build failure if we reintroduce a link-time dependency on UIKit e.g. Undefined symbol: _OBJC_CLASS_$_UIDevice
  • Messages to UIKit classes now use NSClassFromString() to avoid linking against the class symbol
  • Notification names are now hardcoded

Testing

  • Added a unit test case to verify that the hard-coded notification names match those used by the OS.
  • Manually verified that Bugsnag works when used in a file provider extension without UIKit.

@@ -189,27 +190,27 @@ - (instancetype)initWithConfiguration:(BugsnagConfiguration *)config {
[weakSelf setValue:@NO forAppKey:SYSTEMSTATE_APP_IS_IN_FOREGROUND];
}];
#else
[center addObserverForName:UIApplicationWillTerminateNotification object:nil queue:nil usingBlock:^(NSNotification * _Nonnull note) {
[center addObserverForName:@"UIApplicationWillTerminateNotification" object:nil queue:nil usingBlock:^(NSNotification * _Nonnull note) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels dangerous, because any typo won't be detected. Could we instead define our own constants in one place, and then in the tests use a macro to check against the UIKit ones? e.g.

#define ASSERT_NOTIFICATION_NAME(name) XCTAssertEqualObjects(BSGUI ## name, UI ## name)
ASSERT_NOTIFICATION_NAME(ApplicationDidBecomeActiveNotification);
ASSERT_NOTIFICATION_NAME(ApplicationDidEnterBackgroundNotification);

[UIDevice currentDevice].batteryMonitoringEnabled = NO;
[[UIDevice currentDevice] endGeneratingDeviceOrientationNotifications];
[NSClassFromString(@"UIDevice") currentDevice].batteryMonitoringEnabled = NO;
[[NSClassFromString(@"UIDevice") currentDevice] endGeneratingDeviceOrientationNotifications];
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is used in a bunch of places, can we have it as a utility function?

@nickdowell nickdowell changed the title Prevent linking against UIKit Provide the ability for use without UIKit Dec 1, 2020
@nickdowell nickdowell merged commit 3009223 into next Dec 2, 2020
@nickdowell nickdowell deleted the nickdowell/do-not-link-uikit branch December 2, 2020 11:00
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.

3 participants