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

Make CommonCrypto dependency work for both Swift <4.2 and Swift 4.2 (CommonCrypto is public) #190

Closed

Conversation

ziogaschr
Copy link
Contributor

In Swift 4.2 and Xcode 10.0 the CommonCrypto framework became publicly available in iOS. For this reason, we had to remove the modulemap. For doing this, while keeping backward compatibility with later versions, we used an elegant method from @radianttap, who used a Build phase to check for CommonCrypto existence.

p.s.: Thanks @radianttap. I hope you are fine that I used your elegant handler.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #190 into develop will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #190      +/-   ##
===========================================
- Coverage    97.22%   97.08%   -0.15%     
===========================================
  Files            6        6              
  Lines          397      377      -20     
===========================================
- Hits           386      366      -20     
  Misses          11       11
Impacted Files Coverage Δ
Sources/Generator.swift 100% <0%> (ø) ⬆️
Sources/Token.swift 100% <0%> (ø) ⬆️
Sources/PersistentToken.swift 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09b940e...47cacac. Read the comment docs.

@mattrubin
Copy link
Owner

Thanks for the PR, @ziogaschr! The changes were failing to build on Xcode 9.x, so I made a small change and pushed it to your branch to get the CI builds passing.

I like this new method of generating a modulemap at compile time only if necessary, rather than maintaining and linking external modulemap files. Before merging this PR, I'm going to test this new approach with Carthage and CocoaPods to make sure it works for all use cases.

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #190 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #190   +/-   ##
========================================
  Coverage    97.22%   97.22%           
========================================
  Files            6        6           
  Lines          397      397           
========================================
  Hits           386      386           
  Misses          11       11
Impacted Files Coverage Δ
Sources/Crypto.swift 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09b940e...47cacac. Read the comment docs.

Repository owner deleted a comment from codecov bot Sep 19, 2018
Repository owner deleted a comment from codecov bot Sep 19, 2018
Repository owner deleted a comment from codecov bot Sep 19, 2018
@ziogaschr
Copy link
Contributor Author

Oh nice catch @mattrubin, I tried to see if the patch works in another 3rd party lib and it worked nicely in Xcode 9, so I thought of just copy it here without testing it below Xcode10. Nice lesson for me, to not trust any code, likely Travis catch it.

@mattrubin
Copy link
Owner

I was never quite able to get this new approach working when building with CocoaPods, and now I've just updated the whole project to build with Xcode 10, removing the need for the modulemap shims. Thank you to @ziogaschr for the PR!

@mattrubin mattrubin closed this Apr 27, 2019
@ziogaschr
Copy link
Contributor Author

I am glad there is a better way of doing this @mattrubin. Thx for maintaining this lib.

@mattrubin mattrubin added this to the 3.1.6 milestone Sep 18, 2019
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.

2 participants