Expose library version, move it out of options #588

gsoltis · 2017-12-21T23:06:06Z

Expose library version as a constant. Remove it from FIROptions, since it is not per-options instance, and it doesn't make sense to set it.

wilhuff · 2017-12-22T01:35:43Z

Firebase/Core/Public/FIRVersion.h

+#import <Foundation/Foundation.h>
+
+// The version of the Firebase SDK.
+extern NSString *const kFIRLibraryVersionID;


I see your declaration of kFIRLibraryVersionID here but I don't see a definition. Are you missing a file?

No, it turns out it did already exist. It's in FIROptions.m[0]. It is possible that we would like a new home for it though. I'm hoping someone from the core team can give some guidance there.

This constant is also not currently tied to the one actually used to build the sdk by anything other than policy. That being said, it does appear to be current, so that might be good enough for now.

0:

firebase-ios-sdk/Firebase/Core/FIROptions.m

Line 42 in 9ad65fe

NSString *const kFIRLibraryVersionID =

paulb777 · 2017-12-22T16:52:02Z

Can you explain more about the reasoning for this change? Why do you care about the FirebaseCore version?

If anything, I would like to see FirebaseCore adopt the FirebaseFirestore versioning approach using build time compile flags that don't require source changes.

wilhuff · 2017-12-22T16:56:14Z

The issue here is that we're supposed to be reporting the Firebase-wide version to the backend. We also want to include the Firebase version in our log lines. The Firestore version is largely an implementation detail that nobody really cares about. Logging the Firebase-wide version makes it possible to go back to release notes, see if you're up-to-date, etc.

This change makes that version accessible to us.

gsoltis · 2017-12-22T17:06:48Z

Indeed, I think an even better approach would be to make available entire-SDK version that is used to build the whole thing. But, I couldn't see a way to thread it through easily. At the same time, I noticed that this constant is supposed to be kept in sync, so I figured it might work for now.

It also seemed slightly misplaced on FIROptions and with a setter. Different options instances would never have different sdk versions, so I think it is better as just a simple constant. And obviously we would not ever want to programmatically set the version.

paulb777 · 2017-12-22T17:07:07Z

OK, but the FIROptions version isn't the Firebase-wide version - It's the FirebaseCore version.

A Firebase-wide version would need to be implemented in Firebase.h since that is the only file that is part of overall Firebase.

I'd like to see us merge FirebaseCore into Firebase, but that is not currently the implementation.

gsoltis · 2017-12-22T17:13:52Z

I see. In that case, is there a way to get the correct version into Firebase.h?

gsoltis · 2017-12-22T20:38:06Z

Ok, please take another look. I've added both Firebase_VERSION and FIRCore_VERSION to the podspec, as well as FIRVersion.h. If this is accepted, we'll need to add them to the internal build as well.

I've reverted the changes to FIROptions and test, although it seems like those might be nice as an independent change?

This change also includes adding the version to FIRLogger output. I have no strong preferences on the format of the log line, so if anyone would prefer something different, I'm happy to change it.

Once this is accepted, I will update Firestore to send the proper version in the GRPC header.

paulb777

Thanks! Generally looks good to me.

A few nits and an API process question :

paulb777 · 2017-12-22T21:43:40Z

Firebase/Core/Private/FIRVersion.m

+        (const unsigned char *const)STR(Firebase_VERSION);
+
+const unsigned char *const FirebaseCoreVersionString =
+        (const unsigned char *const)STR(FIRCore_VERSION);


Add newline to end of file

paulb777 · 2017-12-22T21:48:37Z

Firebase/Core/Private/FIRVersion.m

