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

Open Sourcing FirebaseCrashlytics #4610

Merged
merged 2 commits into from
Jan 8, 2020
Merged

Open Sourcing FirebaseCrashlytics #4610

merged 2 commits into from
Jan 8, 2020

Conversation

samedson
Copy link
Contributor

@samedson samedson commented Jan 7, 2020

Co-authored-by: Jason Hu jasonhu@google.com
Co-authored-by: Bryan Klimt klimt@google.com

Fixes #4316

@ryanwilson ryanwilson changed the title Crashlytics 4.0.0-beta.1 Staging Branch (#599) Open Sourcing FirebaseCrashlytics Jan 7, 2020
@samedson samedson force-pushed the crashlytics-staging branch 4 times, most recently from 2dfd9ce to d4eae94 Compare January 7, 2020 18:29
@google-oss-bot google-oss-bot added api: core zip-builder Tools related to building the zip file. labels Jan 7, 2020
@ryanwilson
Copy link
Member

Please fix travis/GA: ZipBuilder/Sources/ZipBuilder/ZipBuilder.swift needs formatting.

@samedson samedson force-pushed the crashlytics-staging branch from e6ebb7e to a71f8e0 Compare January 7, 2020 21:35
Co-authored-by: Jason Hu <jasonhu@google.com>
Co-authored-by: Bryan Klimt <klimt@google.com>
@samedson samedson force-pushed the crashlytics-staging branch from edf70fe to f27857f Compare January 7, 2020 22:05
Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

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

🎉

@paulb777 paulb777 added this to the M62 - 6.15.0 milestone Jan 8, 2020
@igor-makarov
Copy link
Contributor

Hi, it's very interesting to look at the code! I wanted to ask, is there a reason why upload-symbols tool hasn't been open-sourced as well? Or am I missing something?
Also hi @paulb777 👋🏻

@igor-makarov
Copy link
Contributor

In addition to my previous remark, I've also tried integrating FirebaseCrashlytics using CocoaPods and I'm getting build errors on bad/missing import statements. I've managed to fix all of them and I'm attaching a patch:

diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSReportManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSReportManager.m
index bba828991938f6be5a523fe15d7693de9f658c3a..6b38f2646120c472f9e74aad358f3dbecc25a7ae 100644
--- a/Crashlytics/Crashlytics/Controllers/FIRCLSReportManager.m
+++ b/Crashlytics/Crashlytics/Controllers/FIRCLSReportManager.m
@@ -14,7 +14,7 @@
 
 #include <stdatomic.h>
 
-#import "FBLPromises.h"
+#import <FBLPromises/FBLPromises.h>
 
 #import "FIRCLSApplication.h"
 #import "FIRCLSDataCollectionArbiter.h"
diff --git a/Crashlytics/Crashlytics/Models/FIRCLSSettings.h b/Crashlytics/Crashlytics/Models/FIRCLSSettings.h
index c83b07104d6f071f0f79e3a823b7a0ad3abb4c32..513ae2fbfde545177d68ef49bd2863ee4327d41c 100644
--- a/Crashlytics/Crashlytics/Models/FIRCLSSettings.h
+++ b/Crashlytics/Crashlytics/Models/FIRCLSSettings.h
@@ -14,7 +14,7 @@
 
 #import <Foundation/Foundation.h>
 
-#import "FBLPromise.h"
+#import <FBLPromises/FBLPromises.h>
 
 @class FIRCLSApplicationIdentifierModel;
 @class FIRCLSFileManager;
diff --git a/Crashlytics/Crashlytics/Settings/FIRCLSSettingsOnboardingManager.h b/Crashlytics/Crashlytics/Settings/FIRCLSSettingsOnboardingManager.h
index 8c818c9feb26e6742cce1c2f6f5a24fe1f7fd735..b792cb8cfa2b482d1e8d3574484f2c66976ec869 100644
--- a/Crashlytics/Crashlytics/Settings/FIRCLSSettingsOnboardingManager.h
+++ b/Crashlytics/Crashlytics/Settings/FIRCLSSettingsOnboardingManager.h
@@ -14,7 +14,7 @@
 
 #import <Foundation/Foundation.h>
 
-#import "FBLPromise.h"
+#import <FBLPromises/FBLPromises.h>
 
 @class FIRCLSApplicationIdentifierModel;
 @class FIRCLSDataCollectionToken;
diff --git a/Crashlytics/Crashlytics/Models/FIRCLSSettings.m b/Crashlytics/Crashlytics/Models/FIRCLSSettings.m
index 41438fdb8ae439593c170e19084cbb607dc90ce7..8b71c1a1622f08e403072e57a142c670bcbe7ad5 100644
--- a/Crashlytics/Crashlytics/Models/FIRCLSSettings.m
+++ b/Crashlytics/Crashlytics/Models/FIRCLSSettings.m
@@ -14,7 +14,7 @@
 
 #import "FIRCLSSettings.h"
 
-#import "FBLPromise.h"
+#import <FBLPromises/FBLPromises.h>
 
 #import "FIRCLSApplicationIdentifierModel.h"
 #import "FIRCLSConstants.h"
diff --git a/Crashlytics/Crashlytics/Handlers/FIRCLSSignal.h b/Crashlytics/Crashlytics/Handlers/FIRCLSSignal.h
index 64ce78e285d6844518e51a18d5f5b7d1b5894b0a..3b6b1b4e2c6dbff7a64b952b869c99e21146a15a 100644
--- a/Crashlytics/Crashlytics/Handlers/FIRCLSSignal.h
+++ b/Crashlytics/Crashlytics/Handlers/FIRCLSSignal.h
@@ -14,6 +14,7 @@
 
 #pragma once
 
+#include "FIRCLSFeatures.h"
 #include "FIRCLSFile.h"
 
 #include <signal.h>
diff --git a/Crashlytics/Crashlytics/DataCollection/FIRCLSDataCollectionArbiter.m b/Crashlytics/Crashlytics/DataCollection/FIRCLSDataCollectionArbiter.m
index b8af47f173643bdfa4e2a460ca046ac223534839..f156e5eed02586fe3f1dc35d4d420cc24397b05a 100644
--- a/Crashlytics/Crashlytics/DataCollection/FIRCLSDataCollectionArbiter.m
+++ b/Crashlytics/Crashlytics/DataCollection/FIRCLSDataCollectionArbiter.m
@@ -14,9 +14,9 @@
 
 #import "FIRCLSDataCollectionArbiter.h"
 
-#import "FBLPromises.h"
+#import <FBLPromises/FBLPromises.h>
+#import <FirebaseCore/FirebaseCore.h>
 
-#import "FIRApp.h"
 #import "FIRCLSUserDefaults.h"
 
 // The legacy data collection setting allows Fabric customers to turn off auto-
diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m
index fd0ece6883ef19b55207b7a0add3ea537de89e39..24dc5ae3c2431e841b0cc42ce5a13e58b3b2b37d 100644
--- a/Crashlytics/Crashlytics/FIRCrashlytics.m
+++ b/Crashlytics/Crashlytics/FIRCrashlytics.m
@@ -14,7 +14,7 @@
 
 #include <stdatomic.h>
 
-#import "FBLPromises.h"
+#import <FBLPromises/FBLPromises.h>
 
 #include "FIRCLSCrashedMarkerFile.h"
 #import "FIRCLSDataCollectionArbiter.h"
diff --git a/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfExpressionMachine.h b/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfExpressionMachine.h
index 2f7ecc27a9ab9a22f468c7406237216ed825f4e7..ec3e10935916d2d27d184fe1fec7aeb16bb94b6d 100644
--- a/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfExpressionMachine.h
+++ b/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfExpressionMachine.h
@@ -16,6 +16,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
+#include "FIRCLSFeatures.h"
 #include "FIRCLSThreadState.h"
 
 #define CLS_DWARF_EXPRESSION_STACK_SIZE (100)
diff --git a/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfUnwindRegisters.h b/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfUnwindRegisters.h
index bfb45ad5d8a73fbefc9af1b574a4734cb7fe13da..5f0506337602a8ee81217cc152844d19bdd48107 100644
--- a/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfUnwindRegisters.h
+++ b/Crashlytics/Crashlytics/Unwind/Dwarf/FIRCLSDwarfUnwindRegisters.h
@@ -14,6 +14,8 @@
 
 #pragma once
 
+#include "FIRCLSDefines.h"
+
 #include <stdint.h>
 
 #if CLS_CPU_X86_64

@samedson
Copy link
Contributor Author

samedson commented Jan 8, 2020

Hey @igor-makarov - thanks for the patch! Can you let us know how you set up building the SDK?

We're going to submit this now to unblock the release testing but may include that patch in a later PR if it ends up being necessary

In terms of upload-symbols, we weren't yet able to open source it.

@samedson samedson merged commit 3dc1185 into master Jan 8, 2020
@maksymmalyhin
Copy link
Contributor

@samedson FYI, to make Promises import working with all possible Pod configurations we had to define the import like this. You may consider the same approach unless you can come up with something nicer.

To validate working imports we do following:

  • run pod lib lint with flags --use-libraries and --use-modular-headers see an example
  • validate different Podfile options with tests. Here are the configurations we check.

@igor-makarov
Copy link
Contributor

@samedson The way I've integrated the SDK is by adding it as a repo to my Podfile, like so:

  pod 'FirebaseCrashlytics', git: 'https://github.com/firebase/firebase-ios-sdk', branch: 'crashlytics-staging'

Additional info - I install pods with use_frameworks!, use_modular_headers! and incremental_installation enabled.

@paulb777
Copy link
Member

paulb777 commented Jan 8, 2020

Hi @igor-makarov! Thanks for taking a look!

We changed some import/include organization in the process of open sourcing and now working through resolving some resulting issues, as well as testing CocoaPods with the various library/framework options.

To use your patch, we need it in a PR with a signed CLA. However, I expect we'll be able to reimplement, if you don't get to it, since it should come up in our expanded open source testing.

@igor-makarov
Copy link
Contributor

Hmm, let me see if I get to it. In any case, it's not blocking me for now.

I've noticed there seems to be a serious incompatibility between FIRCrashlytics and the old Crashlytics. Am I missing something?

Is there transition documentation?

@samedson
Copy link
Contributor Author

samedson commented Jan 9, 2020

@igor-makarov yes! There will be a transition document when we officially release. The FIRCrashlytics and Crashlytics SDKs are not meant to be used together as multiple crash reporters have a tendency to not work well together when they both try to register exception handlers.

@igor-makarov
Copy link
Contributor

I meant that their interfaces seem to differ quite a bit.

@samedson
Copy link
Contributor Author

samedson commented Jan 9, 2020

That's right - FIRCrashlytics will be released under a new beta version 4.0.0-beta.1 with APIs that are more consistent with the other Firebase SDKs. The transition document will help with upgrading to the new API.

@paulb777 paulb777 deleted the crashlytics-staging branch January 10, 2020 01:52
@firebase firebase locked and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: core api: crashlytics cla: yes zip-builder Tools related to building the zip file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashlytics for Mac Catalyst
7 participants