[TIMOB-25353] Android: Allow Titanium modules to process module dependencies #9481

garymathews · 2017-09-26T21:41:49Z

appc-aar-tools#16 NEEDS TO BE MERGED FIRST!

In an attempt to minimise conflicting Google Play Services library versions I have implemented the ability for Titanium Android native modules to have dependencies on other Titanium Android native modules.

This is to allow dependency on the ti.playservices module for modules such as ti.map and ti.admob for Google Play Services.

This will prevent the need to update each modules Play Services library and negate the need for module developers to include their own Play Services library which could conflict with existing modules.

TEST CASE

Acquire ti.map-3.3.2 pre-release
Add ti.playservices and ti.map into your tiapp.xml

<android xmlns:android="http://schemas.android.com/apk/res/android">
  <manifest>  
    <application>
      <meta-data android:name="com.google.android.maps.v2.API_KEY" android:value="<YOUR API KEY>"/>
    </application>
  </manifest>
</android>
<modules>
    <module platform="android" version="3.3.2">ti.map</module>
    <module platform="android" version="11.0.40">ti.playservices</module>
</modules>

var win = Ti.UI.createWindow(),
    Map = require('ti.map'),
    a = Map.createAnnotation({
        title: 'Axway Appcelerator',
        latitude: 37.38935, longitude: -121.980,
        pincolor: Map.ANNOTATION_GREEN
    }),
    map = Map.createView({
        userLocation: true,
        mapType: Map.NORMAL_TYPE,
        animate: true,
        annotations: [a],
        region: {
            latitude: 37.3680, longitude: -121.9145,
            latitudeDelta: 0.1, longitudeDelta: 0.1
        }
    });

map.addAnnotation(a);

win.add(map);
win.open();

Map should display with an annotation

JIRA Ticket

build · 2017-09-26T22:01:29Z

	Warnings
⚠️	🔒 Changes were made to package.json, but not to package-lock.json - Perhaps you need to run `npm install`?

Generated by 🚫 dangerJS

garymathews · 2017-09-26T23:03:49Z

After discussion with @jquick-axway it's probably best this exists as an external module.

benbahrenburg · 2017-09-26T23:36:15Z

@garymathews if implemented as an external module will this still remove the conflicts that module developers have? Is there anyway we can have a standard which we (module developers) build against?

jquick-axway · 2017-09-27T16:56:16Z

@benbahrenburg, the idea is to wrap the Google Play Services main JAR in its own module and to have other modules which depend on it (ex: ti.map) remove the Google Play Services JAR file from their "lib" directory. It'll be our responsibility to keep the Google Play Services module up-to-date. So, this will only resolve dependency conflicts if other modules "play ball", remove their Google Play Services JAR, and instead document that they require this new module as a dependency.

For example, if an app developer wanted to use the "ti.map" module, the developer would have to add the "ti.map" module and the Google Play Services module in order for it to work. However, doing so with an existing module would be a breaking change, so, we'll look into providing a sensible warning message when attempting to a build while a module dependency is missing. May involve having adding a new feature to "timodule.xml" to reference another module as a dependency.

As an added benefit to making Google Play Services its own module, we'll likely expose the APIs needed to ensure that it's installed and enabled on the device as documented by Google below. My impression is that most modules are not doing this right now, which is why we're holding on to an old version of this library. So, exposing these APIs is needed since the Google Play Services library doesn't contain implementation for most of its functionality and instead does inter-process communications with the Google Play app to request it to perform the needed operations.

What do you think?

benbahrenburg · 2017-09-28T01:16:49Z

@jquick-axway sounds like a good plan. I had tried to keep the different versions of Play straight using this repo https://github.com/benbahrenburg/Ti-Google-Play-Services but it never really worked out.

Having one place that everyone could get the latest version to compile against I think would be a welcome addition.

garymathews · 2017-09-28T21:43:19Z

@janvennemann Updated PR

jquick-axway

Looks good to me.

@janvennemann, do you have any feedback you want to give?

janvennemann

Just a few minor notes regarding code style. Apart from that it looks good to me. Nice feature!

janvennemann · 2017-10-04T12:20:45Z

android/cli/commands/_buildModule.js

