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

Themis CocoaPods now support CLOpenSSL-XCF #828

Merged
merged 11 commits into from
May 26, 2021

Conversation

julepka
Copy link
Contributor

@julepka julepka commented May 26, 2021

Release 0.13.10

Now CocoaPods will use CLOpenSSL-XCF that contains the latest version of OpenSSL 1.1.1k.
We no longer need a workaround for arm64 simulator for subspec with CLOpenSSL-XCF.
Cherry-picked latest Carthage examples to fix tests in this branch.

Example projects will be updated afterwards, so they can reference 0.13.10 podspec

Also, just FYI there are thoughts to drop GRKOpenSSLFramework support as there are no updates from them for a long time... It may be an item to consider for 0.14

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Example projects and code samples are up-to-date (in case of API changes)
  • Changelog is updated (in case of notable or breaking changes)

@julepka julepka added O-iOS 📱 Operating system: iOS M-CocoaPods Package manager: CocoaPods, Objective-C and Swift, iOS and macOS labels May 26, 2021
@julepka julepka changed the title Themis cocoapods xcf Themis CocoaPods now support CLOpenSSL-XCF May 26, 2021
@julepka julepka requested a review from ilammy May 26, 2021 09:46
Copy link
Collaborator

@ilammy ilammy left a comment

Choose a reason for hiding this comment

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

I believe cherry-picks should be out of this PR, so that when squash-merged it contains only changes related to XCF migration for CocoaPods.

I sometimes do cherry-picks with just git cherry-pick -x -S commits... and push directly into target branch if the changes are not controversial and does not require conflict resolution. Otherwise a separate PR would be better, linking to original PRs.

CHANGELOG.md Outdated Show resolved Hide resolved
@ilammy
Copy link
Collaborator

ilammy commented May 26, 2021

Regarding GRKOpenSSL, I believe the CLOpenSSL support appear somewhere in 0.13.x release but was not a part of 0.13.0. There was also no deprecation notice for GRKOpenSSL, so I think it would not be nice to just drop it immediately.

At the same time, 0.13.x branch can serve as a fallback. So even if it's not technically nice, and we should have planned it better, I think it would not be a major issue for users if we drop themis/themis-openssl subspec right in 0.14.0 on a short notice. Most of them should be using just themis which is still okay.

So it might be acceptable to issue a deprecation notice now, in 0.13.10, then drop it in 0.14.0. Those who still need old subspecs should continue using 0.13.10 or earlier, until they are ready to migrate to 0.14.x – which should be a matter of replacing themis/themis-openssl with themis, and maybe readding GRKOpenSSL somehow if their process is weird.

While we're at it, maybe it's time to drop themis/themis-boringssl as well? BoringSSL pod has not been updated in ages. I don't believe we are providing much value by shipping this flavor.

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

Thank you, @julepka!

I suggest doing 2 PRs: updating Carthage examples first (merge first), supporting CCPs XCFrameworks (merge second).

  • updating examples is a simple "grab from master" update
  • adding XCF for CCP is a real hotfix change for 0.13.10, which should be a separate PR in case we need to revert it

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@julepka
Copy link
Contributor Author

julepka commented May 26, 2021

Regarding GRKOpenSSL, I believe the CLOpenSSL support appear somewhere in 0.13.x release but was not a part of 0.13.0. There was also no deprecation notice for GRKOpenSSL, so I think it would not be nice to just drop it immediately.

At the same time, 0.13.x branch can serve as a fallback. So even if it's not technically nice, and we should have planned it better, I think it would not be a major issue for users if we drop themis/themis-openssl subspec right in 0.14.0 on a short notice. Most of them should be using just themis which is still okay.

So it might be acceptable to issue a deprecation notice now, in 0.13.10, then drop it in 0.14.0. Those who still need old subspecs should continue using 0.13.10 or earlier, until they are ready to migrate to 0.14.x – which should be a matter of replacing themis/themis-openssl with themis, and maybe readding GRKOpenSSL somehow if their process is weird.

I agree. Let's deprecate 'themis-openssl' with GRKOpenSSL. I'm looking for the best option to give a notice about that. Poscpec syntax doesn't support subspec deprecation. I don't see a way to notify the user when running 'pod install' or 'pod update'. I think we can just make a notice in Changelog, Release Notes and in public docs.

While we're at it, maybe it's time to drop themis/themis-boringssl as well? BoringSSL pod has not been updated in ages. I don't believe we are providing much value by shipping this flavor.

I've just checked it and it seems that there is still some life in their repo. But yeah... its pod was not updated since 2018 I guess... I think we can deprecate it as well.

themis.podspec Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@ilammy
Copy link
Collaborator

ilammy commented May 26, 2021

@julepka,

I've just checked it and it seems that there is still some life in their repo. But yeah... its pod was not updated since 2018 I guess... I think we can deprecate it as well.

BoringSSL repo is alive and kicking, of course. However, they don't seem to be maintaining the podspec anymore. It has been submitted by a person from Google, sure, but I guess they got their promotion and it ended with that 🤷

@vixentael,

CCPs

My tiny head can hold only so many acronyms. There are already two CCPs there (one from China and one from Iceland), and I doubt I can fit any more...

@ilammy ilammy self-requested a review May 26, 2021 14:37
@vixentael vixentael self-requested a review May 26, 2021 14:42
@ilammy ilammy self-requested a review May 26, 2021 14:43
themis.podspec Show resolved Hide resolved
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

So, let's finalise wording on deprecation and Changelog, make sure that Apple-related tests pass, and merge

@vixentael
Copy link
Contributor

CI machine can't find python six package again (see #829 (comment)). I use my sudo powers to merge this PR.

@vixentael vixentael merged commit d869a4f into cossacklabs:release/0.13 May 26, 2021
julepka added a commit that referenced this pull request May 27, 2021
* Carthage XCF support and OpenSSL 1.1.1k (#817)
* Update Carthage examples to unbreak tests (#829)
* Cherry-pick: Updated Carthage examples to use XCF (#823)
* Fixing Carthage examples embed settings (#827)
* fix changelog conflict
* Themis CocoaPods now support CLOpenSSL-XCF (#828)
* Updated podspec to support OpenSSL-XCF
* versions update
* update changelog
* Cherry-pick: Updated Carthage examples to use XCF (#823)
* Fixing Carthage examples embed settings (#827)
* Revert "Fixing Carthage examples embed settings (#827)"
This reverts commit d9403c5.
* Revert "Cherry-pick: Updated Carthage examples to use XCF (#823)"
This reverts commit 6c14677.
* update changelog
* added deprecation notice to podspec and changelog
* update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-CocoaPods Package manager: CocoaPods, Objective-C and Swift, iOS and macOS O-iOS 📱 Operating system: iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants