-
Notifications
You must be signed in to change notification settings - Fork 371
Add ability to set XCode target from Dependencies file #335
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 ability to set XCode target from Dependencies file #335
Conversation
googlebot
commented
Mar 10, 2020
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
📝 Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
What to do if you already signed the CLA
Individual signers
- It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
Corporate signers
- Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
- The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
- The email used to register you as an authorized contributor must also be attached to your GitHub account.
i️ Googlers: Go here for more info.
@googlebot I signed it!
googlebot
commented
Mar 10, 2020
CLAs look good, thanks!
i️ Googlers: Go here for more info.
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.
Thanks for the patch, I've added a few comments.
It would be great to squash the commit so that it's easier to review in one pass since there are overlapping changes in here.
@santoshbagadi any chance you could squash this into a single commit so that it's easier to review in one go?
d4942cc
to
c4c18bf
Compare
@stewartmiles done!
I just gave this a try with a FIrebase project and it appears to work, very nice job!
The only other thing I just noticed is that it would be great to update "documentation" (it's just a sample) in https://github.com/googlesamples/unity-jar-resolver/blob/master/sample/Assets/PlayServicesResolver/Editor/SampleDependencies.xml#L52
Also, feel free to tag your commit message with the issue this is resolving https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
c4c18bf
to
a179d00
Compare
Added target
to documentation. I'm not sure whether or not to mention what the default value will be, since it depends on the Unity version and could change in the future.
I've already tagged the issue that this PR resolves here 😅
@santoshbagadi sounds good, I just pushed a release so you'll need to rebase. I think once that's done, I'll merge.
a179d00
to
16ce8d0
Compare
Changes made
Sweet, thanks again!
@santoshbagadi while testing our latest release we found this caused a regression for iOS builds in some versions of Unity so we're rolling this back.
sp-ivan-hernandez
commented
Mar 23, 2020
This was fixing unity 2019.3 for us, not becaue we were able to set the target of a pod, but because in our xcode project the Unity-iPhone target in the Pods project existed again thanks to the
target Unity-iPhone do
end
added in the podfile (despite not having any pods).
@chkuang-g FYI
@sp-ivan-hernandez are you saying you need the empty "target" in the Podfile to fix an error for your build? Any chance you have an example project that demonstrates this?
sp-ivan-hernandez
commented
Mar 23, 2020
i can not share the project right now but i can provide some info about it:
this is the Podfile
source 'https://github.com/CocoaPods/Specs.git'
platform :ios, '10.0'
target 'Unity-iPhone' do
end
target 'UnityFramework' do
pod 'Bolts', '~> 1.7'
pod 'FBSDKCoreKit', '~> 5.2'
pod 'FBSDKLoginKit', '~> 5.2'
pod 'FBSDKShareKit', '~> 5.2'
pod 'Firebase/Analytics', '6.14.0'
pod 'IronSourceAdColonyAdapter', '= 4.1.8.1'
pod 'IronSourceAdMobAdapter', '= 4.3.8.4'
pod 'IronSourceAmazonAdapter', '= 4.3.2.0'
pod 'IronSourceAppLovinAdapter', '= 4.3.8.1'
pod 'IronSourceFacebookAdapter', '= 4.3.10.1'
pod 'IronSourceHyprMXAdapter', '= 4.1.3.4'
pod 'IronSourceSDK', '= 6.14.0.0'
pod 'IronSourceUnityAdsAdapter', '= 4.1.8.4'
pod 'IronSourceVungleAdapter', '= 4.1.9.1'
pod 'TapResearch', '= 2.0.9'
end
Without the "target 'Unity-iPhone' do end" the application was crashing on startup because of IronSourceHyprMXAdapter resources not found.
But with the "target 'Unity-iPhone' do end" all is working fine.
Screenshot 2020年03月23日 at 18 31 18
Notice that Pods-Unity-iPhone is created and Pods-Unity-iPhone-frameworks.sh adds the HyprMX.framework.
@stewartmiles I have also noticed that having an empty Unity-iPhone
target in the pod file was fixing the issue as well.
Would you mind providing more information on what the exact issue that these changes are causing. Apart from adding an empty Unity-iPhone
target to the pod file, it shouldn't have changed any other behavior when a target is not specified.
Fixes #333