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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update iOS wrapper header structure to compile Swift framework #415

Merged
merged 6 commits into from Mar 6, 2019

Conversation

Projects
None yet
2 participants
@vixentael
Copy link
Member

commented Mar 6, 2019

Compiling Swift framework (f.e. adding Themis as dependency into some Swift framework) requires using modular framework.

This change aims to solve #411, there you can find more details about modular frameworks.

Modular frameworks don't expose non-modular headers as public. It's quite a tricky issue, because of non-trivial themis-soter source code structure and how it's exposed to the objc wrapper files. Previously we lived happily, because most users linked Themis from Bridging Header :)

What was changed:

  • remove extra headers in objc wrapper code, especially remove large headers like #import <themis/themis.h>
  • move many headers from .h files to .m files
  • use local #import "scell_seal.h" imports instead of #import <objcthemis/scell_seal.h>
  • (workaround) to remove themis secure session callback headers, I re-defined one structure typedef struct secure_session_user_callbacks_type secure_session_user_callbacks_t; here (not sure if this is a best thing to do, but I couldn't imagine any other quick way to remove themis import from header).

How to test:

Example project linked Themis as import themis from Swift code, which triggers Xcode to build Swift (modular) framework, and here we are.

Updated podspec:

  • Podspec was updated to hide as much as possible headers, see next PR.

Next steps:

Upon testing and merging, podspec will be updated to link to the master branch (next PR), all example projects will be updated, podspec will be pushed.

P.S. I have tested on real device. Works fine 馃拋

@vixentael vixentael added the ios label Mar 6, 2019

@vixentael vixentael self-assigned this Mar 6, 2019

@vixentael vixentael requested review from ilammy and Lagovas Mar 6, 2019

@@ -7,13 +7,13 @@
//
import UIKit
import themis

This comment has been minimized.

Copy link
@vixentael

vixentael Mar 6, 2019

Author Member

line that triggers compiling Swift framework :)

@@ -1,14 +1,14 @@
#ifndef _OBJCTHEMIS_
#define _OBJCTHEMIS_

#import <objcthemis/scell_seal.h>

This comment has been minimized.

Copy link
@vixentael

vixentael Mar 6, 2019

Author Member

real changes start from here

@@ -19,15 +19,13 @@
* @brief secure session trancport callbacs interface
*/
#import <Foundation/Foundation.h>
#import <themis/themis.h>

typedef struct secure_session_user_callbacks_type secure_session_user_callbacks_t;

This comment has been minimized.

Copy link
@vixentael

vixentael Mar 6, 2019

Author Member

i dunno what is the better way to remove header, but to expose callback, and not to break backwards-compatibility for current users.

This comment has been minimized.

Copy link
@ilammy

ilammy Mar 6, 2019

Contributor

Do we need to avoid all and any <themis/> headers? If not then I'd think that a proper way to avoid duplication would be to introduce some (private) <themis/types.h> header to keep all our typedefs and include it here.

Though since we have only this typedef to take care of I think it is fine to leave it as is. If we decide to remove or rename the underlying structure then ssession_transport_interface.m will stop compiling so this typedef can't get out of sync with the main header. (And I guess the compiler will warn us about conflicting typedefs if they do not match.)

However, I think that Obj-C users have never actually used the callback struct (as they don't have any reason to). So I guess this typedef can be safely removed. Since _callbacks ivar is no longer a public, any code that actually used it should fail to compile now 鈥 this is a breaking change, I'd imagine.

This comment has been minimized.

Copy link
@vixentael

vixentael Mar 6, 2019

Author Member

_callbacks is not a public property, and it's not supposed to be accessed "as is", so I safely moved it into .m file here

https://github.com/cossacklabs/themis/pull/415/files/21bc76aa0a906b44e5542d9ed4a5acf6a18fcd6b#diff-a5dd6093239a6c2087d20d0b49978d9fR22

Basically, _callbacks are set during init only.

_callbacks getter is a method - (secure_session_user_callbacks_t *)callbacks;

And you're right, these things are used in SecureSession wrapper only (here), users don't use them, so I believe it's not a breaking changes.

@ilammy

ilammy approved these changes Mar 6, 2019

Copy link
Contributor

left a comment

LGTM 馃憤

Nice job at figuring out all these modules, frameworks, etc.

#import "smessage.h"
#import "scomparator.h"
#import "ssession.h"
#import "serror.h"

This comment has been minimized.

Copy link
@ilammy

ilammy Mar 6, 2019

Contributor

I'm a bit confused wrt the usage of <objcthemis/serror.h> vs "serror.h". Shouldn't it be backwards instead?

  • <objcthemis/serror.h> should be used in header files as that's what included into the user code
  • "serror.h" should be used in source files to ensure that we compile with headers that are nearby

This comment has been minimized.

Copy link
@vixentael

vixentael Mar 6, 2019

Author Member

Are you confused with any of this change <objcthemis/smessage.h> vs "smessage.h" or only with serror.h?
Because for me it's also a bit weird: this thing is working properly for ObjC, but these headers are "non-modulars" for Swift frameworks.

This comment has been minimized.

Copy link
@ilammy

ilammy Mar 6, 2019

Contributor

All of them, not just serror.h.

I have a feeling that we have to use "serror.h" style here because we copy the headers as a flat list when constructing the framework. I remember seeing in Xcode that all of the files are just dumped into the framework source without preserving the subdirectories. Though, we seem to use this 'subspec' thing of Cocoapods which should group files properly...

This comment has been minimized.

Copy link
@vixentael

vixentael Mar 6, 2019

Author Member

yes, somehow Xcode/Cocoapods don't pay attention on subdirectories, and "umbrella" header also consists of "flat" headers 炉_(銉)_/炉
well, that's what we have here..

@vixentael vixentael merged commit 4d453c8 into master Mar 6, 2019

8 checks passed

ci/bitrise/b32b4ea8bffedad7/push Passed - themis
Details
ci/circleci: analyze Your tests passed on CircleCI!
Details
ci/circleci: android Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: php5 Your tests passed on CircleCI!
Details
ci/circleci: php70 Your tests passed on CircleCI!
Details
ci/circleci: php71 Your tests passed on CircleCI!
Details
ci/circleci: x86_64 Your tests passed on CircleCI!
Details

@vixentael vixentael deleted the vxtl/ios-header branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.