[TIMOB-23570] Add support for dynamic Frameworks #159

janvennemann · 2017-04-21T14:31:32Z

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

hansemannn · 2017-04-22T11:30:47Z

metabase/ios/src/util.cpp

+      filePathAndName = filePathAndName.substr(lastSlashBeforeFramework + 1);
+    } else {
+      filePathAndName = replace(CXStringToString(clang_getFileName(file)), ctx->getSDKPath(), "");
+    }


Minor formatting issue

hansemannn · 2017-04-22T11:31:24Z

metabase/ios/lib/metabase.js

+		}
+		var frameworkName = frameworkNameMatches[1];
+		var frameworkHeadersPath = path.join(dynamicFrameworkPath, 'Headers');
+		var moduleMapPathAndFilename = path.join(dynamicFrameworkPath, 'Modules', 'module.modulemap');


Is the modulemap always named module ?

Yes, see https://clang.llvm.org/docs/Modules.html#module-maps

I have to disagree. There are module maps like RobotKit.modulemap, AWSCore.modulemap and AWSMobileAnalytics.modulemap available out there which would not be handled correctly.

Interesting, can you give a more specific example? I just tried AWSCore from CocoaPods and the built AWSCore.Framework only includes a module.modulemap.

hansemannn · 2017-04-22T11:33:07Z

metabase/ios/lib/metabase.js

+		cacheTokenData.specs.push(checksumMatches[1]);
+		checksumMatches = specChecksumRegex.exec(podLockfileContent);
+	}
+	var podfileChecksumMatch = podLockfileContent.match(/PODFILE CHECKSUM: (.*)/);


Is this working with CocoaPods 0.39 as well? If not we should officially remove it's support as we work more intensively with it.

Oh yeah, you are right, a Podfile for <=0.39 will not contain the Podfile checksum. While just testing this i was also running into http://blog.cocoapods.org/Sharding/. Considering this we should definitely drop the support for CocoaPods 0.39. Hyperloop 2.2.0 would be the perfect release for that, hence i created TIMOB-24623 and i'd say we leave this as is.

Feeling bad to do it without a major release-bump, but it would irritate people. So I guess we can make an excuse and remove it in 2.2.0. We deprecated it already right? I know we left a note, but it should definitely show a clear deprecating note by now.

Yes, we print a warning if a version below 1.0 is used here.

hansemannn · 2017-04-22T11:36:26Z

metabase/ios/lib/metabase.js

+		var f = includes[fw] || {};
+		extractImplementations(fn, f);
+		includes[fw] = f;
+	});


Obviously no fan of string comparisons, but I'm wondering if there might be a way without it here.

I wouldn't know how else to properly determine the framework/library name. The path is always structured something like Headers/<LibraryName>/HeaderName.h. There can be more subfolders under Headers so i guess this is the easiest way.

hansemannn · 2017-04-22T11:37:32Z

metabase/ios/lib/metabase.js

