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

Add "swift" flag to firebaseUserAgent #4448

Merged
merged 8 commits into from Dec 6, 2019
4 changes: 4 additions & 0 deletions Example/Core/Tests/FIRAppTest.m
Expand Up @@ -735,6 +735,10 @@ - (void)testRegisteringNonConformingLibrary {
XCTAssertFalse([[FIRApp firebaseUserAgent] containsString:@"InvalidLibrary`/1.0.0"]);
}

- (void)testSwiftFlagWithNoSwift {
XCTAssertFalse([[FIRApp firebaseUserAgent] containsString:@"swift"]);
}

#pragma mark - Core Diagnostics

- (void)testCoreDiagnosticsLoggedWhenFIRAppIsConfigured {
Expand Down
22 changes: 22 additions & 0 deletions Example/Core/Tests/Swift/FIRAppTests.swift
@@ -0,0 +1,22 @@
// Copyright 2019 Google
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import XCTest
@testable import FirebaseCore

class FIRAppTests: XCTestCase {
func testSwiftFlagWithSwift() {
XCTAssertTrue(FirebaseApp.firebaseUserAgent().contains("swift"))
}
}
@@ -0,0 +1,15 @@
// Copyright 2019 Google
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#import "FIRAppInternal.h"
12 changes: 12 additions & 0 deletions Firebase/Core/FIRApp.m
Expand Up @@ -35,6 +35,8 @@
#import "Private/FIROptionsInternal.h"
#import "Private/FIRVersion.h"

#import <objc/runtime.h>

// The kFIRService strings are only here while transitioning CoreDiagnostics from the Analytics
// pod to a Core dependency. These symbols are not used and should be deleted after the transition.
NSString *const kFIRServiceAdMob;
Expand Down Expand Up @@ -537,7 +539,13 @@ + (NSString *)firebaseUserAgent {
if (sdkVersion) {
[FIRApp registerLibrary:@"apple-sdk" withVersion:sdkVersion];
}

if ([self hasSwiftRuntime]) {
// Add "swift" flag to the `firebaseUserAgent`.
[FIRApp registerLibrary:@"swift" withVersion:@"true"];
}
});

NSMutableArray<NSString *> *libraries =
[[NSMutableArray<NSString *> alloc] initWithCapacity:sLibraryVersions.count];
for (NSString *libraryName in sLibraryVersions) {
Expand All @@ -549,6 +557,10 @@ + (NSString *)firebaseUserAgent {
}
}

+ (BOOL)hasSwiftRuntime {
return objc_lookUpClass("Swift._SwiftObject") != nil;
Copy link
Member

Choose a reason for hiding this comment

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

Great! Is there a resource you could add (even as a PR description) describing where you found this information and why this will work?

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Dec 6, 2019

Choose a reason for hiding this comment

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

Good point! I added comments to the code. The idea originated from an internal discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Will this always be true when running on ios 12.4 and newer, even if there is no swift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not on simulator. I think even if it is, it will be fine. I assume, the main question we would like to answer is "how many applications/devices will have size bump if Swift is added".

But anyways, let me check on a real device just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me the class Swift._SwiftObject is found only when a swift file present in the project on my iPhone iOS 13.2.3

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. So we'll need to cross-reference the Swift flag with the device iOS version to know where Swift causes a size impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks like it.

}

- (void)checkExpectedBundleID {
NSArray *bundles = [FIRBundleUtil relevantBundles];
NSString *expectedBundleID = [self expectedBundleID];
Expand Down
8 changes: 8 additions & 0 deletions FirebaseCore.podspec
Expand Up @@ -48,4 +48,12 @@ Firebase Core includes FIRApp and FIROptions which provide central configuration
unit_tests.dependency 'OCMock'
unit_tests.resources = 'Example/Core/App/GoogleService-Info.plist'
end

s.test_spec 'swift-unit' do |swift_unit_tests|
swift_unit_tests.source_files = 'Example/Core/Tests/Swift/**/*.swift',
'Example/Core/Tests/Swift/**/*.h'
swift_unit_tests.pod_target_xcconfig = {
'SWIFT_OBJC_BRIDGING_HEADER' => '$(PODS_ROOT)/../../Example/Core/Tests/Swift/FirebaseCore-iOS-Unit-swift-unit-Bridging-Header.h'
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be PODS_TARGET_SRCROOT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, that works! Thank you for the suggestion.

}
end
end