Split GoogleUtilities from FirebaseCore #1370

paulb777 · 2018-06-01T20:57:06Z

Create a new FirebaseUtilities pod consisting of Network, Environment, and Logger subspecs - extracted from FirebaseCore. A follow-on PR will add a Swizzler subspec.

paulb777 · 2018-06-01T21:02:03Z

This PR is not yet ready for detailed review. Current status is the six components in the Firebase Xcode project build and pass their unit tests with the new structure. For now, I've left the Utilities unit tests combined with Core.

Next steps:

Verify Firestore and Functions
Verify static libraries
Invent infrastructure to do do pod lib lint when FirebaseCore API's change
Run copybara and integrate closed sourced dependencies

cc: @ryanwilson, @baolocdo, @tejasd

wilhuff · 2018-06-01T21:13:58Z

Does this name (and grouping) at all align with Android? How does this relate to firebase_common over there?

It's getting hard to discuss/reason about dependencies across the platforms.

paulb777 · 2018-06-01T21:49:20Z

@wilhuff I discussed with @bjornick and Android doesn't currently have shared utilities other than what might be in GMSCore.

paulb777 · 2018-06-02T04:52:34Z

Travis is green and the PR is ready for review. I'm planning to wait until after the 5.2.0 merge back to master before merging this one.

baolocdo

Can we call it Utilities or something different from FirebaseUtilities? The point of having a separate module is that it can be depended on by Analytics without explicitly mentioning Firebase.

paulb777 · 2018-06-02T15:34:18Z

@baolocdo Do you have a suggestion for something more specific than Utiliities? Utilities seems too non-descriptive for something being published in the CocoaPods namespace and something that will be used by many different Google products.

What exactly is the reason for not mentioning Firebase? It seems that FirebaseUtilities is the most descriptive since the Firebase team owns the code and it the utilities are primarily used by Firebase. Analytics uses other utility pods already like GoogleToolboxForMac and nanopb.

paulb777 · 2018-06-02T15:55:14Z

@salqadri Please help with the naming of the Utilities pod.

paulb777 · 2018-06-29T14:36:17Z

Now ready for review:

Split out GoogleUtilities from FirebaseCore
Change prefix to GUL (for GoogleUtilitiesLibrary) for moved symbols
Replace usage of GoogleToolboxForMac/NSData+zlib with a subspec in GoogleUtilities
Add ALT_SOURCES and temporary tag mechanisms to enable pre-release lint testing when dependency pods change
Keep FIRLogger as a wrapper around GULLogger with Firebase specific functionality and to make transition easier
Split Reachability into its own subspec for FirebaseMessaging
Replace GoogleToolboxForMac GTMDevLog usage in Messaging unit tests with NSLog
Use Firebase/CoreOnly instead of Firebase/Core so that unit tests build and run without FirebaseAnalytics

Next step is integration with closed source dependencies

wilhuff

I'm going to stop reviewing. Github is going to break. I can already feel it slowing down while I'm typing.

This really should have been three PRs:

Define GoogleUtilities, forking Core stuff but otherwise not changing it.
Integrate GoogleUtilities into the various builds
Migrate usage of Core APIs to GoogleUtilities as needed

If you have the stomach for it we should rebase squash this state and then split it up.

wilhuff · 2018-06-29T14:49:51Z

Example/Core/Tests/FIRAppEnvironmentUtilTest.m


 @property(nonatomic) id processInfoMock;

 @end

-@implementation FIRAppEnvironmentUtilTest
+@implementation GULAppEnvironmentUtilTest


Shouldn't this test move along with the sources?

This comment applies both here and to the other tests in Core in this PR.

Yep. The tests will be factored out in a subsequent PR.

wilhuff · 2018-06-29T14:52:14Z

Example/Core/Tests/FIRLoggerTest.m

@@ -18,26 +18,27 @@
 #import "FIRTestCase.h"

 #import <FirebaseCore/FIRLogger.h>
