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

Changes for Swift3 #49

Merged
merged 6 commits into from
Oct 11, 2016
Merged

Changes for Swift3 #49

merged 6 commits into from
Oct 11, 2016

Conversation

rajeevpra
Copy link
Contributor

Most of the changes are created automatically by xcode.. I had fix few issues after that. I ran all the tests successfully. Please make sure to have simulator is set to ios 9.x. It will fail on 10.x because of bug in new simulator.

@rajeevpra
Copy link
Contributor Author

This failure seems to be because its trying to build with xcode7.1, this needs to be built with 8.0. Let me know if I need to update something.

@rajeevpra
Copy link
Contributor Author

Test is failing because it is trying to run it on iOS 10 simulation and as I mentioned it will fail because of the issue #47. I need help to update config in way to make it not run on ios10. Please help.

@henrinormak
Copy link
Owner

I assume this is now actually good enough to be merged. Unfortunately, I will not be able to push the new release to cocoapods in a while, but I could still merge this. Thoughts?

@jorystiefel
Copy link
Collaborator

jorystiefel commented Sep 26, 2016

@rajeevpra I don't think it makes sense to run the tests on an outdated version of iOS, as that's not really proving that the code works. You can make these tests work in iOS 10 by creating a dummy app target with Keychain Sharing enabled, and setting the tests to run "On Host App" with that app.

@rajeevpra
Copy link
Contributor Author

I agree @jorystiefel. I will give it a try when I get chance. It can be argued that it is still useful for people to have it as this only affects ios 10 simulator not the device.
@henrinormak I would vote merge it anyways. Let me know if can help you with release to cocoapods.

@jorystiefel
Copy link
Collaborator

Another note I feel worth mentioning: It seems this branch of Heimdall is not backwards compatible with the current master. We could not get our client/server to successfully talk to each other until both were on the Swift 3 branch.

@henrinormak
Copy link
Owner

I will be unavailable to discuss/test/merge this for a while as I am away from my dev machine and lack the necessary tools. Two things to note:

  1. New changes must remain backwards compatible, so either there is a bug in one of them that needs fixing, or there needs to be a bump in the major version to signify non-backwards compatibility.
  2. Tests must pass on the latest SDK. There might be problems with importing the system libraries, but that can be fixed, I am sure.

@henrinormak
Copy link
Owner

One last thing, based on this discussion, I am willing to give someone else merge rights to master, as well as cocoapods. This is to speed up the process of adding Swift 3 support. @jorystiefel would you be interested in this?

@jorystiefel
Copy link
Collaborator

@henrinormak Sure, I'd be happy to help.

@rchatham
Copy link

rchatham commented Oct 7, 2016

Definitely looking to use the swift 3 version soon! I am using it to add RSA to https://github.com/kylef/JSONWebToken.swift. Currently I'm having issues pointing the pod dependency to @rajeevpra's repository and it seems like having it is due to only being able to use official pod releases.

@henrinormak
Copy link
Owner

henrinormak commented Oct 9, 2016

Added @jorystiefel as a collaborator to the repo, I would also add you to the cocoapod as an owner, but I need your email for that

@henrinormak henrinormak mentioned this pull request Oct 9, 2016
@jorystiefel
Copy link
Collaborator

@henrinormak thank you, my email is jorystiefel at me.com

@henrinormak
Copy link
Owner

Have you registered that email with Cocoapods? It claims that you have not, if so, could tou, so that I could also add you as a contributor there

@jorystiefel
Copy link
Collaborator

All set.

@henrinormak
Copy link
Owner

Done on my end, I'll leave merging/testing/verifying this up to you. Once done please also push a new release to Cocoapods

@jorystiefel
Copy link
Collaborator

@rajeevpra @rchatham I'm going to merge this soon. I've been testing this extensively over the past week.

The break in compatibility I noticed was due to the padding change that happened in April, not due to the Swift 3 changes. I will make updates to make the tests work on iOS 10 simulator after this is merged.

@jorystiefel jorystiefel merged commit a4249e8 into henrinormak:master Oct 11, 2016
@mayooresan
Copy link

is it available in cocoapods yet?

@henrinormak
Copy link
Owner

I don't think it is, @jorystiefel could you also update the pod? You need to update the podfile withnew version number, but that's pretty much it.

@jorystiefel
Copy link
Collaborator

Yes, coming today.

@hools
Copy link

hools commented Oct 13, 2016

thanks for doing this! much appreciated =)
can you post when the pod is updated?

@rchatham
Copy link

Please pod trunk push! Thank you!!

@jorystiefel
Copy link
Collaborator

@rchatham I'm having problems with CocoaPods crashing, sorry

@rchatham
Copy link

What's wrong? Have you tried running pod lint?

@jorystiefel
Copy link
Collaborator

@rchatham @hools the lint error I'm getting is:

error: no such module 'CommonCrypto'

Any ideas?

@rchatham
Copy link

From what I can find you will need to add a script like the one below,

https://github.com/iosdevzone/IDZSwiftCommonCrypto/blob/master/GenerateCommonCryptoModule.swift

And run it in the .podspec.

https://github.com/iosdevzone/IDZSwiftCommonCrypto/blob/master/IDZSwiftCommonCrypto.podspec

It gets mentioned it in the beginning of the realm talk here https://realm.io/news/danny-keogan-swift-cryptography/.

@duyleekun
Copy link

@rchatham I followed your guidance, it fixed the CommonCrypto issue

master...dworkvn:fix/missing-common-ctypto

@adrianbrink
Copy link

@jorystiefel The build from cocoapods is broken. Have you merged the PR?

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

Successfully merging this pull request may close these issues.

8 participants