Initial Draft of Dynamic Framework Target #2

sgoldberg-sfdc · 2017-01-26T20:20:21Z

Platforms affected

iOS / iOS Simulator

What does this PR do?

Add's a Dynamic Framework Target for building Cordova

What testing has been done on this change?

Tested dynamic framework in application

sgoldberg-sfdc · 2017-01-26T20:21:13Z

@khawkins Just getting the ball rolling here. I probably need some sort of template for the header comments in the framework header.

khawkins · 2017-01-26T20:24:52Z

CordovaLib/CordovaLib.xcodeproj/project.xcworkspace/contents.xcworkspacedata should not be checked in—those are local development files. CordovaLib/CordovaLib.xcodeproj/project.xcworkspace/ should probably be added to .gitignore.

sgoldberg-sfdc · 2017-01-26T20:25:41Z

Oops, I got used to having a .gitignore setup.

khawkins · 2017-01-26T20:38:57Z

CordovaLib/Cordova/Cordova.h

+//! Project version string for Cordova.
+FOUNDATION_EXPORT const unsigned char CordovaVersionString[];
+
+// In this header, you should import all the public headers of your framework using statements like #import <Cordova/PublicHeader.h>


All of the public headers should be represented in the umbrella header, a la:

#import <Cordova/CDVViewController.h

And so on.

Hmm, I swore that was automated by adding them to public. I guess not. Will do.

khawkins · 2017-01-26T20:46:08Z

.gitignore

@@ -224,3 +224,4 @@ node_modules/color-support/
 node_modules/fs.realpath/
 node_modules/jasmine-core/
 node_modules/jasmine/
+CordovaLib/CordovaLib.xcodeproj/project.xcworkspace


Given that CordovaLib has its own .gitignore, let's just add this in there—there aren't any other CordovaLib entries in this file.

khawkins · 2017-01-26T22:03:19Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				OTHER_LDFLAGS = "-all_load";
+				PRODUCT_BUNDLE_IDENTIFIER = com.salesforce.Cordova;
+				PRODUCT_NAME = "$(TARGET_NAME)";
+				PUBLIC_HEADERS_FOLDER_PATH = Cordova.framework/PublicHeaders;


Can we go with the standard value for framework targets here? I.e. $(CONTENTS_FOLDER_PATH)/Headers.

khawkins · 2017-01-26T22:03:38Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				OTHER_LDFLAGS = "-all_load";
+				PRODUCT_BUNDLE_IDENTIFIER = com.salesforce.Cordova;
+				PRODUCT_NAME = "$(TARGET_NAME)";
+				PUBLIC_HEADERS_FOLDER_PATH = Cordova.framework/PublicHeaders;


$(CONTENTS_FOLDER_PATH)/Headers here too.

khawkins · 2017-01-26T22:17:18Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
+				INFOPLIST_FILE = Cordova/Info.plist;
+				INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks";
+				IPHONEOS_DEPLOYMENT_TARGET = 10.2;


I'd roll this back to 8.0, as the current min versions of iOS support in CordovaLib are already less than that.

khawkins · 2017-01-26T22:17:35Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
+				INFOPLIST_FILE = Cordova/Info.plist;
+				INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks";
+				IPHONEOS_DEPLOYMENT_TARGET = 10.2;


Roll back to 8.0.

+1. Roll back to 8.0.

khawkins · 2017-01-26T22:19:28Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				MTL_ENABLE_DEBUG_INFO = NO;
+				ONLY_ACTIVE_ARCH = NO;
+				OTHER_LDFLAGS = "-all_load";
+				PRODUCT_BUNDLE_IDENTIFIER = com.salesforce.Cordova;


I'd make this com.apache.cordova.Cordova.

khawkins · 2017-01-26T22:19:42Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				MTL_ENABLE_DEBUG_INFO = YES;
+				ONLY_ACTIVE_ARCH = NO;
+				OTHER_LDFLAGS = "-all_load";
+				PRODUCT_BUNDLE_IDENTIFIER = com.salesforce.Cordova;


I'd make this com.apache.cordova.Cordova.

+1 here as well.

khawkins · 2017-01-26T23:25:22Z

So the other body of work that we're going to need to do to make this reasonably production ready as a first class sibling to the CordovaLib library, is to integrate it into the testing environment. The tests run through the npm local package (npm install, npm test), and ultimately end up executing (among other things) tests from the tests/cordova-ios.xcworkspace workspace, which currently links a Cordova testing app and test suite to the CordovaLib static library.

My thought is to add a new app and test target that links the framework instead of the static lib, but shares the same test files as the test target for the static lib. Thoughts?

sgoldberg-sfdc · 2017-01-26T23:27:23Z

Lets discuss in the work item. My thoughts were we would probably want to add a sample app and integrate into tests. I was going to pick your brain on their test architecture. From what I can tell best practices for sample apps to demonstrate dynamic frameworks is to implement them in Swift.

…

On Thu, Jan 26, 2017 at 5:25 PM, Kevin Hawkins ***@***.***> wrote: So the other body of work that we're going to need to do to make this reasonably production ready as a first class sibling to the CordovaLib library, is to integrate it into the testing environment. The tests run through the npm local package (npm install, npm test), and ultimately end up executing (among other things) tests from the tests/cordova-ios.xcworkspace workspace, which currently links a Cordova testing app and test suite to the CordovaLib static library. My thought is to add a new app and test target that links the framework instead of the static lib, but shares the same test files as the test target for the static lib. Thoughts? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APNSeEQUmKqsqkH3KyBX90WMR5rNXWQ8ks5rWStigaJpZM4LvF9k> .

bhariharan · 2017-01-26T23:58:58Z

CordovaLib/Cordova/Cordova.h

@@ -0,0 +1,19 @@
+//