+#import <GoogleUtilities/GULLogger.h>


It doesn't seem like anything should import both FIRLogger and GULLogger.

Agreed. Adding a TODO comment to split FIRLoggerTest from GULLoggerTest.

wilhuff · 2018-06-29T14:55:07Z

Example/Podfile


 target 'Core_Example_iOS' do
  platform :ios, '8.0'

  # The next line is the forcing function for the Firebase pod. The Firebase
  # version's subspecs should depend on the component versions in their
  # corresponding podspec's.
-  pod 'Firebase/Core', '5.3.0'
+  pod 'Firebase/CoreOnly', '5.3.0'


This should be "CoreCore" :-P.

:) This is the change that excludes FirebaseAnalytics and its no longer valid current dependencies from the unit tests.

wilhuff · 2018-06-29T14:57:13Z

Firebase/Core/FIRLogger.m

-#import "FIRLoggerLevel.h"
-#import "Private/FIRVersion.h"
-#import "third_party/FIRAppEnvironmentUtil.h"
+#import <GoogleUtilities/GULAppEnvironmentUtil.h>


nit: import order should be

self header

system headers

other headers

Actually all deleted here and reordered in GULLogger.m

wilhuff · 2018-06-29T15:06:25Z

Firebase/Core/FIRLogger.m

