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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Nov 9, 2017
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f521363
[TIMOB-25353] Allow Titanium modules to process module dependencies
Sep 27, 2017
e575aae
[TIMOB-25353] Use appc-aar-tools 1.1.4
Sep 28, 2017
e5f385f
[TIMOB-25353] Handle unresolved module dependencies
Oct 25, 2017
d876555
[TIMOB-25353] Attempt to include missing module dependencies
Nov 7, 2017
a8a17b7
[TIMOB-25353] Fix linting issues
Nov 7, 2017
96839a6
Merge branch 'master' into TIMOB-24722
Nov 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 53 additions & 1 deletion android/cli/commands/_build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,7 +1490,7 @@ AndroidBuilder.prototype.validate = function validate(logger, config, cli) {
}

return function (callback) {
this.validateTiModules('android', this.deployType, function (err, modules) {
this.validateTiModules('android', this.deployType, function validateTiModulesCallback(err, modules) {
this.modules = modules.found;

this.commonJsModules = [];
Expand Down Expand Up @@ -1621,6 +1621,58 @@ process.exit(1);
this.modulesNativeHash = this.hash(nativeHashes.length ? nativeHashes.sort().join(',') : '');
this.modulesBindingsHash = this.hash(bindingsHashes.length ? bindingsHashes.sort().join(',') : '');

// check for any missing module dependencies
let unresolvedDependencies = [];
for (let module of this.nativeLibModules) {
const timoduleXmlFile = path.join(module.modulePath, 'timodule.xml'),
timodule = fs.existsSync(timoduleXmlFile) ? new tiappxml(timoduleXmlFile) : undefined;

if (timodule && Array.isArray(timodule.modules)) {
for (let dependency of timodule.modules) {
if (!dependency.platform || /^android$/.test(dependency.platform)) {

let missing = true;
for (let module of this.nativeLibModules) {
if (module.id === dependency.id) {
missing = false;
break;
}
}
if (missing) {
dependency.depended = module;

// attempt to include missing dependency
this.cli.tiapp.modules.push({
id: dependency.id,
version: dependency.version,
platform: ['android'],
Copy link
Contributor

@sgtcoolguy sgtcoolguy Nov 7, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?)

deployType: [this.deployType]
});

unresolvedDependencies.push(dependency);
}
}
}
}
}
if (unresolvedDependencies.length) {
/*
let msg = 'could not find required module dependencies:';
for (let dependency of unresolvedDependencies) {
msg += __('\n id: %s version: %s platform: %s required by %s',
dependency.id,
dependency.version ? dependency.version : 'latest',
dependency.platform ? dependency.platform : 'all',
dependency.depended.id);
}
logger.error(msg);
process.exit(1);
*/

// re-validate modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

return this.validateTiModules('android', this.deployType, validateTiModulesCallback.bind(this));
}

// check if we have any conflicting jars
const possibleConflicts = Object.keys(jarHashes).filter(function (jar) { return jarHashes[jar].length > 1; }); // eslint-disable-line max-statements-per-line
if (possibleConflicts.length) {
Expand Down
84 changes: 72 additions & 12 deletions android/cli/commands/_buildModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const AdmZip = require('adm-zip'),
markdown = require('markdown').markdown,
path = require('path'),
temp = require('temp'),
tiappxml = require('node-titanium-sdk/lib/tiappxml'),
util = require('util'),
wrench = require('wrench'),
spawn = require('child_process').spawn, // eslint-disable-line security/detect-child-process
Expand Down Expand Up @@ -266,17 +267,6 @@ AndroidModuleBuilder.prototype.initialize = function initialize(next) {
this.classPaths[this.androidCompileSDK.androidJar] = 1;
this.manifestFile = path.join(this.projectDir, 'manifest');

[ 'lib', 'modules', '' ].forEach(function (folder) {
var jarDir = path.join(this.platformPath, folder);

fs.existsSync(jarDir) && fs.readdirSync(jarDir).forEach(function (name) {
var file = path.join(jarDir, name);
if (/\.jar$/.test(name) && fs.existsSync(file)) {
this.classPaths[file] = 1;
}
}, this);
}, this);

this.dependencyJsonFile = path.join(this.platformPath, 'dependency.json');
this.templatesDir = path.join(this.platformPath, 'templates', 'build');
this.moduleIdSubDir = this.manifest.moduleid.split('.').join(path.sep);
Expand All @@ -293,6 +283,76 @@ AndroidModuleBuilder.prototype.initialize = function initialize(next) {
this.sharedHooksDir = path.resolve(this.projectDir, '..', 'hooks');

this.timoduleXmlFile = path.join(this.projectDir, 'timodule.xml');
this.timodule = fs.existsSync(this.timoduleXmlFile) ? new tiappxml(this.timoduleXmlFile) : undefined;
this.modulesDir = path.join(this.projectDir, 'modules', 'android');
this.globalModulesDir = path.join(this.globalModulesPath, 'android');

// 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var localPath = path.join(this.modulesDir, m.id),
globalPath = path.join(this.globalModulesDir, m.id),
getModulePath = function(modulePath) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var items = fs.readdirSync(modulePath);
if (m.version) {
for (var item of items) {
if (item === m.version) {
m.path = path.join(modulePath, m.version);
return true;
}
}
} else if (items.length) {
var latest = items[items.length - 1];
if (!latest.startsWith('.')) {
m.version = latest;
m.path = path.join(modulePath, m.version);
return true;
}
}
return false;
}.bind(this);

if ((fs.existsSync(localPath) && getModulePath(localPath)) ||
(fs.existsSync(globalPath) && getModulePath(globalPath))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}
}
return false;
}.bind(this));

// obtain module dependency android archives for aar-transform to find
this.moduleAndroidLibraries = [];
for (var module of this.modules) {
var libPath = path.join(module.path, 'lib');
fs.existsSync(libPath) && fs.readdirSync(libPath).forEach(function (name) {
var file = path.join(libPath, name);
if (/\.aar$/.test(name) && fs.existsSync(file)) {
this.moduleAndroidLibraries.push({
aarPathAndFilename: String(file),
originType: 'Module'
});
}
}, this);
}

// module java archive paths
this.jarPaths = [ path.join(this.platformPath, 'lib'), path.join(this.platformPath, 'modules'), this.platformPath ];

// module dependencies java archive paths
for (var module of this.modules) {
this.jarPaths.push(path.join(module.path));
this.jarPaths.push(path.join(module.path, 'lib'));
}

this.jarPaths.forEach(function (jarDir) {
fs.existsSync(jarDir) && fs.readdirSync(jarDir).forEach(function (name) {
var file = path.join(jarDir, name);
if (/\.jar$/.test(name) && fs.existsSync(file)) {
this.classPaths[file] = 1;
}
}, this);
}, this);

this.licenseFile = path.join(this.projectDir, 'LICENSE');
if (!fs.existsSync(this.licenseFile)) {
this.licenseFile = path.join(this.projectDir, '..', 'LICENSE');
Expand Down Expand Up @@ -376,7 +436,7 @@ AndroidModuleBuilder.prototype.processResources = function processResources(next
var mergedResPath = path.join(this.buildIntermediatesDir, 'res/merged');
var extraPackages = [];
var merge = function (src, dest) {
fs.readdirSync(src).forEach(function (filename) {
fs.existsSync(src) && fs.readdirSync(src).forEach(function (filename) {
var from = path.join(src, filename),
to = path.join(dest, filename);
if (fs.existsSync(from)) {
Expand Down
8 changes: 4 additions & 4 deletions android/cli/hooks/aar-transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,12 @@ function scanProjectAndStartTransform(builder, logger, callback) {
* @param {Function} callback Function to call once the transform is complete
*/
function scanModuleAndStartTransform(builder, logger, callback) {
const moduleAndroidLibraries = [];
const moduleAndroidLibraries = builder.moduleAndroidLibraries || [];
fs.readdirSync(builder.projLibDir).forEach(function (file) {
if (/\.aar/.test(file)) {
moduleAndroidLibraries.push({
aarPathAndFilename: path.join(builder.projLibDir, file),
originType: LIBRARY_ORIGIN_PORJECT
originType: LIBRARY_ORIGIN_MODULE
});
}
});
Expand Down Expand Up @@ -194,7 +194,7 @@ function transformAndroidLibraries(transformTasks, builder, buildVariant, logger
return done(null, hash);
}

logger.trace('Skipping ' + aarPathAndFilename.cyan + ' because it is a duplicate of ' + libraryHashMap[hash].aarPathAndFilename.cyan);
logger.trace('Skipping ' + aarPathAndFilename.cyan + ' because it is a duplicate of ' + libraryHashMap[hash].packageName.cyan);
done(new SkipLibraryError());
},

Expand Down Expand Up @@ -264,7 +264,7 @@ function transformAndroidLibraries(transformTasks, builder, buildVariant, logger
function formatDupeInfo(dupeLibraryInfo) {
var infoString = dupeLibraryInfo.task.aarPathAndFilename + ' (hash: ' + dupeLibraryInfo.sha256;
if (dupeLibraryInfo.task.originType === LIBRARY_ORIGIN_MODULE) {
infoString += ', origin: Module ' + dupeLibraryInfo.task.moduleInfo.id;
infoString += ', origin: Module';
} else if (dupeLibraryInfo.task.originType === LIBRARY_ORIGIN_PORJECT) {
infoString += ', origin: Project';
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
},
"dependencies": {
"adm-zip": "0.4.7",
"appc-aar-tools": "1.1.3",
"appc-aar-tools": "1.1.4",
"appc-tasks": "^1.0.1",
"archiver": "1.3.0",
"async": "2.3.0",
Expand Down