Maybe add the standard Apache license/copyright to every new file you're creating. That seems to be the Cordova way.

Here you go:

/* Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */

bhariharan · 2017-01-27T00:01:02Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
+				INFOPLIST_FILE = Cordova/Info.plist;
+				INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks";
+				IPHONEOS_DEPLOYMENT_TARGET = 10.2;


+1. Roll back to 8.0.

bhariharan · 2017-01-27T00:01:17Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				MTL_ENABLE_DEBUG_INFO = YES;
+				ONLY_ACTIVE_ARCH = NO;
+				OTHER_LDFLAGS = "-all_load";
+				PRODUCT_BUNDLE_IDENTIFIER = com.salesforce.Cordova;


+1 here as well.

bhariharan · 2017-01-27T00:01:44Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
+				INFOPLIST_FILE = Cordova/Info.plist;
+				INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks";
+				IPHONEOS_DEPLOYMENT_TARGET = 10.2;


bhariharan · 2017-01-27T00:02:44Z

CordovaLib/CordovaLib.xcodeproj/project.pbxproj

+				OTHER_LDFLAGS = "-all_load";
+				PRODUCT_BUNDLE_IDENTIFIER = com.salesforce.Cordova;
+				PRODUCT_NAME = "$(TARGET_NAME)";
+				PUBLIC_HEADERS_FOLDER_PATH = Cordova.framework/PublicHeaders;


Like @khawkins said, can we go with the standard value for framework targets here? I.e. $(CONTENTS_FOLDER_PATH)/Headers

- Removed .gitignore changes - Added standard header comment - Rolled iOS target back to 8.0 - Used standard header path - Added all public headers to Cordova.h

sgoldberg-sfdc · 2017-02-01T20:24:06Z

@khawkins @bhariharan - Pushed requested changes

khawkins · 2017-02-01T21:43:58Z

CordovaLib/Cordova/Cordova.h

+
+// In this header, you should import all the public headers of your framework using statements like #import <Cordova/PublicHeader.h>
+
+#import "CDVAvailability.h"


These should be in the same format as expected direct access from a framework consumer. E.g. #import <Cordova/CDVAvailability.h>.

bhariharan

+1 to Kevin's comment. Looks good otherwise.

sgoldberg-sfdc · 2017-02-03T18:27:59Z

Headers updated. @khawkins @bhariharan I think a sample app should be part of a different PR.

khawkins

Looks good over all. We can look at the test infrastructure in the next PR.

Initial Draft of Dynamic Framework Target

4d76510

sgoldberg-sfdc added 2 commits January 26, 2017 14:32

Remove workspace.data

3fbe176

fixed untracked files

7265e10

khawkins reviewed Jan 26, 2017

View reviewed changes

sgoldberg-sfdc added 3 commits January 26, 2017 14:41

Remove workspacedata redux

6a20874

Removed again.

bc105fb

Updated .gitignore

523b4d5

khawkins reviewed Jan 26, 2017

View reviewed changes

bhariharan suggested changes Jan 27, 2017

View reviewed changes

sgoldberg-sfdc added 2 commits February 1, 2017 14:20

Updated with PR feedback

f4c5160

- Removed .gitignore changes - Added standard header comment - Rolled iOS target back to 8.0 - Used standard header path - Added all public headers to Cordova.h

Removed workspace data

53ae856

khawkins reviewed Feb 1, 2017

View reviewed changes

bhariharan suggested changes Feb 2, 2017

View reviewed changes

Update Cordova Headers

9c8f63f

khawkins approved these changes Feb 3, 2017

View reviewed changes

khawkins merged commit 2e9a697 into khawkins:master-dynamic-framework Feb 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Draft of Dynamic Framework Target #2

Initial Draft of Dynamic Framework Target #2

sgoldberg-sfdc commented Jan 26, 2017

sgoldberg-sfdc commented Jan 26, 2017

khawkins commented Jan 26, 2017

sgoldberg-sfdc commented Jan 26, 2017

khawkins Jan 26, 2017

sgoldberg-sfdc Jan 26, 2017

khawkins Jan 26, 2017

khawkins Jan 26, 2017

khawkins Jan 26, 2017

khawkins Jan 26, 2017

khawkins Jan 26, 2017

bhariharan Jan 27, 2017

khawkins Jan 26, 2017

khawkins Jan 26, 2017

bhariharan Jan 27, 2017

khawkins commented Jan 26, 2017

sgoldberg-sfdc commented Jan 26, 2017 via email

bhariharan Jan 26, 2017

bhariharan Jan 27, 2017

bhariharan Jan 27, 2017

bhariharan Jan 27, 2017

bhariharan Jan 27, 2017

sgoldberg-sfdc commented Feb 1, 2017

khawkins Feb 1, 2017

bhariharan left a comment

sgoldberg-sfdc commented Feb 3, 2017

khawkins left a comment


		// In this header, you should import all the public headers of your framework using statements like #import <Cordova/PublicHeader.h>

		#import "CDVAvailability.h"

Initial Draft of Dynamic Framework Target #2

Initial Draft of Dynamic Framework Target #2

Conversation

sgoldberg-sfdc commented Jan 26, 2017

Platforms affected

What does this PR do?

What testing has been done on this change?

sgoldberg-sfdc commented Jan 26, 2017

khawkins commented Jan 26, 2017

sgoldberg-sfdc commented Jan 26, 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

khawkins commented Jan 26, 2017

sgoldberg-sfdc commented Jan 26, 2017 via email

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

sgoldberg-sfdc commented Feb 1, 2017

Choose a reason for hiding this comment

bhariharan left a comment

Choose a reason for hiding this comment

sgoldberg-sfdc commented Feb 3, 2017

khawkins left a comment

Choose a reason for hiding this comment