+ * @param {Object} data The CocoaPods metabase mappings to store
+ */
+function writeCocoaPodsMetabaseMappingsToCache(cacheDir, cacheToken, data) {
+	var cacheFilename = 'metabase-mappings-cocoapods-' + cacheToken + '.json';


We may want to leave a note somewhere that this name is used across the project or reference it from a global config file. Not sure what's the best way to deal with hardcoded values here.

That cache file is only accessed inside the metabase.js file so there is no need to make it available globally. Although, for better maintainability i moved this line to a separate function, so the naming can be controlled from a single place.

perfect, thanks.

hansemannn · 2017-04-25T13:23:59Z

Different question: We enable dynamic frameworks support for Cocoapods. How about non-cocoapods libraries (e.g. local embedded frameworks)?

janvennemann · 2017-04-25T14:19:10Z

The current implementation is not directly tied to CocoaPods so we can reuse that to support local embedded frameworks in TIMOB-23853 as well.

hansemannn · 2017-04-25T14:34:03Z

Could we do both together then? Would just love to get the other ticket "In Progress", so the (pretty high number of) watchers are happy that's something is going on.

janvennemann · 2017-04-25T17:43:25Z

Sure we can do that, the Ticket is marked for 2.2.0 anyway. I just already had the CocoaPods stuff laying around from a previous PoC.

hansemannn · 2017-04-26T07:49:58Z

Yeah sure, 2.1.0 is bundled and out of my head, haha. Great!

hansemannn · 2017-06-30T12:03:27Z

@janvennemann Can you please resolve the merge conflicts and validate the ticket with the latest changes you did? I would like to schedule a 2.2.0 with this one and #189 asap.

Support Hyperloop usage of frameworks that are recognized by the new framework hook in the SDK. Also refactors a major party of the CocoaPods dyanamic framework support to support additional third-party framework configurations.

hansemannn · 2017-09-16T13:46:18Z

metabase/ios/lib/generate/index.js

@@ -308,6 +314,33 @@ function generateFromJSON (name, json, state, callback, includes) {
 }

 /**
+ * Takes a single metadata object and normalized the framework property on that object.


normalizes

hansemannn · 2017-09-16T13:46:59Z

metabase/ios/lib/generate/index.js

+ * This normalization will try to associate the path with a virtual third-party
+ * framework that is configured in the appc.js file, and replace it with the virtual
+ * framework name. Should the path be unknown we remove the framework property as
+ * it is a symbol which can not be associated to a specific framework and we can't


cannot or can't

hansemannn · 2017-09-16T13:47:46Z

metabase/ios/lib/generate/module.js

@@ -77,8 +77,11 @@ function generate (json, mod, state) {
 		m.class.obj_class_method.length ||
 		Object.keys(m.class.static_variables).length ||
 		m.class.blocks.length) {
-		if (mod.filename && mod.filename.indexOf('/') > 0) {
-			m.import = mod.filename;
+		if (mod.filename.match(/-Swift\.h$/)) {


Do all Swift frameworks expose their Obj-c header with the "-Swift.h" suffix?

Yes, Xcode will automatically generate that header filename using the PRODUCT_MODULE_NAME build setting and adds -Swift.

hansemannn · 2017-09-16T13:48:10Z

metabase/ios/lib/metabase.js

+/**
+ * Creates a MD5 hash from the given string data.
+ *
+ * @param {String} data Data the hash will be genreated for


generated

hansemannn · 2017-09-16T13:49:35Z

metabase/ios/lib/metabase.js

+ * and their header file.
+ *
+ * Frameworks written in Swift are currently only supported if they provide an
+ * ObjC Interface Header.


interface header

hansemannn · 2017-09-16T13:50:14Z

metabase/ios/lib/metabase.js

+		var implementationToHeaderFileMap = includes[frameworkName] || {};
+		extractImplementations(headerPathAndFilename, implementationToHeaderFileMap);
+		includes[frameworkName] = implementationToHeaderFileMap;
+	}


Are inline functions still a valid procedure in JS? Not really a fan of it.

hansemannn · 2017-09-16T13:51:40Z

metabase/ios/lib/metabase.js

+				var objcInterfaceHeaderFilename = objcInterfaceHeaderMatch[1];
+				var headerPathAndFilename = path.join(frameworkHeadersPath, objcInterfaceHeaderFilename);
+				if (!fs.existsSync(headerPathAndFilename)) {
+					return callback(new Error('Objective-C interface header file for swift based framework ' + frameworkName.green + ' not found at expected path ' + headerPathAndFilename.cyan + '.'));


'Objective-C interface header-file for Swift-based framework ' frameworkName.green + ' not found at expected path ' + headerPathAndFilename.cyan + '.'

hansemannn · 2017-09-16T13:51:51Z

metabase/ios/lib/metabase.js

+				if (!fs.existsSync(headerPathAndFilename)) {
+					return callback(new Error('Objective-C interface header file for swift based framework ' + frameworkName.green + ' not found at expected path ' + headerPathAndFilename.cyan + '.'));
+				}
+				util.logger.trace('Swift based framework detected, parsing Objective-C interface header ' + objcInterfaceHeaderFilename.cyan);


Swift-based

Either use header or header-file every time. Above it's header-file, here it's header.

hansemannn · 2017-09-16T13:54:05Z

metabase/ios/src/def.cpp

+		if (frameworkPosition != std::string::npos) {
+      size_t slashBeforeFrameworkPosition = filename.find_last_of("/", frameworkPosition);
+      return filename.substr(slashBeforeFrameworkPosition + 1, frameworkPosition - (slashBeforeFrameworkPosition + 1));
+    }


Indentation

hansemannn · 2017-09-16T13:54:12Z

metabase/ios/src/def.cpp

-		}
+		kv["framework"] = getFramework();
+    kv["thirdparty"] = !getContext()->isSystemLocation(filename);
+    kv["filename"] = filename;


Indentation

hansemannn · 2017-09-16T13:54:33Z

plugins/hyperloop.js

@@ -15,53 +15,61 @@ exports.cliVersion = '>=3.2';
 exports.init = init;

 /**
- * Main entry-point for our plugin which looks for the platform specific
- * plugin to invoke
+ * main entry point for our plugin which looks for the platform specific


hansemannn · 2017-09-16T13:54:58Z

plugins/hyperloop.js

+ * main entry point for our plugin which looks for the platform specific
+ * plugin to invoke.
+ *
+ * A priority of 1300 to makes sure the Hyperloop plugin starts only after the


1300 makes

hansemannn

Left some review comments to be addressed.

hansemannn

CR looks fine now, approved!

janvennemann added 3 commits April 20, 2017 16:22

[TIMOB-23570] Add embed pods frameworks build phase

f9e96f3

[TIMOB-23570] Add filename detection for third-party frameworks

199499d

[TIMOB-23570] Add support for dynamic Frameworks from CocoaPods

8eb5cda

hansemannn added this to the v2.2.0 milestone Apr 22, 2017

hansemannn reviewed Apr 22, 2017

View reviewed changes

janvennemann added 4 commits April 25, 2017 15:11

[TIMOB-23570] Refactor cache token calculation into own function

e04fdad

[TIMOB-23570] Move helper functions to buttom

0e5c0ab

[TIMOB-23579] Remove unused parameter and add missing docs

b783909

[TIMOB-23570] Change indentation to tabs

46d5e6e

janvennemann added 2 commits April 25, 2017 16:44

[TIMOB-23570] Use correct function name and parameters

81e5500

[TIMOB-23570] Add fallback to parse each individual header file

9594f12

hansemannn and others added 2 commits August 8, 2017 08:25

Merge branch 'master' into TIMOB-23570

b08d894

janvennemann changed the title ~~[TIMOB-23570] Add support for dynamic Frameworks from CocoaPods~~ [TIMOB-23570] Add support for dynamic Frameworks Sep 13, 2017

janvennemann added 6 commits September 13, 2017 14:28

Merge remote-tracking branch 'origin/TIMOB-23570' into TIMOB-23570

479ade2

[TIMOB-23570] Convert spaces to tabs

77a2279

[TIMOB-23570] Lower hook priority

4bfa0fa

[TIMOB-23570] Prevent module helper generation for non-frameworks

c71ad27

[TIMOB-23570] Add search paths for appc.js third-party sources

a01a95a

Merge branch 'master' into TIMOB-23570

9f0a77b

hansemannn reviewed Sep 16, 2017

View reviewed changes

hansemannn requested changes Sep 16, 2017

View reviewed changes

janvennemann force-pushed the TIMOB-23570 branch from de79d64 to 459bec2 Compare September 18, 2017 14:56

[TIMOB-23570] Fix coding-style and typos

1add5ef

janvennemann force-pushed the TIMOB-23570 branch from 459bec2 to 1add5ef Compare September 18, 2017 14:59

hansemannn approved these changes Sep 18, 2017

View reviewed changes

hansemannn merged commit e923feb into tidev:master Sep 18, 2017

janvennemann mentioned this pull request Sep 21, 2017

Hook up Grunt to run linting and eventually run unit tests #209

Closed

[TIMOB-23570] Add support for dynamic Frameworks #159

[TIMOB-23570] Add support for dynamic Frameworks #159

Conversation

janvennemann commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansemannn commented Apr 25, 2017

janvennemann commented Apr 25, 2017

hansemannn commented Apr 25, 2017

janvennemann commented Apr 25, 2017

hansemannn commented Apr 26, 2017

hansemannn commented Jun 30, 2017

Choose a reason for hiding this comment

hansemannn Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansemannn left a comment

Choose a reason for hiding this comment

hansemannn left a comment

Choose a reason for hiding this comment

hansemannn Sep 16, 2017 •

edited