-
-
Notifications
You must be signed in to change notification settings - Fork 878
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
Update Facebook SDK to 5.2.1 & 1.17.3 version bump #1424
Conversation
*update podspec file for new Facebook SDK 5.1.1
@drdaz could you take a look at this? |
@rico237 The CHANGELOG seems to have been abandoned but in the spirit of getting this SDK in better shape could you make an entry for 1.17.3. If you could follow the style in the Parse Server CHANGELOG that would be great. |
OK, btw Travis tests are not passing i will try to update my code later this weekend or next week so they can all pass |
@TomWFox The changes look fine to me. Either I'm blind, or I don't have the option to check this PR out; maybe it's because I'm not a 'Member' here. But I checked out @rico237's branch and ran the test suite, and it passed. Which is good. I also integrated his branch in my own app and tried a login. It worked. Also good. But it's weird that the Travis build fails. I'm not sure why that happened. |
@drdaz If you are interested in contributing more I can ask the others about adding you to the org on the iOS team which would make things a bit easier. I looked at the previous release and other files were changed such as the info.plist files to reflect the version bump, maybe that could be causing Travis to fail. |
@TomWFox That might be a good idea, yeah. I mean all @rico237's changes happened in his branch, including the version bump. I suspect this is related to the CI build process. This is the end of the build log from Travis:
Is there some way of just triggering a new build in there to see if it happens again? |
@drdaz yep, I've restarted it |
Same deal it seems :( It looks like when it builds on Travis, the updated types from the new v5 SDK aren't there; so it's using the old version of the FB SDK. The Cartfile still points at ~> 4.29 of the facebook-objc-sdk. That certainly needs to be updated, and seems like a possible source of the problem. |
I will make the change on the cartfile to see if that corrects this issue |
🤞🏼 |
Sweet. We've got a new error:
Looks like the signature for the initialiser for FBSDKAccessToken changed, but the build is using the old version for some reason. In my checkout of @rico237's branch I can see a binary copy of the FBSDKCoreKit framework with the old signature for the initialiser. |
Looks like that was temporary. We're back to the same error again. |
May I had you @drdaz as a collaborator to my branch/fork ? I will work on those errors as well but maybe you have a better experience than I have and fix those before me ... |
@rico237 Sure. I'm not sure I'll be faster than you here though :) |
I'm still a junior ios dev. |
So I've found the cause for the current build issue; in the ParseFacebookUtils project, there's a build phase (script) called 'Fetch Latest Dependencies'. It grabs a zip file from FB's servers - https://origincache.facebook.com/developers/resources/?id=facebook-ios-sdk-current.zip. This zip file contains a version 4.29 of the FB libs. The 2 FBSDK files (Core, Login) end up used in the ParseFacebookUtils project, in Frameworks/User Frameworks/Static/iOS/. I've tried removing the build phase and just copying the files from Carthage/Build in their place, but this doesn't seem to work. Probably dynamic vs static frameworks at play here. |
Ok i see the problem here xD There is the official Facebook releases I think you can try with this link for statics frameworks |
Way ahead of you :) The iOS version releases for me now. Getting tvOS working. |
xD im gonna try in my project too 😮 |
@drdaz good question. I’ll try to make time today to check the Carthage bug. |
@drdaz Carthage is still failing but it looks like your fork is actually a fork of "tamir-maoz/Parse-SDK-iOS-OSX" which is a fork of "parse-community/Parse-SDK-iOS-OSX" and the tamir-maoz repo is 3 commits behind master. Do you have access to update the "tamir-maoz/Parse-SDK-iOS-OSX"? Here is the one liner I use to test carthage.
it failed with the following error:
|
@mrmarcsmith Something fishy is going on here 🥴 When I run your one-liner, I don't get that error. What stands out to me in your error output is the path; I do get another error, however, later in the process while building ParseFacebookUtilsV4-iOS-Dynamic. I'll look into that one. I tried this a couple times, the second time deleting DerivedData. Results were the same. |
@drdaz do you have access to the tamir-maoz repo to update it? I think that might fix it. |
No I don’t, just Rico. But I don’t understand why I’m not seeing the same issue? We should be running the same code? Is there a specific commit you think will fix it? |
@mrmarcsmith Looking at the latest 3 commits here, this is the only one which changes any code (the others are documentation changes): 4a82099 I don't believe this change would have an effect on the Carthage build. |
any news? Is it safe to use this branch? |
@markuswinkler I think it is safe to use that branch since the main changes are just the version number of the libs used by the Parse SDK, the thing is we are having issues with the tests in circleci (local tests seems to be passing most of the time). To keep the project as clean and bug free as possible those tests need to pass before merging this branch. (this is the only reason why this branch as not been merged yet) |
@markuswinkler There are also issues with the Carthage build. If you don't use Carthage to install, you should be fine. |
To be clear we can merge with the tests not passing (they haven’t been for months) it’s the Carthage build which is more concerning. |
@TomWFox Can you try running @mrmarcsmith's one-liner at some point and see where / if it fails for you? EDIT: Here's hoping it's not a third place 🤞🏼 |
So a little update here. I just spent some time trying to figure out why this fork wasn't building with Carthage. I tried the same one-liner against an older commit, before we started doing all the things. It still failed. I tried against the main repo at master, and it still fails. I used this line:
So... whatever the problem is, it doesn't really look to me like the source is from this fork. With that said, I still haven't been able to reproduce the issue @mrmarcsmith mentioned - my build fails when compiling ParseFacebookUtils-*-Dynamic. @TomWFox (EDIT: or anybody else) can you try running this Carthage build command and see if / where you fail? It would be nice to have some consensus on the actual state of things here. EDIT2: The main repo fails for me in the same place (ParseFacebookUtils-*-Dynamic) but with a different error. sigh |
As previously mentioned I got Carthage build working in my fork of your repo here maybe you could compare forks to see what’s breaking your Carthage build This would be the one liner |
For rollback of separation of dynamic and static builds
@mrmarcsmith Thanks for that. Carthage builds now. I feel a little silly; we even discussed the relevant change in your review. 🥴 But at least we got there. |
My goodness I think we're there. Unless there are any objections? |
Actually, these pointers look like they need updating too: https://github.com/parse-community/Parse-SDK-iOS-OSX/tree/master/Carthage/Checkouts Does anybody know if / where they're used? |
Those dependencies are used in the Parse-SDK-iOS-OSK framework itself. For example, Bolts is the framework that allows us to use in-line asynchronous callbacks. I would leave them for now since we really need to get this PR released and they aren’t hurting anything. You can open a new PR that bumps them to keep the repo fresh though! |
Is there a specific reason why the Facebook SDK will be locked down to 5.2.1? |
@markuswinkler it shouldn't be locked to that. We're using the ~> notation in the dependency managers. If you use this fork in Cocoapods or Carthage, you'll get FB SDK 5.5.0. |
Aaaand merged. Finally :) |
... the merge seems to have broken so many tests. sigh |
Re-ran the tests on CI. All green except macOS 👌🏼 |
Tried to run pod update on from one. Still limits to 5.2.1. |
@markuswinkler Just pushed a couple changes that loosen the FB deps for both Carthage and Cocoapods. I actually did test this before claiming we weren't locked in, but FB Core got installed at v5.5.0 and confused me (we don't specify a version for FB Core) 😊 |
@mrmarcsmith Just getting back to this, I'm confused as to why these mismatched dependencies aren't causing trouble if they're actually being used. They're API incompatible at this point. Just trying to figure out how all this is strung together really 😊. It's weird to me that we have those submodules defined where they're going to get overwritten by Carthage. |
Anyone facing issue with Facebook Login? What's happening is that when I sign in through facebook, after a while I get Invalid Session Token error on queries I do |
Fix issue :