+
+	// process module dependencies
+	this.modules = this.timodule && !Array.isArray(this.timodule.modules) ? [] : this.timodule.modules.filter(function(m) {
+		if (!m.platform || /^android$/.test(m.platform)) {


Consider using a negative check and return early here so you won't have to wrap everything in the if block.

janvennemann · 2017-10-04T12:22:02Z

android/cli/commands/_buildModule.js

+		if (!m.platform || /^android$/.test(m.platform)) {
+			var localPath = path.join(this.modulesDir, m.id),
+				globalPath = path.join(this.globalModulesDir, m.id),
+				getModulePath = function(modulePath) {


If possible do not define functions inside loop-like function like forEach, map or filter and don't use bind as this is expensive. Move it out and pass the module object into it so you can still edit it. Not really an issue here considering the number of modules a project probably has but this will also make the code more readable.

janvennemann · 2017-10-04T13:23:39Z

android/cli/commands/_buildModule.js

+				}.bind(this);
+
+			if ((fs.existsSync(localPath) && getModulePath(localPath)) ||
+				(fs.existsSync(globalPath) && getModulePath(globalPath))) {


I like to move longer if checks like this into a simple function with a fitting name so it's easier to understand what it does.

janvennemann · 2017-10-04T14:00:02Z

@garymathews, what about when a modules defines dependencies but they are not present? As i understand it, currently this will just be silently ignored. Maybe we should print a nice error message instead of just continuing the build, knowing that it will probably fail later on anyway.

jquick-axway · 2017-10-04T16:31:51Z

When the build can't find a module in "tiapp.xml", we see the following error messages logged:

[WARN] :   Could not find a valid Titanium module id=ti.whatever version=latest platform=android deploy-type=test
[ERROR] :  Could not find all required Titanium Modules:
[ERROR] :     id: ti.whatever	 version: latest	 platform: android	 deploy-type: test

If we can't replicate the above for dependency modules as well, then that would be great. (Unless it's doing this already. I haven't tested this code change yet.)

Also, does the build simply log all of the <module> element's attributes if the module hasn't been found? If so, then perhaps as a best-practice we should add a url attribute to it so that developers will know where to download it from.

garymathews · 2017-10-04T19:04:18Z

@janvennemann Updated PR

hansemannn

@garymathews Can you fix the typo in LIBRARY_ORIGIN_PORJECT while you're at it?

janvennemann

Approved! appc-aar-tools is also updated to 1.1.4

We should probably also make changes to the app build so that it is aware of module dependencies and can print an appropriate error message if a dependency is missing.

garymathews · 2017-10-07T03:29:37Z

@janvennemann Like this?

janvennemann · 2017-10-17T18:19:49Z

@garymathews Yes, but i mean in the app build. Let's say you use a module that has another dependency then you would also require both modules during the app build, right?

janvennemann

@garymathews, can you please check the linting errors reported by Jenkins? Thanks!

sgtcoolguy · 2017-11-07T15:58:03Z

android/cli/commands/_build.js

+								this.cli.tiapp.modules.push({
+									id: dependency.id,
+									version: dependency.version,
+									platform: ['android'],


Do dependencies always have to be explicitly the same platform and native, or could it be commonjs too?
Do we need to specify the apiversion here to be the same as the requirer/SDK, or is that handled by validateTiModules?

Also, If they specify a version for the dependency, I assume it has to be an exact match, we have no concept of semver here?

commonjs modules aren't supported as module dependencies

I tried my best to re-use our current module handling code, so all of the existing module checks will take place. apiversion needs to match the SDK

It will be the same as our current module version handling code (I believe it's exact match only?)

sgtcoolguy · 2017-11-07T15:59:09Z

android/cli/commands/_build.js

@@ -1657,6 +1667,10 @@ process.exit(1);
 		 		}
 		 		logger.error(msg);
 		 		process.exit(1);
+		 		*/
+
+		 		// re-validate modules


Is there a possible infinite loop here? I see before you'd just give up and report missing dependencies, but now we basically implicitly add them for you and re-try.

It looks like that, but our existing checks will prevent that (if the module is missing/invalid) from happening and return before reaching this code again

lokeshchdhry · 2017-11-09T00:10:35Z

FR Passed.

We can now use modules using google playservices & module ti.playservices together in an app.

Studio Ver: 4.10.0.201709271713
SDK Ver: 7.0.0.v20171108114616
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.10
Appc CLI: 6.3.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.7
Node Ver: 7.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 5 --- Android 6.0.1
⇨ google Nexus 6P --- Android 8.0.0

benbahrenburg · 2017-11-09T11:27:40Z

@garymathews @jquick-axway is there anything that module developers should be doing to embrace this? For example, how would I add this as a dependency to my module projects. Or is this simply asking the module developers to take the same version of playservices and use it in this modules?

sgtcoolguy · 2017-11-09T19:50:14Z

@benbahrenburg Here's the PR that makes use of ti.playservices as a dependency of ti.map: tidev/ti.map#216

I think the goal here is to have ti.playservices be a dependency of any native modules requiring it, and then they'll grab the latest ti.playservices module by default (or if you specify an exact version in your tiapp.xml it'll use that). That way we can avoid having all these modules embed differing versions.

jquick-axway · 2017-11-09T19:56:33Z

@benbahrenburg, we plan on writing up a blog post about this sometime this month to inform 3rd party module developers.

For a more specific example, have a look at our "ti.map" module's "timodule.xml" file (link below) and note the new <modules> section. This is where you define your dependencies.
https://github.com/garymathews/ti.map/blob/9b3c4120cfd907468ca3a598a93d3c48ff49e9d2/android/timodule.xml#L11

So, when a developer includes your module in their project, our build system will double check that their "tiapp.xml" file also references your module's module dependency too. If it doesn't, then the developer will get a build error stating that they need to also include said module dependency into their project. You can see an example of what a "tiapp.xml" file will look like with the "ti.map" module and the new Google Play Service module "ti.playservices" at the top of this pull request.
#9481 (comment)

The plan is to always bundle the new "ti.playservices" dependency module with Titanium so that developer's won't have to download/install it themselves.

sgtcoolguy · 2017-11-09T20:08:38Z

@jquick-axway Just to note: I asked Gary to modify the behavior of handling module dependencies so that we will attempt to automatically/implicitly add the dependency as if the user had done so in their tiapp.xml (but if they specify it in the tiapp.xml that works too). This was to avoid causing a breaking change to ti.map usage for existing apps (suddenly they'd stop working and the user would have to explicitly add ti.playservices to the app to get it working again).

garymathews added android improvement work in progress 🚧 labels Sep 26, 2017

garymathews added this to the 7.0.0 milestone Sep 26, 2017

garymathews requested review from janvennemann, hansemannn, ypbnv and jquick-axway September 26, 2017 21:41

garymathews changed the title ~~[TIMOB-24722] Android: Add Play Services 11.0.4 module~~ [TIMOB-25353] Android: Add Play Services 11.0.4 module Sep 26, 2017

garymathews force-pushed the TIMOB-24722 branch 3 times, most recently from 532a4f2 to 0a46faa Compare September 27, 2017 23:14

This was referenced Sep 28, 2017

[6_3_X][TIMOB-25353] Android: Allow Titanium modules to process module dependencies #9488

Closed

[TIMOB-25353] Validate classes.jar path appcelerator-archive/appc-aar-tools#16

Merged

garymathews changed the title ~~[TIMOB-25353] Android: Add Play Services 11.0.4 module~~ [TIMOB-25353] Android: Allow Titanium modules to process module dependencies Sep 28, 2017

garymathews removed the work in progress 🚧 label Oct 2, 2017

jquick-axway approved these changes Oct 3, 2017

View reviewed changes

janvennemann requested changes Oct 4, 2017

View reviewed changes

garymathews force-pushed the TIMOB-24722 branch from f01d899 to 8fb0db6 Compare October 4, 2017 19:02

garymathews force-pushed the TIMOB-24722 branch 2 times, most recently from 5c29168 to cc7ff30 Compare October 4, 2017 19:16

hansemannn reviewed Oct 4, 2017

View reviewed changes

janvennemann approved these changes Oct 5, 2017

View reviewed changes

garymathews force-pushed the TIMOB-24722 branch from 3f90de5 to 2a66415 Compare October 25, 2017 17:21

janvennemann requested changes Nov 3, 2017

View reviewed changes

garymathews force-pushed the TIMOB-24722 branch from 113fec4 to e007bf6 Compare November 7, 2017 00:51

Gary Mathews added 3 commits November 6, 2017 16:52

[TIMOB-25353] Allow Titanium modules to process module dependencies

f521363

[TIMOB-25353] Use appc-aar-tools 1.1.4

e575aae

[TIMOB-25353] Handle unresolved module dependencies

e5f385f

garymathews force-pushed the TIMOB-24722 branch from e007bf6 to bebc657 Compare November 7, 2017 00:52

[TIMOB-25353] Attempt to include missing module dependencies

d876555

garymathews force-pushed the TIMOB-24722 branch from bebc657 to d876555 Compare November 7, 2017 00:53

[TIMOB-25353] Fix linting issues

a8a17b7

sgtcoolguy reviewed Nov 7, 2017

View reviewed changes

Merge branch 'master' into TIMOB-24722

96839a6

lokeshchdhry merged commit cff6674 into tidev:master Nov 9, 2017

hansemannn mentioned this pull request Nov 15, 2017

Module Not Found Error. tidev/ti.playservices#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-25353] Android: Allow Titanium modules to process module dependencies #9481

[TIMOB-25353] Android: Allow Titanium modules to process module dependencies #9481

garymathews commented Sep 26, 2017 •

edited

build commented Sep 26, 2017 •

edited

garymathews commented Sep 26, 2017

benbahrenburg commented Sep 26, 2017

jquick-axway commented Sep 27, 2017

benbahrenburg commented Sep 28, 2017

garymathews commented Sep 28, 2017

jquick-axway left a comment

janvennemann left a comment

janvennemann Oct 4, 2017

janvennemann Oct 4, 2017

janvennemann Oct 4, 2017

janvennemann commented Oct 4, 2017

jquick-axway commented Oct 4, 2017

garymathews commented Oct 4, 2017

hansemannn left a comment

janvennemann left a comment

garymathews commented Oct 7, 2017

janvennemann commented Oct 17, 2017

janvennemann left a comment

sgtcoolguy Nov 7, 2017 •

edited

garymathews Nov 7, 2017

sgtcoolguy Nov 7, 2017

garymathews Nov 7, 2017 •

edited

lokeshchdhry commented Nov 9, 2017

benbahrenburg commented Nov 9, 2017

sgtcoolguy commented Nov 9, 2017

jquick-axway commented Nov 9, 2017

sgtcoolguy commented Nov 9, 2017

[TIMOB-25353] Android: Allow Titanium modules to process module dependencies #9481

[TIMOB-25353] Android: Allow Titanium modules to process module dependencies #9481

Conversation

garymathews commented Sep 26, 2017 • edited

appc-aar-tools#16 NEEDS TO BE MERGED FIRST!

TEST CASE

build commented Sep 26, 2017 • edited

garymathews commented Sep 26, 2017

benbahrenburg commented Sep 26, 2017

jquick-axway commented Sep 27, 2017

benbahrenburg commented Sep 28, 2017

garymathews commented Sep 28, 2017

jquick-axway left a comment

Choose a reason for hiding this comment

janvennemann left a comment

Choose a reason for hiding this comment

janvennemann Oct 4, 2017

Choose a reason for hiding this comment

janvennemann Oct 4, 2017

Choose a reason for hiding this comment

janvennemann Oct 4, 2017

Choose a reason for hiding this comment

janvennemann commented Oct 4, 2017

jquick-axway commented Oct 4, 2017

garymathews commented Oct 4, 2017

hansemannn left a comment

Choose a reason for hiding this comment

janvennemann left a comment

Choose a reason for hiding this comment

garymathews commented Oct 7, 2017

janvennemann commented Oct 17, 2017

janvennemann left a comment

Choose a reason for hiding this comment

sgtcoolguy Nov 7, 2017 • edited

Choose a reason for hiding this comment

garymathews Nov 7, 2017

Choose a reason for hiding this comment

sgtcoolguy Nov 7, 2017

Choose a reason for hiding this comment

garymathews Nov 7, 2017 • edited

Choose a reason for hiding this comment

lokeshchdhry commented Nov 9, 2017

benbahrenburg commented Nov 9, 2017

sgtcoolguy commented Nov 9, 2017

jquick-axway commented Nov 9, 2017

sgtcoolguy commented Nov 9, 2017

garymathews commented Sep 26, 2017 •

edited

build commented Sep 26, 2017 •

edited

sgtcoolguy Nov 7, 2017 •

edited

garymathews Nov 7, 2017 •

edited