Skip to content

Conversation

santoshbagadi
Copy link
Contributor

Fixes #333

@googlebot
Copy link

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
Corporate signers

ℹ️ Googlers: Go here for more info.

@santoshbagadi
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@stewartmiles stewartmiles left a 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.

https://github.com/wprig/wprig/wiki/How-to-squash-commits

@stewartmiles
Copy link
Contributor

@santoshbagadi any chance you could squash this into a single commit so that it's easier to review in one go?

https://github.com/wprig/wprig/wiki/How-to-squash-commits

@santoshbagadi santoshbagadi force-pushed the feat/add_ability_to_set_target_from_dependencies_file branch from d4942cc to c4c18bf Compare March 11, 2020 23:23
@santoshbagadi
Copy link
Contributor Author

santoshbagadi commented Mar 11, 2020

@stewartmiles done!

@stewartmiles
Copy link
Contributor

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

@santoshbagadi santoshbagadi force-pushed the feat/add_ability_to_set_target_from_dependencies_file branch from c4c18bf to a179d00 Compare March 12, 2020 01:55
@santoshbagadi
Copy link
Contributor Author

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 😅

@stewartmiles
Copy link
Contributor

@santoshbagadi sounds good, I just pushed a release so you'll need to rebase. I think once that's done, I'll merge.

@santoshbagadi santoshbagadi force-pushed the feat/add_ability_to_set_target_from_dependencies_file branch from a179d00 to 16ce8d0 Compare March 12, 2020 18:02
@santoshbagadi
Copy link
Contributor Author

Changes made

@stewartmiles
Copy link
Contributor

Sweet, thanks again!

@stewartmiles stewartmiles merged commit 5193d38 into googlesamples:master Mar 12, 2020
@santoshbagadi santoshbagadi deleted the feat/add_ability_to_set_target_from_dependencies_file branch March 12, 2020 19:52
@stewartmiles
Copy link
Contributor

@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
Copy link

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).

@stewartmiles
Copy link
Contributor

@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
Copy link

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.

@santoshbagadi
Copy link
Contributor Author

@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.

@googlesamples googlesamples locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource bundles not linked when adding pod dependency to UnityFramework target
4 participants