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

fix build error when building on XCode 8.3 #121

Merged
merged 14 commits into from
Mar 30, 2017
Merged

Conversation

igor-makarov
Copy link
Contributor

@igor-makarov igor-makarov commented Mar 28, 2017

Fix for #119

@button-bot
Copy link

Hi @igor-makarov,
Thank you for your Pull Request!

It looks like you haven't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen license.
Wikipedia

Please read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to let us know.

Thank you,

ButtonBot

@igor-makarov
Copy link
Contributor Author

[clabot:check]

@button-bot
Copy link

@wessmith It looks like @igor-makarov just signed our Contributor License Agreement. 👍

ButtonBot

@Unihilator
Copy link

whats wrong with the checks? Will it be merged today? 😄

@igor-makarov
Copy link
Contributor Author

It fails because the bundler is on CocoaPods 0.39 and it's been EOL.

Copy link
Contributor

@chrismaddern chrismaddern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style change but otherwise seems sane -- thanks for an awesome contribution.

@ioswes can you take a look as you're the owner?

- pod install
script:
- xctool test -workspace DeepLinkKit.xcworkspace -scheme ReceiverDemo -sdk iphonesimulator ONLY_ACTIVE_ARCH=NO
- travis_retry xcodebuild build test -workspace DeepLinkKit.xcworkspace -scheme ReceiverDemo -sdk iphonesimulator -configuration Debug -destination 'platform=iOS Simulator,name=iPhone 7,OS=latest'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't believe that exists! Haha

.travis.yml Outdated
branches:
only:
master
before_install:
- bundle
# - gem install xcpretty --no-rdoc --no-ri --no-document --quiet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill commented code? Tidy Travis YML file, tidy mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem

@@ -1,5 +1,8 @@
@import DeepLinkKit.Private;
@import DeepLinkKit.AppLinks;
#import "DPLDeepLink.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What prevents you importing the module here? This will still work externally, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you haven't got subspecs defined, so I don't see the point... CP generates an umbrella header for it all and you can do @import DeepLinkKit.AppLinks;. I updated the test to use it like that.

@igor-makarov
Copy link
Contributor Author

All done, please merge!

@salabaha
Copy link

@igor-makarov thanks for the fixes. @chrismaddern any estimation when this pr can be merged?

@yusuftor
Copy link

Just downloaded Xcode 8.3 and needing this branch merged!

@wessmith
Copy link
Member

Thanks for this @igor-makarov!

@wessmith wessmith dismissed chrismaddern’s stale review March 30, 2017 15:51

Requested changes are corrected.

@wessmith wessmith merged commit 1c239a3 into button:master Mar 30, 2017
@igor-makarov
Copy link
Contributor Author

Awesome! Please tag out a new release for the benefit of @salabaha @yusuftor @Unihilator and also @taher-mosbah @superpeteblaze @Ekhoo from #119

@wessmith
Copy link
Member

Available in 1.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants