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

[TIMOB-25927] Add Swift support for native modules, move SDK-core into "TitaniumKit.framework" #9983

Closed
wants to merge 36 commits into from

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn commented Apr 5, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-25927


⚠️If accepted, it will break native modules to be recompiled using TitaniumKit.framework instead of referencing single header files. It also removes the kroll thread and custom TiCore completely.

⚠️Highly work in progress, requires major CLI, SDK and internal library (APSAnalytics & APSHTTPClient) changes

🗓 Scheduled for Q3/Q4


Overview

Replaces the Titanium iOS Core with framework based alternative that is required if we want to deal with native modules that use Swift libraries, as we cannot bake Swift frameworks into static libraries and Swift frameworks do not allow non modular imports. Especially not with our (currently) hacky solution that attempts to reference symlinks from the application data directory. The following tasks represent the base of this effort.

SDK Tasks

  • Create universal TitaniumKit.framework
  • Move SDK-core to TitaniumKit.framework
  • Remove Kroll Thread (use Main Thread)
  • Remove TiCore (use native JSCore)
  • Reference TitaniumKit as a target dependency of the core
  • Integrate TitaniumKit into the build pipeline
  • Update the CLI to copy TitaniumKit files into the packaged SDK
  • Update the CLI to copy TitaniumKit files into generated project build directory
  • Be able to recompile core-framework if compiler flags change (e.g. IS_XCODE_*). Can we use OTHER_C_FLAG=$(inherited) -DISXCODE_*?
  • Create .podspec for usage with CococaPods

Module / CLI Tasks

  • Create framework based module template (embedded Swift framework)
  • Reference TitaniumKit locally
  • Test common Module API's (String, Number, Array, Dictionary, Date, id/Any)
  • Update module build
    • Compile (universal) framework instead of static library
    • Package framework instead of static library
    • Link framework based modules in SDK core (update _build.js validateModules step)
    • Keep backwards compatibility for Obj-C based modules (which still need to be recompiled)
    • Update templates to include a Swift module template. Prompt to choose between Swift and ObjC-module
    • Be able to link TitaniumKit into module projects without actually linking the framework?
    • Update Obj-C module template to use TitaniumKit as well
    • Link frameworks to the "Embedded Frameworks" section (adjust the framework-manager from @janvennemann to look for it?)
    • Write migration script to fix up old project imports -> RegEx replace for imports, link TitaniumKit, eventually symlink TitaniumKit to prevent breaking change?

@build build added the ios label Apr 5, 2018
@hansemannn hansemannn changed the title [TIMOB-25927] Add Swift support for native modules, introduce TitaniumKit.framework [TIMOB-25927] Add Swift support for native modules, move SDK-core into "TitaniumKit.framework" Apr 9, 2018
@build build added the ios label Apr 9, 2018
@build
Copy link
Contributor

build commented Apr 9, 2018

Fails
🚫

😥 npm test failed. See below for details.

Messages
📖
> titanium-mobile@8.0.0 test /Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ
> grunt

Running "appcJs:src" (appcJs) task

Running "eslint:src" (eslint) task

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/build/ios.js
 3:7  warning  'exec' is assigned a value but never used                no-unused-vars
10:2  warning  'copyAndModifyFiles' is assigned a value but never used  no-unused-vars
11:2  warning  'globCopy' is assigned a value but never used            no-unused-vars

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/build/scons-modules-integrity.js
9:12  warning  Found non-literal argument in require  security/detect-non-literal-require

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/cli/commands/create.js
154:1  error  Trailing spaces not allowed  no-trailing-spaces

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/cli/lib/creator.js
249:3   warning  'config' is assigned a value but never used  no-unused-vars
250:16  warning  A space is required after '['                array-bracket-spacing
250:32  warning  A space is required before ']'               array-bracket-spacing
543:1   error    Trailing spaces not allowed                  no-trailing-spaces
545:61  error    Trailing spaces not allowed                  no-trailing-spaces

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/cli/lib/creators/module.js
98:3  warning  'codeBase' is assigned a value but never used  no-unused-vars

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/android/cli/commands/_buildModule.js
554:18  warning  Found non-literal argument to RegExp Constructor  security/detect-non-literal-regexp

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/iphone/cli/commands/_build.js
  18:2   warning  'bufferEqual' is assigned a value but never used  no-unused-vars
  27:2   warning  'humanize' is assigned a value but never used     no-unused-vars
2875:1   error    Trailing spaces not allowed                       no-trailing-spaces
2877:1   error    Trailing spaces not allowed                       no-trailing-spaces
3265:46  warning  A space is required after '['                     array-bracket-spacing
3265:63  warning  A space is required before ']'                    array-bracket-spacing
4343:8   warning  'nameChanged' is assigned a value but never used  no-unused-vars
4345:3   warning  'extRegExp' is assigned a value but never used    no-unused-vars

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/iphone/cli/commands/_buildModule.js
186:60   error    Trailing spaces not allowed          no-trailing-spaces
187:116  error    Trailing spaces not allowed          no-trailing-spaces
477:1    error    Trailing spaces not allowed          no-trailing-spaces
478:99   error    Missing semicolon                    semi
514:76   error    Missing semicolon                    semi
526:71   error    Trailing spaces not allowed          no-trailing-spaces
720:50   warning  'name' is defined but never used     no-unused-vars
723:6    error    The function binding is unnecessary  no-extra-bind

/Users/build/jenkins/workspace/dom_titanium_mobile_PR-9983-BKK3FPHV3KN5SLHV6DQKSYFLD2O5UPXXLH7YQVXVMUXFREV4MXEQ/tests/Resources/ti.ui.tableview.addontest.js
14:3  warning  'didFocus' is assigned a value but never used  no-unused-vars

✖ 29 problems (12 errors, 17 warnings)
12 errors, 4 warnings potentially fixable with the `--fix` option.

Warning: Task "eslint:src" failed.� Use --force to continue.

Aborted due to warnings.
npm ERR! Test failed.  See above for more details.

Generated by 🚫 dangerJS

@build build added the ios label Apr 16, 2018
@sgtcoolguy
Copy link
Contributor

Is this a continuation of #9798? Does it include the same changes (plus a whole lot more)?

@hansemannn
Copy link
Collaborator Author

Yep, it does! Although I begin to feel that we may need to fix some memory issues around JSCore that haven't even been reported so far.

@sgtcoolguy
Copy link
Contributor

@hansemannn I wasn't suggesting we should have closed #9798

In fact, I think it would be more useful/easier to review and track if we had major parts of this PR separated:

  • Removal of TiCore/move to JSCore exclusively
  • Removal of KrollThread (did we deprecate that?)
  • Move to framework
  • Miscellaneous stuff like the Cocopods spec

@hansemannn
Copy link
Collaborator Author

Its super hard to remove ticore/krollthread once we moved the core to a framework. I am still unsure we will even be able to do the framework change, since it will be a semi-hard task to migrate old modules properly.

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy I thought about this quite long so far: How do we minimize the impact of removing core-classes from modules in favor of a framework? Right now, developers import core-classes via

#import "TiApp.h"
import "TiUtils.h"

etc. This is possible, because we have "ghost" headers inside ~/Library/Application Support/Titanium/mobilesdk/osx/7.3.0/iphone/include that are generated and copied during the scons build and have been removed as part of this PR already. So what if we would leave them there, but import the framework using the module-syntax (e.g. @import Titanium;) instead? It could possible work, since the module-syntax only imports the required classes if not already imported to avoid duplicate imports.

We could still write a migration script that goes through the module's classes and scans for the "blacklisted" headers that are now part of the core-framework. Would this be a practical way?

Sorry, I just need someone to talk to about this 😄.

@sgtcoolguy
Copy link
Contributor

@hansemannn
I'm not sure I follow why it'd be hard to break apart the major parts of this PR.

Its super hard to remove ticore/krollthread once we moved the core to a framework. I am still unsure we will even be able to do the framework change, since it will be a semi-hard task to migrate old modules properly.

I get that the framework change may cause native module breakage, which we should work on in a PR that handles the move to a framework. But we should be able to remove TiCore via the old PR you closed. And I assume removing KrollThread would be similar.

As far as the framework changes around imports, that's outside my knowledge right now.

@build build added the ios label Jun 6, 2018
@hansemannn hansemannn changed the base branch from master to next July 29, 2018 07:33
@hansemannn
Copy link
Collaborator Author

Rebased and reworked from next in #10218

@hansemannn hansemannn closed this Jul 29, 2018
@hansemannn hansemannn deleted the TIMOB-25927 branch July 29, 2018 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants