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

Add support for using URLCache on macCatalyst #1460

Merged
merged 2 commits into from Dec 4, 2019

Conversation

mman
Copy link
Contributor

@mman mman commented Oct 15, 2019

This simple pull request makes it possible to compile Parse-SDK-iOS-OSX for Catalyst. It also requires the following patch to Bolts-ObjC parse-community/Bolts-ObjC#1.

@mman mman mentioned this pull request Oct 15, 2019
@TomWFox
Copy link
Contributor

TomWFox commented Oct 16, 2019

Thanks for doing that. I don't think we can rely on the bolts pr being merged anytime soon, I wonder whether it might be a good idea to fork bolts into the parse-community. thoughts?

cc @drdaz @noobs2ninjas

@mrmarcsmith
Copy link

I think a Parse community fork of bolts is a wonderful idea. It’s not often that we need to modify it and we could switch back if the Bolts community ever decides to pull our changes but at least we wouldn’t have to wait for them to approve PRs in the mean time

@dplewis
Copy link
Member

dplewis commented Oct 17, 2019

@TomWFox I had that idea a while ago. Bolts project isn't maintained anymore or perhaps someone can take it over.

It was mentioned that we should replace it with PromiseKit (12k stars on GitHub)or a similar alternative or create an extension.

But for now a fork would solve a lot of issue with building.

@drdaz
Copy link
Member

drdaz commented Oct 17, 2019

I mean sure, we can fork it.

My only concern is development ressources. It's more code waiting to fail 😃. On the other hand, historically, it hasn't been high-maintenance.