@@ -0,0 +1,39 @@
+/*


Current convention is Private directory is only for headers. FIRVersion.m should be in Firebase/Core/

Ah, got it. Done.

paulb777 · 2017-12-22T21:50:21Z

Firebase/Core/Public/FIRVersion.h

@@ -0,0 +1,23 @@
+/*


Putting FIRVersion.h may require an API review. @ryanwilson ?

I moved it to Private. It probably would make sense to move it to Public/ at some point though.

paulb777 · 2017-12-22T21:52:17Z

FirebaseCore.podspec

  s.framework = 'SystemConfiguration'
  s.dependency 'GoogleToolboxForMac/NSData+zlib', '~> 2.1'
+  s.pod_target_xcconfig = {
+    'OTHER_CFLAGS' => '-DFIRCore_VERSION=' + s.version.to_s + ' -DFirebase_VERSION=0.0.1-dev'


Firebase version should be 4.8.0. Part of the iCore release process is synchronizing the open source development build with the official last Firebase public release with the glue in Example/Podfile and Firestore/ExamplePodfile.

paulb777 · 2017-12-22T21:53:55Z

Firebase/Core/Private/FIRVersion.m

+const unsigned char *const FirebaseVersionString =
+        (const unsigned char *const)STR(Firebase_VERSION);
+
+const unsigned char *const FirebaseCoreVersionString =


Is the idea to modify the FIROptions FirebaseCore option handling in a separate PR?

paulb777 · 2017-12-22T22:02:04Z

If we want to defer the API question, FIRVersion.h could be added to Private instead.

paulb777

LGTM.

Why not a PR to master instead of firestore-api-changes?

googlebot · 2017-12-23T01:36:03Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2017-12-23T01:40:11Z

CLAs look good, thanks!

gsoltis · 2017-12-23T01:45:10Z

Ok, I think I've managed to rebase on top of master.

ryanwilson

LGTM with one minor question for Paul.

ryanwilson · 2017-12-28T18:47:22Z

Firebase/Core/FIRVersion.m

+
+
+#ifndef Firebase_VERSION
+#error "Firebase_VERSION is not defined: add -DFirebase_VERSION=... to the build invocation"


@paulb777 Will this cause trouble for future build systems that don't use CocoaPods?

@ryanwilson This should be fine. We already require any build system to add -D versions for multiple Firebase components. This ifndef is a great way to catch it at build time.

Separately, we do often use #ifdef COCOAPODS in a binary way that will have to be reviewed with a third build system.

* slight cleanup * Use -D defines for versions * Undo FIROptionsTest change * Drop failed macro attempt * Add correct version to podspec * Add newline * Shuffle files around * Bring back log change * Fix change * Fix space

gsoltis assigned ryanwilson Dec 21, 2017

gsoltis requested a review from ryanwilson December 21, 2017 23:06

googlebot added the cla: yes label Dec 21, 2017

wilhuff reviewed Dec 22, 2017

View reviewed changes

gsoltis assigned paulb777 Dec 22, 2017

gsoltis requested a review from paulb777 December 22, 2017 20:38

paulb777 requested changes Dec 22, 2017

View reviewed changes

paulb777 approved these changes Dec 23, 2017

View reviewed changes

gsoltis changed the base branch from firestore-api-changes to master December 23, 2017 00:38

gsoltis changed the base branch from master to firestore-api-changes December 23, 2017 00:39

Greg Soltis added 7 commits December 22, 2017 17:30

slight cleanup

b2ff312

Use -D defines for versions

6ef08bf

Undo FIROptionsTest change

35af49c

Drop failed macro attempt

5bd9990

Add correct version to podspec

8b2db47

Add newline

2d223e2

Shuffle files around

82a9f33

gsoltis force-pushed the expose_library_version branch from 6495eff to 82a9f33 Compare December 23, 2017 01:35

googlebot added cla: no and removed cla: yes labels Dec 23, 2017

gsoltis changed the base branch from firestore-api-changes to master December 23, 2017 01:36

Bring back log change

dd396e9

googlebot added cla: yes and removed cla: no labels Dec 23, 2017

Greg Soltis added 2 commits December 22, 2017 17:41

Fix change

6527d93

Fix space

583235a

ryanwilson reviewed Dec 28, 2017

View reviewed changes

ryanwilson approved these changes Dec 28, 2017

View reviewed changes

gsoltis merged commit 412e759 into master Jan 2, 2018

gsoltis deleted the expose_library_version branch January 2, 2018 01:43

firebase locked and limited conversation to collaborators Nov 9, 2019



		#ifndef Firebase_VERSION
		#error "Firebase_VERSION is not defined: add -DFirebase_VERSION=... to the build invocation"

Expose library version, move it out of options #588

Expose library version, move it out of options #588

Uh oh!

Conversation

gsoltis commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulb777 commented Dec 22, 2017

Uh oh!

wilhuff commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsoltis commented Dec 22, 2017

Uh oh!

paulb777 commented Dec 22, 2017

Uh oh!

gsoltis commented Dec 22, 2017

Uh oh!

gsoltis commented Dec 22, 2017

Uh oh!

paulb777 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!

paulb777 commented Dec 22, 2017

Uh oh!

paulb777 left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Dec 23, 2017

Uh oh!

googlebot commented Dec 23, 2017

Uh oh!

gsoltis commented Dec 23, 2017

Uh oh!

ryanwilson 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!

Uh oh!

wilhuff commented Dec 22, 2017 •

edited

Loading