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

Upgrade Firebase to 5.7.0 and prune frameworks #826

Merged
merged 2 commits into from
Sep 8, 2018

Conversation

briantq
Copy link
Contributor

@briantq briantq commented Sep 5, 2018

Related Issue #796

I updated the Firebase SDK to 5.7.0. From what I can tell, it seems to be working. I was able to test it in my application (which only references Messaging), but got everything to compile successfully.

@briantq
Copy link
Contributor Author

briantq commented Sep 6, 2018

@soumak77 Thanks for letting me know about the failing tests. From what I can tell, it appears that the common resolution for Error code 65 when dealing with Frameworks is to remove and add the ios platform back again. I believe this is because cordova prepare will add the Frameworks, but if they is a different version already there, it's likely going to cause problems. I multiple issues with this error and the same solution 1 2.

Is there some way we can modify the test to run that prior to running the tests? Since I am just moving frameworks around and made a single line change, it seemed to make no sense why certain version of Cordova are failing...

If you want, I can include in the pull request an update to the Readme that mentions what to do if you see this error.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

@briantq I'm fine if we need to add cordova prepare to the tests, however, the tests run a clean install of the plugin on travis, so I wouldn't expect for that line to be needed. It would be ideal if you can figure out the signing issue you were running into when running the iOS tests locally, so that you can troubleshoot without having to make changes to the PR and wait for the travis tests to run.

If you do find you need to add cordova prepare, I think it would need to go at the bottom of the file platform-add.sh. Feel free to add this line to see if it fixes the issue for the travis tests. If not, just revert that change.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

@briantq you can also run just the iOS tests for a specific cordova version. Here are all the iOS test commands:

npm run test:cordova@6.5.0:ios@4.5.4
npm run test:cordova@7.1.0:ios@4.5.4
npm run test:cordova@8.0.0:ios@4.5.4

Only the first two are failing on travis. For whatever reason, the last test is fine.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

@briantq one last thing to be aware of is that the tests install cordova locally only, so it won't affect the cordova version you have installed globally on your system

@briantq
Copy link
Contributor Author

briantq commented Sep 6, 2018

@soumak77 I was able to get it working on another machine and running both
npm run test:cordova@6.5.0:ios@4.5.4
npm run test:cordova@7.1.0:ios@4.5.4
completed successfully.

Is there some where I can take a look at the automated test configuration? I can try to figure out what's different on the CI build than my machine.

If you have 2 minutes, would you mind running one of the tests on your machine from briantq:firebase-sdk-5.7.0. I want to make sure I am not crazy.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

You can check out the travis configuration here: https://github.com/arnesson/cordova-plugin-firebase/blob/master/.travis.yml

I can respond to any comments in a timely manner but won't be able to help with development changes until after the weekend as I won't be near my development machine.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

I do see the iOS tests run on xcode 8.3. What verision of xcode are you using? I dont know if many even still use xcode 8.x, so if we need to upgrade the xcode version on the tests it might be okay.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

I don't plan on releasing any new versions soon, so I'm ok with merging this PR so that others might be able to help out and test the changes without needing to use your fork.

The only change we might want to get in before that is adding back the auto-init of firebase. I can try to figure out which line(s) were removed by the Crashlytics PR and release a patch version before merging this PR.

@briantq
Copy link
Contributor Author

briantq commented Sep 6, 2018

@soumak77 we should definitely fix the firebase init first. I am no longer blocked on my development as I can reference the branch directly from my fork.

Good call on the XCode version. I am currently running 9.4.1. Maybe that has something to do with it... Still odd that one passes since it really is just copying frameworks. I am shocked that cordova versions affect that.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

@briantq looks like updating the xcode version fixed the tests. I'll merge this PR and publish v2.0.0 of this plugin once we get v1.1.4 out with the auto-init fix.

Would you happen to know what code needs to be added back to fix the auto-init issue?

@briantq
Copy link
Contributor Author

briantq commented Sep 6, 2018

Sweet, it was easy to try it. I can separate out the XCode test update to a separate PR.

I agree it makes sense in a 2.0.0.

I can look, there were a lot of changes. We definitely need to modify FirebasePlugin.java. I was looking at AppDelegate+FirebasePlugin.m and I don't think the PR changed the initialization at all on iOS. If we get doc updates, we probably want to reflect that the initialization depends on which platform you are developing for.

@soumak77
Copy link
Contributor

soumak77 commented Sep 6, 2018

No need to split the xcode changes to a new PR as it is required to make this PR pass the tests, so it makes sense to keep it as part of this PR.

@soumak77 soumak77 merged commit 48fca22 into arnesson:master Sep 8, 2018
@briantq briantq deleted the firebase-sdk-5.7.0 branch September 9, 2018 05:07
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.

2 participants