(I hope I didn't jinx it.)

@brianyyz
Copy link

+1 for removing a dependency if the Bolts community is not active and as @mrmarcsmith says we can always merge back if things pick up.

@noobs2ninjas
Copy link
Member

noobs2ninjas commented Oct 18, 2019

I agree. You know what? That is still maintained by facebook. Seeing as we have history with them. I'm curious if they'd let us just have it. Cant remember the history of Bolts but Parse has always used it even in the early early days back when it was first release. In fact, unless my memory deceives me(which it could), Bolts was originally a Parse framework aka it was transfered to Facebook after the purchase.

@noobs2ninjas
Copy link
Member

noobs2ninjas commented Oct 23, 2019

Hey @mman while I think your update to bolts is still necessary I think it actually misses the actual problem we are having here. I just noticed its the same issue we are having with building Parse dependency for ParseLiveQuery.

So the primary issue here that we are facing is this. First we have target dependencies that have name ambiguity issues aka rather than having Bolts-OSX, Bolts-iOS, Bolts-Dynamic all targets are just Bolts. In Xcode 10 this wasn't an issue because the system would simply pick the Bolts target that was intended for the OS it was building for aka Bolts(macOS).

In Xcode 11 this became a lot more complicated. Every iOS build target as well as every dynamic target now can ALSO build for OSX as well as iOS and iPad OS. So rather than just grabbing the Bolts(macOS) target. It now grabs the first one it finds which is the dynamic. Now, if you look at the Bolts(macOS) target you'll notice that BFWebViewAppLinkResolver.m etc doesn't even build for this target. However, if you look at Bolts(iOS-dynamic) you'll notice in the build dependencies that this file is labeled for both macOS and iOS. This is the reason this file is even getting built to begin with. I think the main issue is that our rake file thats used to setup the build itself actually specifies these build targets. Either that or in Xcode 11 dynamic framework targets is just now the default it looks for regardless of the OS. In either case to properly fix the issue we need to make it where these files arent being built for macOS in the first place.

Heres the picture to show you what I'm talking about.

Bolts_xcodeproj

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

I just remembered that as of this change to master, we don't actually depend on the full Bolts package anymore, just Task.

So this change to Bolts isn't actually necessary for things to work.

@noobs2ninjas
Copy link
Member

noobs2ninjas commented Oct 23, 2019

Also,I really like @dplewis idea and think its the better long-term solution here. However, if I(unless someone else wants to) jump onto github as parse-community and do the fork, I think we can fork Bolts, get it update and push out new versions of both Parse and Parse Live Query in about a day and start getting caught up on some much needed updates. PromiseKit would be a lot taller order and there are already issues stacking up on both repos because of this underlying problem.

@noobs2ninjas
Copy link
Member

noobs2ninjas commented Oct 23, 2019

@drdaz you are correct. Technically this issue is isolated specifically to the building for macOS via xcbuild as Xcode allows us to specify specific build targets from sub projects. Aka not something anyone is likely to do. Just build Parse for OSX via terminal. Doesnt make sense in any other setting than a nightly build or something.

@noobs2ninjas
Copy link
Member

To add to that. @drdaz may have a great point. If we fork Bolts it would probably allow us to thin the project way out to only what we use for parse and parse live query.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

@noobs2ninjas About static and dynamic, I rolled back some changes made by @flovilmart that made the binaries build in separate locations, because they caused the build to fail.

I'm not sure where he was going with it (or indeed if it was an unwanted commit), and he's not around to ask. I'm also not sure if / how it affects what you're describing here. But it maybe worth having a sniff.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

About the fork, I'd wait until we actually need to make changes. What's in master right now doesn't need that.

@noobs2ninjas
Copy link
Member

@drdaz interesting. I'd definitely be interested to know what his intent was. The only thing I could think of is if you wanted to use it for like cache or something to speed up the build time for the project. I dont think this was an issue. I've definitely gotten a few build errors even inside xcode though because of it. Post-Xcode 11 its definitely become an issue.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

@drdaz interesting. I'd definitely be interested to know what his intent was. The only thing I could think of is if you wanted to use it for like cache or something to speed up the build time for the project. I dont think this was an issue. I've definitely gotten a few build errors even inside xcode though because of it. Post-Xcode 11 its definitely become an issue.

Yeah it's hard to know. I have a suspicion that it was a bastard commit; that's why I reversed it.

I checked through his work, and most of the dynamic / static work he did was many months before, in the main SDK repo. This one came along with some unrelated changes we made to xctoolchain. So it really looks like something he might have been experimenting with locally and never completed, that just slipped through.

@noobs2ninjas
Copy link
Member

noobs2ninjas commented Oct 23, 2019

Ok here is the forked bolts framework. @mman if you wouldnt mind creating a pull request here as well that would be awesome. I'm going to make a few adjustments to the framework to fix the issues above.

After this you think we should open an issue and have a discussion about whether to keep this solution long term. If we do keep it we should probably add our own builds to CircleCI for future pull requests. Also, we should probably look at only keeping what we actually use from it as @drdaz pointed out Parse uses only a very small portion. In fact maybe we should change the name and call it Bolts-Tasks or whatever and basically have our own little light weight Promise-Kit style Bolts library. Anyway, we can decide that in a discussion later. If everyone else is on the same page we are just looking to get our migration to mac catalyst/Xcode11 100% completed right now and well make a more official decision going forward in the coming weeks.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

Also, we should probably look at only keeping what we actually use from it as @drdaz pointed out Parse uses only a very small portion. In fact maybe we should change the name and call it Bolts-Tasks or whatever and basically have our own little light weight Promise-Kit style Bolts library.

I pointed out that we don't need to fork or change anything in Bolts right now, because we've already moved to just depend on Bolts/Tasks, which is currently sufficient for this repo's needs (EDIT: and doesn't use any deprecated APIs). I don't really understand what we gain from maintaining our own Bolts fork at this stage 🤷🏽‍♂️.

EDIT: Am I missing other changes, perhaps elsewhere, we want to make to Bolts than this issue with the deprecated Web APIs?

@noobs2ninjas
Copy link
Member

@drdaz Oh I didnt realize that this was already done. Now, I see that its been changed in the podspec. The primary issue here is that this hasn't been implemented in Carthage which unfortunately is responsible for everything inside the project because it interacts with git submodules which is how circle CI builds all its targets. Aka just doing it in the podspec only matters for deploying to cocoapods. If it's not changed in carthage it might as well not be changed at all. Anyway, this is where our disconnect was. I thought you where more or less just suggesting it because I hadnt noticed it anywhere else.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

@noobs2ninjas Ahhh! My bad; I merged that change and totally didn't think of Carthage 😃

@noobs2ninjas
Copy link
Member

@drdaz No problem. Need to figure out how carthage and git submodules deals with...well submodules.

@noobs2ninjas
Copy link
Member

