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.

Sign up for GitHub

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

Jump to bottom

Multiple platform support #23

Merged
merged 1 commit into from
Jul 26, 2016
Merged

Multiple platform support #23

merged 1 commit into from
Jul 26, 2016

Conversation

greenantdotcom
Copy link
Contributor

  • Added additional targets for watchOS and tvOS platforms
  • Tests pass on tvOS
  • Changed the master SwiftyRSA.h file to pull in Foundation and not UIKit/Cocoa since those frameworks aren't needed

Also includes the work sent in on #22, which I know is a faux pas, so I can resubmit "clean" if you prefer and/or reject #22.

Thanks!

@ldiqual
Copy link
Contributor

ldiqual commented Jul 14, 2016

@greenantdotcom Thanks a lot for both your PRs! I'll take a look asap. As suggested, would you mind keeping those two changes separate? It'll make the process easier.

@greenantdotcom greenantdotcom force-pushed the multiple-platform-support branch 2 times, most recently from f97bc13 to 9a060c7 Compare July 14, 2016 00:51
@greenantdotcom
Copy link
Contributor Author

This should be a clean patch set now!

@greenantdotcom greenantdotcom force-pushed the multiple-platform-support branch 3 times, most recently from 28ae009 to 672bfcf Compare July 15, 2016 22:33
@greenantdotcom
Copy link
Contributor Author

greenantdotcom commented Jul 15, 2016 •
edited
Loading

@ldiqual FYI - I was having a really tough time getting my CI tests to pass, and I think it's because of the multiple schemes now for each of the three platforms.

I've refactored the CI away from scan and toward a handrolled CI script a bit like SwiftyJSON's but a little more open for improvement.

@ldiqual
Copy link
Contributor

ldiqual commented Jul 18, 2016

@greenantdotcom Thanks a lot for taking time to make the tests pass. A couple comments to get this review going:

  • Have you explored the Alamofire way of using Travis on multiple platforms? https://github.com/Alamofire/Alamofire/blob/master/.travis.yml I would like to avoid having a build script, in my experience those break pretty easily and are highly CI dependent -- mainly regarding environment variables. I'm ok with moving away from scan if you feel like it's not flexible enough. Also, I'm concerned that your script doesn't test for iOS 8.0 support.
  • Can you also modify the podspec and add the new platforms in there?
  • I see a couple whitespace changes. Can you revert those? They have a tendency to pollute the commit history and it makes it harder to git bisect or git blame.

Awesome work, thanks so much for contributing!

@greenantdotcom greenantdotcom force-pushed the multiple-platform-support branch 5 times, most recently from 6639fa4 to 81c601b Compare July 19, 2016 01:29
@greenantdotcom
Copy link
Contributor Author

  • Whitespace changes should be removed
  • Podspec updated to have 8.3 as the lowest supported iOS version, as well as the new platforms (per TravisCI support in Xcode 7.3)
  • Adopted Alamofire style for builds, which is CI'ing clean now

Per https://developer.apple.com/support/app-store/, I don't think 8.x is highly used, but I'm certain there are some real use cases like enterprise/school deployments that might not fully upgraded - question is whether this is a deal breaker, or whether it can be assimilated into #19 separately.

Thanks!

ldiqual
ldiqual reviewed Jul 19, 2016
View reviewed changes
.travis.yml

# Build Framework in Debug and Run Tests if specified
- if [ $RUN_TESTS == "YES" ]; then
Copy link
Contributor

@ldiqual ldiqual Jul 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Let's just test every time. You can remove the if and the second body,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ldiqual: It's an unfortunate need for watchOS which isn't test targetable - otherwise, it would definitely be simpler… It's the same as Alamofire (https://github.com/Alamofire/Alamofire/blob/master/.travis.yml)

@ldiqual
Copy link
Contributor

ldiqual commented Jul 19, 2016

@greenantdotcom Thanks for updating your PR.

  • Re iOS 8.0 - 8.3: let's treat this as a separate issue. Actually there's one open already: Please provide support for iOS 8.0 #19
  • I added a couple comments inline, just around optionality of testing.

The PR looks great. I'll merge it after you remove the test flags.

@greenantdotcom
Added targets for watchOS and tvOS
81ae771 
- Added additional targets for watchOS and tvOS platforms
- Tests pass on tvOS
- Changed the master `SwiftyRSA.h` file to pull in Foundation and not UIKit/Cocoa since those frameworks aren't needed
- The current CI pipeline doesn't assume multiple targets/schemes, so I refactored `.travis.yml` to allow for AppleTV, iOS, and watchOS targets, and based on how Alamofire does theirs for multiple platform support
- Updated `podspec` to show multiple platform support
    - Also raised iOS version to 8.3 since Travis platform doesn't seem to have support for iOS 8.0-8.2
@greenantdotcom greenantdotcom force-pushed the multiple-platform-support branch from 81c601b to 81ae771 Compare July 22, 2016 23:42
@ldiqual
Copy link
Contributor

ldiqual commented Jul 26, 2016

@greenantdotcom Thanks a lot! We're good to go. Merging now.

@ldiqual ldiqual merged commit 47d5236 into TakeScoop:master Jul 26, 2016
ldiqual added a commit that referenced this pull request Sep 15, 2016
@ldiqual
Swift 3.0 changes (#20)
ec89616 
* Make SwiftyRSA compile with Swift 3.0

* Fixed all errors + warnings

* Fix spelling

* Added support to read multiple public keys from a PEM file (#22)

For a usecase where we may have multiple public keys that our content might have been signed with, we wanted to put multiple public keys into a single file, much like you may have multiple SSL certs in a chain file.

This method parses out any data between `-----BEGIN PUBLIC KEY-----`  and `-----END PUBLIC KEY-----`, and then uses `publicKeyFromPEMString` to try to parse it.

Tests added for a really insane usecases, an empty string, and reading one of the test private keys for fun. All added code has 100% coverage, and the new method is commented.

* Added targets for watchOS and tvOS (#23)

- Added additional targets for watchOS and tvOS platforms
- Tests pass on tvOS
- Changed the master `SwiftyRSA.h` file to pull in Foundation and not UIKit/Cocoa since those frameworks aren't needed
- The current CI pipeline doesn't assume multiple targets/schemes, so I refactored `.travis.yml` to allow for AppleTV, iOS, and watchOS targets, and based on how Alamofire does theirs for multiple platform support
- Updated `podspec` to show multiple platform support
    - Also raised iOS version to 8.3 since Travis platform doesn't seem to have support for iOS 8.0-8.2

* Updated changelog

* Bump to 0.4.0

* Update to Xcode 8.0 beta 4

* Migrate to Xcode 8 beta 6

* Don't reduce maximum block size when padding is `none`
Closes #29

* Update CHANGELOG.md

#29

* Fix OSStatus -34018 on iOS 10

* Update travis to support xcode 8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@greenantdotcom @ldiqual