-
Notifications
You must be signed in to change notification settings - Fork 144
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
Proper Carthage project: ObjCThemis #604
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Our first attempt at providing Carthage support was a bit haphazard and resulted in an Xcode project that builds ObjCThemis into a framework called "themis.framework". This has been done in order to preserve the module name for Swift the same as with CocoaPods. However, this also means that Objective-C has to import this framework as #import <themis/themis.h> while CocoaPods use #import <objcthemis/objcthemis.h> This discrepancy is not ideal. First of all, this wart^W special case complicates integration of Themis into projects. We have it described in documentation, but that's an excuse, not a solution. Then, this import conflicts with Themis Core (which also uses <themis/themis.h>). Finally, *we* are also affected by this discrepancy because ObjCThemis test suite uses <objcthemis/objcthemis.h>, making it impossible to run existing unit tests on Carthage project. Provide an alternate Xcode project which builds ObjCThemis into "objcthemis.framework" and keeps Swift module name "themis". This is how it should have been done from the start. Now, ObjCThemis installed via Carthage can be imported in Objective-C projects as #import <objcthemis/objcthemis.h> That is the same import when ObjCThemis is installed via CocoaPods. Older Objective-C import syntax is still accepted for now but it is considered deprecated and will be removed. Swift import syntax stays unchanged: import themis However, aside from imports the users will also need to update their projects to link against "objcthemis.framework" instead of "themis.framework". Note that this is a separate Xcode project, not an additional pair of targets in existing Themis.xcodeproj. We have to make a new project because of this very issue with conflicting names. It is impossible to add another target that builds ObjCThemis because it uses #import <themis/themis.h> to import Themis Core. This works when building "themis.framework" itself, but for any other target in the project this syntax will import "themis.framework", not Themis Core. Sibling projects are included in the search path automatically so there is no way to prevent this conflict other than making a completely separate Xcode project. Multiple projects are supported by Carthage. It will just build them all. This means that the users will be building ObjCThemis twice if they are using the simple carthage bootstrap Unfortunate, but that's how it will be until the next release when we will be able to drop "themis.framework" (the only alternative is to break compatibility immediately). Please accept my sincere apologies for increased CI bills. This was my mistake from the start.
ilammy
added
W-SwiftThemis 🔶
Wrapper: SwiftThemis, Swift API
compatibility
Backward and forward compatibility, platform interoperability issues, breaking changes
installation
Installation of Themis core and wrapper packages
W-ObjCThemis 🎨
Wrapper: ObjCThemis, Objective-C API
M-Carthage
Package manager: Carthage, Objective-C and Swift, iOS and macOS
labels
Mar 14, 2020
Honestly saying, I hate renaming things. So, TLDR, not depending on package mananger:
right? ————- Also, can we please make sure that we have internal tasks for:
after/before/during 0.13 release? |
vixentael
approved these changes
Mar 19, 2020
Co-Authored-By: vixentael <[email protected]>
So, TLDR, not depending on package mananger:
- for ObjC, people will link #import <objcthemis/objcthemis.h>
- for Swift, people will link import themis
right?
Yes, that's correct. This is what #427 failed to do initially.
Also, can we please make sure that we have internal tasks for:
- updating ObjC Themis guide on docserver
- updating Swift Themis guide on docserver
- updating Themis examples
after/before/during 0.13 release?
Or, ideally, “instead of” 😂
Sure, done.
…--
Best regards,
Alexei Lozovsky
|
Merge 'em all! |
I've checked this manually just in case: Bitrise does build the new Xcode project. So we're safe 🤞 |
This was referenced Aug 14, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
compatibility
Backward and forward compatibility, platform interoperability issues, breaking changes
installation
Installation of Themis core and wrapper packages
M-Carthage
Package manager: Carthage, Objective-C and Swift, iOS and macOS
W-ObjCThemis 🎨
Wrapper: ObjCThemis, Objective-C API
W-SwiftThemis 🔶
Wrapper: SwiftThemis, Swift API
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our first attempt at providing Carthage support (#427) was a bit haphazard and resulted in an Xcode project that builds ObjCThemis into a framework called
themis.framework
. This has been done in order to preserve the module name for Swift the same as with CocoaPods. However, this also means that Objective-C has to import this framework aswhile CocoaPods use
This discrepancy is not ideal. First of all, this
wartspecial case complicates integration of Themis into projects. We have it described in documentation, but that's an excuse, not a solution. Then, this import conflicts with Themis Core (which also uses<themis/themis.h>
). Finally, we are also affected by this discrepancy because ObjCThemis test suite uses<objcthemis/objcthemis.h>
, making it impossible to run existing unit tests on Carthage project. (Well, without silly preprocessor tricks, that is.)Provide an alternate Xcode project which builds ObjCThemis into
objcthemis.framework
and keeps Swift module namethemis
. This is how it should have been done from the start. Now, ObjCThemis installed via Carthage can be imported in Objective-C projects asThat is the same import when ObjCThemis is installed via CocoaPods.
Older Objective-C import syntax is still accepted for now but it is considered deprecated and will be removed.
Swift import syntax stays unchanged (for both Carthage and CocoaPods):
However, aside from imports the users will also need to update their projects to link against
objcthemis.framework
instead ofthemis.framework
.Note that this is a separate Xcode project, not an additional pair of targets in existing Themis.xcodeproj. We have to make a new project because of this very issue with conflicting names. It is impossible to add another target that builds ObjCThemis because it uses
to import Themis Core. This works when building
themis.framework
itself, but for any other target in the project this syntax will importthemis.framework
, not Themis Core. Sibling projects are included in the search path automatically so there is no way to prevent this conflict other than making a completely separate Xcode project.Multiple projects are supported by Carthage. It will just build them all. This means that the users will be building ObjCThemis twice if they are using the simple
Unfortunate, but that's how it will be until the next release when we will be able to drop
themis.framework
(the only alternative is to break compatibility immediately).Please accept my sincere apologies for increased CI bills. This was my mistake from the start.
Checklist
Change is covered by automated tests1Example projects and code samples are up-to-date21 Bitrise probably will not automatically test this, but I remember something about Carthage there.
WhenIf this gets merged, I'll look at it in detail (and update GitHub Actions too).2 No, the code samples are not updated in this PR. Since we pin them to a particular released version of Themis, they will have to be updated after we make a release which this changes. GitHub Actions will test that the examples compile unchanged with updated Themis. We could also add a specific test for this, but that's too much of a bother. I really don't want four more projects there.