Wait...can carthage do submodules? Unlike cocoapods I see our carthage instructions only includes github "parse-community/Parse-SDK-iOS-OSX". Nothing like Parse\UI or anything like that. So, I'm not sure how to do it for bolts. If not we need to see if git submodules can do sub-submodules? haha that doesnt seem like it makes sense but I dont know how else to word it.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

@drdaz No problem. Need to figure out how carthage and git submodules deals with...well submodules.

Yeah 😬. Some quick googling doesn't suggest that it's actually a thing. At least, not for what we're trying to do here.

@noobs2ninjas
Copy link
Member

Subspecs! Thats the word I'm looking for.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

Looks like that's really not something Carthage does well. Grrrr.

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

So umm yeah. I guess that's why we need that fork 😃

@noobs2ninjas
Copy link
Member

Well dang. I got really exited there for a minute. Beyond fixing the build it would have really dropped the size and therefore build time using a subspec. So, what do you think is the call here. Should we leave the Podspec alone and just fix this for Carthage?

@noobs2ninjas
Copy link
Member

noobs2ninjas commented Oct 23, 2019

Holy crap I was right. Yea we should just ask Facebook for Bolts. Have you read the main ReadMe. It literally gives examples using Parse aka Bolts was originally created for Parse.

// Swift
var query = PFQuery(className:"Student")
query.orderByDescending("gpa")
findAsync(query).continueWithSuccessBlock {
  (task: BFTask!) -> BFTask in
  let students = task.result() as NSArray
  var valedictorian = students.objectAtIndex(0) as PFObject
  valedictorian["valedictorian"] = true
  return self.saveAsync(valedictorian)
}.continueWithSuccessBlock {
  (task: BFTask!) -> BFTask in
  var valedictorian = task.result() as PFObject
  return self.findAsync(query)
}.continueWithSuccessBlock {
  (task: BFTask!) -> BFTask in
  let students = task.result() as NSArray
  var salutatorian = students.objectAtIndex(1) as PFObject
  salutatorian["salutatorian"] = true
  return self.saveAsync(salutatorian)
}.continueWithSuccessBlock {
  (task: BFTask!) -> AnyObject! in
  // Everything is done!
  return nil
}

@drdaz
Copy link
Member

drdaz commented Oct 23, 2019

Well dang. I got really exited there for a minute. Beyond fixing the build it would have really dropped the size and therefore build time using a subspec. So, what do you think is the call here. Should we leave the Podspec alone and just fix this for Carthage?

Yeah I really liked the idea of using the subspec, and particularly not having to maintain more code ourselves.

I think both dependency managers should refer to the same dependency. In every other direction, madness lies.

EDIT: I think I may have misunderstood you. But I don't think we can avoid the fork if we want to fix that deprecation issue, and Bolts isn't being maintained. And as long as we don't use anything other than Tasks, I don't see an issue with referring the subspec of the fork directly for CocoaPods.

@noobs2ninjas
Copy link
Member

@drdaz I meant I can just fix Carthage to work with our forked version of Bolts and leave the Podspec alone for now since it really isnt the problem here. However, I agree with you and would prefer to have everything pointing to the same place.

What I would like to do here is literally strip down our fork to nothing but bolts/tasks. I haven't checked ParseLiveQuery though to see if it is basically using bolts-task too. Guess if it's doing more than that we could have two forks. I think a better option might be to just create specific targets for each framework to build, one with just task and the other full. We'll take a look though.

I am a little sentimental about Bolts since it was literally the backbone of Parse since its inception and I've used it for years. In fact I am super surprised it wasn't given to us to begin with because of that. I checked it out and I don't think the Facebook employee who was assigned to it is working for them anymore. None of his contact information on github is valid including his facebook. He hasn't committed anything anywhere in months. So, I sent a message to the employee who's over react-native. To me ideally we would have it transfered. At least continue to maintain it for the community but also really start making it more specific for our uses. I dont think Bolts real intention was ever to be used outside of Parse to begin with from what I remember.

@mman
Copy link
Contributor Author

mman commented Oct 24, 2019

@noobs2ninjas I have updated the pull request against the forked repo. It should be here: parse-community/Bolts-ObjC#1

@noobs2ninjas
Copy link
Member

Lets go ahead and merge this and then we can work on bolts.

@noobs2ninjas noobs2ninjas merged commit 1347567 into parse-community:master Dec 4, 2019
@TomWFox TomWFox mentioned this pull request Apr 13, 2020
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.

None yet

8 participants