-
-
Notifications
You must be signed in to change notification settings - Fork 875
Remove iPhoneSimulator #1496
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
Remove iPhoneSimulator #1496
Conversation
So it seems the builds are running fine, which to me suggests this change is fine from a CI point of view. @drdaz @noobs2ninjas thoughts? I would think we would also need to remove iPhoneSimulator from ParseUI, Facebook and Twitter utils but I'm just waiting for feedback from @terryworona in #1491. |
@TomWFox I'm wondering what the possible values of that field are. And I'm wondering what we lose by making the change? |
@drdaz not sure, I did look into it a bit but found very limited info, I thought it might break the test builds but it seems not to. In my not very scientific research I checked some other other iOS SDKs to see if they used iPhoneSimulator and none of them did - in fact none of them even had supported platforms specified in their info.plist. I’ll try and look into it some more tomorrow, but feel free to take a look yourself if you want to! |
I did check it out before I wrote. Like you said, the documentation is not great. Mostly hearsay type stuff. The one bit of official documentation I found said the simulators shouldn't be present in that field when the binary is sent to Apple. But that it could also be a problem when it's missing. I feel like it's probably the responsibility of the app that's using our SDK to provide those values. |
It’s strange that this has only popped up now considering we never added iPhoneSimulator. It wouldn’t be the end of the world to leave it to the app dev but ideally we don’t want those errors coming up by ‘default’ when archiving. |
@TomWFox yes it is weird indeed that its popped up. Of course lots of things changed this year. For instance just opening an old project in Xcode 11 marks iOS/Universal targets as Catalyst capable. Perhaps this is one of the other liberties Apple is taking as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see how this could hurt really.
I mean we absolutely did add it; I checked the commit last night (nlutsenko in 2015 IIRC) 😃. It's odd that it's just becoming a problem now though. I really wonder about the possible values. It's been quite a while since there's officially been anything called 'iPhoneOS'. And about removing the whole field, we'd need to check that it works first obviously. But I'd imagine that Parse would inherit the supported platforms of the containing app. And I'd imagine that there's a requirement for the containing app to have those details. |
Yes that’s what I was getting at - by we I meant me you and Nathan didn’t add it 🙂
I agree. I think the the main risk is that it messes up the tests but that should be quite obvious. |
@noobs2ninjas it's not a big deal as I'll just open another PR to finish this off but so you know when I create a draft PR that means I don't want it to be merged yet. |
This is an experimental fix for #1491 and #1495.
Just want to see if the tests run ok...