Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

Copy link
Contributor

@santoshbagadi santoshbagadi commented Mar 10, 2020

Fixes #333

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

i️ Googlers: Go here for more info.

Copy link
Contributor Author

@googlebot I signed it!

Copy link

CLAs look good, thanks!

i️ 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

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
Copy link
Contributor Author

santoshbagadi commented Mar 11, 2020
edited
Loading

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
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 😅

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
Copy link
Contributor Author

Changes made

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

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

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?

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.

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.

sp-ivan-hernandez reacted with thumbs up emoji

@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.
Reviewers
1 more reviewer

@stewartmiles stewartmiles stewartmiles left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resource bundles not linked when adding pod dependency to UnityFramework target

AltStyle によって変換されたページ (->オリジナル) /