-    sMessageCodeRegex =
-        [NSRegularExpression regularExpressionWithPattern:kMessageCodePattern options:0 error:NULL];
-#endif
+    GULLoggerInitializeASL([arguments containsObject:kFIRDisableDebugModeApplicationArgument],


Was there an API review for this?

If not:

Let's drop "ASL", which is deprecated. We should eventually move to os_log.

Is there any way we can avoid passing three booleans to this? It's a terrible interface.

Is there a way to translate debug modes into a maximum log level instead so that GULLogger doesn't even have to be aware of this wart?

In particular it seems like if argument processing is to be done here, then we should completely figure out if we want debug mode from the arguments locally. Passing two booleans that seem like logical inverses of each other seems wrong.

+1 to moving to os_log but it's only available on iOS 10+, so we'll have to keep ASL around until we drop support for iOS 8 and 9 (will be a while).

I moved the UserDefaults stuff back to FIRLogger and reduced to two booleans. Overall, this really needs an overall rewrite with a breaking change.

wilhuff · 2018-06-29T15:27:18Z

Firebase/Utilities/Logger/GULLogger.m

+#include <unistd.h>
+
+/// Arguments passed on launch.
+NSString *const kGULDisableDebugModeApplicationArgument = @"-GULDebugDisabled";


wilhuff · 2018-06-29T15:30:11Z

Firebase/Utilities/Logger/GULLogger.m

+NSString *const kGULLoggerForceSDTERRApplicationArgument = @"-GULLoggerForceSTDERR";
+
+/// Key for the debug mode bit in NSUserDefaults.
+NSString *const kGULPersistedDebugModeKey = @"/google/utilities/debug_mode";


This implementation is now split-brained. Why are we handling command-line arguments for debug mode in FIRLogger, but NSUserDefaults here?

Either insulate GULLogger from knowing about this gunk, or just put it all in here. This combined approach is even harder to reason about than the old system.

wilhuff · 2018-06-29T15:32:20Z

Firebase/Utilities/Logger/GULLogger.m

+const char *kGULLoggerASLClientFacilityName = "com.google.utilities.app.logger";
+
+/// Keys for the number of errors and warnings logged.
+NSString *const kGULLoggerErrorCountKey = @"/google/utilities/count_of_errors_logged";


This is declared and defined but appears otherwise unused. Can we just drop it?

wilhuff · 2018-06-29T15:33:06Z

Firebase/Utilities/Logger/GULLogger.m

+NSString *const kGULPersistedDebugModeKey = @"/google/utilities/debug_mode";
+
+/// ASL client facility name used by GULLogger.
+const char *kGULLoggerASLClientFacilityName = "com.google.utilities.app.logger";


This no longer really has anything to do with "app". "com.gogle.utilities.logger"?

wilhuff · 2018-06-29T15:37:06Z

Firebase/Utilities/Logger/GULLogger.m

+}
+
+void GULSetAnalyticsDebugMode(BOOL analyticsDebugMode) {
+  GULLoggerInitializeASL(NO, NO, NO);


This is now functionally different than what came before.

Previously if your first call was FIRSetAnalyticsDebugMode we would initialize FIRLogger, including evaluating the command-line arguments. This now disregards the command-line arguments.

wilhuff · 2018-06-29T15:59:35Z

At a high level I think we should try really hard to avoid putting any kind of "debug mode" in GULLogger. As implemented it's broken.

From my reading of the initialization code, debug mode has the effect of calling

asl_set_filter(sGULLoggerClient, ASL_FILTER_MASK_UPTO(ASL_LEVEL_DEBUG));

But it also sets sGULAnalyticsDebugMode and sGULLoggerDebugMode.

But GULLogBasic now has a "force" parameter and does not even concern itself with analytics debug mode.

GULIsLoggableLevel allows you to query by whether or not it's an analytics component but GULLogBasic doesn't. How is this supposed to work in practice?

I propose that analytics debug mode be pushed up into FIRLogger completely.

I'd also propose that we try to eliminate logger debug mode. Why isn't this equivalent to GULSetLoggerLevel to the max?

I also suggest moving the NSUserDefaults gunk up into FIRLogger as well.

ryanwilson · 2018-06-29T15:22:33Z

Example/Auth/Sample/ApplicationDelegate.m

@@ -17,7 +17,7 @@
 #import "ApplicationDelegate.h"

 #import <FirebaseCore/FIRApp.h>
-#import <FirebaseCore/FIRLogger.h>
+#import <GoogleUtilities/FIRLogger.h>


Shouldn't this stay as FirebaseCore?

ryanwilson · 2018-06-29T15:28:57Z

Example/Core/Tests/FIRAppEnvironmentUtilTest.m

@@ -15,17 +15,17 @@
 #import <Foundation/Foundation.h>
 #import <XCTest/XCTest.h>

-#import <FirebaseCore/FIRAppEnvironmentUtil.h>
+#import <GoogleUtilities/GULAppEnvironmentUtil.h>


Should this eventually just be moved to a GoogleUtilities test suite?

Yes. The tests will be factored out in a subsequent PR.

ryanwilson · 2018-06-29T17:21:47Z

Firebase/Utilities/Logger/GULLogger.m

@@ -0,0 +1,273 @@
+// Copyright 2017 Google


I realize this was copied over, but does this count as new code and an updated copyright?

My interpretation was that since it was copied that it should keep the copyright.

ryanwilson · 2018-06-29T17:27:15Z

Firebase/Auth/Source/FIRAuthAPNSTokenManager.m

@@ -137,7 +137,7 @@ + (BOOL)isProductionApp {

  NSError *error = nil;

-  Class envClass = NSClassFromString(@"FIRAppEnvironmentUtil");
+  Class envClass = NSClassFromString(@"GULAppEnvironmentUtil");


Since Auth relies on GoogleUtilities/GULAppEnvironmentUtil already, can we remove the string based selectors here?

Yes - done.

ryanwilson · 2018-06-29T17:32:32Z

Firebase/Core/FIRLogger.m

-    sMessageCodeRegex =
-        [NSRegularExpression regularExpressionWithPattern:kMessageCodePattern options:0 error:NULL];
-#endif
+    GULLoggerInitializeASL([arguments containsObject:kFIRDisableDebugModeApplicationArgument],


+1 to moving to os_log but it's only available on iOS 10+, so we'll have to keep ASL around until we drop support for iOS 8 and 9 (will be a while).

ryanwilson · 2018-06-29T17:50:01Z

FirebaseCore.podspec

@@ -25,13 +27,12 @@ Firebase Core includes FIRApp and FIROptions which provide central configuration
  s.prefix_header_file = false

  s.source_files = 'Firebase/Core/**/*.[mh]'
-  s.public_header_files = 'Firebase/Core/Public/*.h', 'Firebase/Core/Private/*.h'
-  s.private_header_files = 'Firebase/Core/Private/*.h', 'Firebase/Core/third_party/*.h'
+  s.public_header_files = 'Firebase/Core/Public/*.h'


What was the reason for having Private/ here as well again?

To CocoaPods, "private" means publicly available but not included by default by importing the module - only by explicitly importing the header.

ryanwilson · 2018-06-29T17:51:55Z

GoogleUtilities.podspec

+  s.summary          = 'Firebase Utilities for iOS (plus community support for macOS and tvOS)'
+
+  s.description      = <<-DESC
+Firebase Utilities including Network, Environment, Logger, and Swizzling.


Do we want to have a note here saying not for public use, or "Internal Google Utilities for iOS including Network, Environment, Logger, and Swizzling"

ryanwilson · 2018-06-29T17:52:08Z

GoogleUtilities.podspec

+Pod::Spec.new do |s|
+  s.name             = 'GoogleUtilities'
+  s.version          = '5.0.4'
+  s.summary          = 'Firebase Utilities for iOS (plus community support for macOS and tvOS)'


Nit: s/Firebase/Google

ryanwilson

LGTM with some small nits!

ryanwilson · 2018-07-05T15:00:57Z

Example/Firebase.xcodeproj/project.pbxproj

@@ -4887,6 +4887,10 @@
 				INFOPLIST_FILE = "$(SRCROOT)/Core/App/macOS/Core-Info.plist";
 				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/../Frameworks";
 				MTL_ENABLE_DEBUG_INFO = YES;
+				OTHER_LDFLAGS = (
+					"$(inherited)",
+					"-ObjC",


Was this missed before, or why does it need to be added now?

The new pod structure exposed macOS test failures from categories not being linked in. There's no change to iOS since their tests are linked with -all_load.

ryanwilson · 2018-07-05T15:21:46Z

Firebase/Core/FIRLogger.m

-#import "FIRLoggerLevel.h"
+#import <FirebaseCore/FIRLoggerLevel.h>
+#import <GoogleUtilities/GULAppEnvironmentUtil.h>
+#import <GoogleUtilities/GULLogger.h>


Nit: Should have vertical whitespace between the framework imports and file import.

ryanwilson · 2018-07-05T15:40:19Z

Firebase/Core/FIRLogger.m

-      aslOptions = 0;
-    }
-#endif  // TARGET_OS_SIMULATOR
+    // Register Firebase Version with GULLogger


Nit: punctuation.

ryanwilson · 2018-07-05T15:48:48Z

Firebase/Utilities/Network/Private/GULNetworkLoggerProtocol.h

-  kFIRNetworkLogLevelInfo = FIRLoggerLevelInfo,
-  kFIRNetworkLogLevelDebug = FIRLoggerLevelDebug,
+#import <GoogleUtilities/GULLoggerLevel.h>
+#import "GULNetworkMessageCode.h"


Nit: Vertical whitespace between framework and file imports.

ryanwilson · 2018-07-05T15:54:19Z

GoogleUtilities.podspec

+other Google CocoaPods. They're not intended for direct public usage.
+                       DESC
+
+  s.homepage         = 'https://firebase.google.com'


Should this be a more general homepage?

ryanwilson · 2018-07-05T15:54:56Z

GoogleUtilities.podspec

+#    :tag => 'Utilities-' + s.version.to_s
+    :tag => 'pre-5.3-' + s.version.to_s
+  }
+  s.social_media_url = 'https://twitter.com/Firebase'


Same as above - maybe this should be @googledevs?

ryanwilson

Thanks! LGTM, please make sure @wilhuff is okay with this as well since he previously requested changes.

paulb777 · 2018-07-06T16:34:03Z

@ryanwilson Thanks for the review! I'm going to merge now to unblock other tasks. I believe I've addressed most, if not all, of @wilhuff's requests. If there are more, I'll address them in a follow-up PR.

wilhuff

I still have plenty of reservations about this code.

I wish this had gone through API review because this is a new public API that isn't very good.

For example, it's still not clear to me why we need forceLog. Why can't setting analytics debug mode in FIRLogger translate into a SetLoggerLevel down here?

I don't think this should have been submitted because now we have to release it (unless you revert it, which I suggest you do).

wilhuff · 2018-07-07T22:51:47Z

Firebase/Utilities/Logger/Private/GULLogger.h

+/**
+ * Logs a message to the Xcode console and the device log. If running from AppStore, will
+ * not log any messages with a level higher than GULLoggerLevelNotice to avoid log spamming.
+ * (required) log level (one of the GULLoggerLevel enum values).


This is not the way to format documentation. It ends up looking like a mess of run-on text in the Xcode help.

See examples for how to do this correctly in http://google.github.io/styleguide/objcguide.html.

In particular, use @param variableName to introduce each parameter and @return to describe what's being returned.

wilhuff · 2018-07-07T22:52:07Z

Firebase/Utilities/Logger/Private/GULLogger.h

+ * (required) overrideSTDERR Override the aslOptions to ASL_OPT_STDERR.
+ * (required) forceDebugMode Force the asl level to ASL_LEVEL_DEBUG.
+ */
+extern void GULLoggerInitializeASL(BOOL overrideSTDERR, BOOL forceDebugMode);


We should drop the "ASL" in this name and references to specific ASL options here.

wilhuff · 2018-07-07T23:02:16Z

Firebase/Utilities/Logger/Private/GULLogger.h

+                        NSString *message,
+// On 64-bit simulators, va_list is not a pointer, so cannot be marked nullable
+// See: http://stackoverflow.com/q/29095469
+#if __LP64__ && TARGET_OS_SIMULATOR || TARGET_OS_OSX


This #if makes no sense. There's no useful way to write code that passes NULL here since code we write must be testable on simulators (and 64-bit is our most common width).

In FIRLogger this was a pre-existing wart that we couldn't change, but we should get rid of this here before we publish it.

Additionally, this just passed directly to [NSString stringWithFormat:message arguments:args_ptr] and that function has no similar decorations.

Meanwhile, if you're calling this function and don't want to pass any arguments you can instantiate a va_list, not initialize it, and pass it here.

Finally note that message is always a format string, even if you pass no arguments. For example, to get a literal % in the output you still need to escape it (i.e. pass @"%%").

Also a va_list is not necessarily a pointer, so we should call this "args" not "args_ptr".

wilhuff · 2018-07-07T23:04:16Z

Firebase/Utilities/Logger/Private/GULLogger.h

+ * not log any messages with a level higher than GULLoggerLevelNotice to avoid log spamming.
+ * (required) log level (one of the GULLoggerLevel enum values).
+ * (required) service name of type GULLoggerService.
+ * (required) message code starting with "I-" which means iOS, followed by a capitalized


This is missing a description of forceLog.

paulb777 · 2018-07-09T14:06:08Z

Thanks @wilhuff. I'll make another PR to address these in the next few days.

Like FIRLogger, the only part of the GULLogger API that is technically public is GULLoggerLevel.h. See https://github.com/firebase/firebase-ios-sdk/blob/master/GoogleUtilities.podspec#L38. However, the GoogleUtilities pod documents itself as to not supporting non-Google usage. We can incrementally improve everything else without impacting the public API.

I don't think we should revert since this PR is an incremental step forward. It unblocks other projects that need to use GoogleUtilities. It provides size improvements for many Firebase configurations. And is a step towards cleaning up FIRLogger.

wilhuff · 2018-07-09T15:41:34Z

Ah, interesting! I saw these listed in the public headers and thought this was a public API.

https://github.com/firebase/firebase-ios-sdk/blob/master/GoogleUtilities.podspec#L37

In that case please don't revert, but yes, let's address some of these rough edges as we get time.

googlebot added the cla: yes label Jun 1, 2018

paulb777 added this to the M28 milestone Jun 1, 2018

paulb777 force-pushed the pb-utilities branch 3 times, most recently from 916d170 to a5ccdea Compare June 2, 2018 02:11

paulb777 changed the title ~~WIP - Split FirebaseUtilities from FirebaseCore~~ Split FirebaseUtilities from FirebaseCore Jun 2, 2018

baolocdo reviewed Jun 2, 2018

View reviewed changes

paulb777 modified the milestones: 5.3.0, M29 Jun 13, 2018

paulb777 force-pushed the pb-utilities branch from 9318d0c to 5ceab1b Compare June 21, 2018 16:18

paulb777 modified the milestones: M29, M30 Jun 25, 2018

morganchen12 added the api: core label Jun 25, 2018

paulb777 force-pushed the pb-utilities branch 3 times, most recently from b001565 to bf3a795 Compare June 28, 2018 21:29

paulb777 requested review from ryanwilson, wilhuff and charlotteliang June 29, 2018 14:36

paulb777 changed the title ~~Split FirebaseUtilities from FirebaseCore~~ Split GoogleUtilities from FirebaseCore Jun 29, 2018

wilhuff suggested changes Jun 29, 2018

View reviewed changes

ryanwilson reviewed Jun 29, 2018

View reviewed changes

paulb777 added 7 commits June 30, 2018 09:25

fix bad merge

e1555da

macOS and tvOS Core tests now need to force category loading

ce59ba2

Option on app not tests

6d49931

clean up

e022441

podspec cleanup

970f8c3

address review feedback

739e6c0

Fix bad rebase merge

a123f6e

paulb777 force-pushed the pb-utilities branch from 3979370 to a123f6e Compare June 30, 2018 16:31

paulb777 added 11 commits June 30, 2018 09:35

Update GoogleUtilities version

9c8f282

style

74d74bf

podspec consistency

4bc7166

Add missing Security framework for SecTrustEvaluate API call

640550b

Consistently use module reference for public/private headers

df4c242

delete unnecessary import

bce3438

consistency

aa2f13f

Add library dependency

1592bf9

GoogleUtilities does not have to be static - let Podfile determine

9c26db1

Fix non-modular import error

988aee0

fix analytics tests

c2e9973

paulb777 mentioned this pull request Jul 4, 2018

ThreadSanitizer detects data race when initializing FirebaseApp instance #1293

Closed

ryanwilson reviewed Jul 5, 2018

View reviewed changes

review updates

aeb878b

ryanwilson approved these changes Jul 6, 2018

View reviewed changes

paulb777 merged commit 98b6eef into master Jul 6, 2018

paulb777 deleted the pb-utilities branch July 6, 2018 16:37

wilhuff reviewed Jul 7, 2018

View reviewed changes

firebase locked and limited conversation to collaborators Nov 1, 2019

Split GoogleUtilities from FirebaseCore #1370

Split GoogleUtilities from FirebaseCore #1370

Uh oh!

Conversation

paulb777 commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulb777 commented Jun 1, 2018

Uh oh!

wilhuff commented Jun 1, 2018

Uh oh!

paulb777 commented Jun 1, 2018

Uh oh!

paulb777 commented Jun 2, 2018

Uh oh!

baolocdo left a comment

Choose a reason for hiding this comment

Uh oh!

paulb777 commented Jun 2, 2018

Uh oh!

paulb777 commented Jun 2, 2018

Uh oh!

paulb777 commented Jun 29, 2018

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulb777 commented Jun 1, 2018 •

edited

Loading

wilhuff commented Jun 29, 2018 •

edited

Loading