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-24734](6_1_X) Add compatibility with Hyperloop AAR handling #9087

Merged
merged 5 commits into from
May 26, 2017
Merged

[TIMOB-24734](6_1_X) Add compatibility with Hyperloop AAR handling #9087

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
726ce44
[TIMOB-24734] Add compatibility with Hyperloop AAR handling
janvennemann May 25, 2017
b821e0e
Merge branch '6_1_X' into TIMOB-24734-6_1_X
May 25, 2017
d39f31d
[TIMOB-24734] Apply fix to Hyperloop < 2.2.0 only
janvennemann May 26, 2017
fc6982d
[TIMOB-24734] Apply code style fixes and improve error handling
janvennemann May 26, 2017
4e79519
Merge branch '6_1_X' into TIMOB-24734-6_1_X
cb1kenobi May 26, 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
132 changes: 95 additions & 37 deletions android/cli/hooks/aar-transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/

var AarTransformer = require('appc-aar-tools').AarTransformer;
var appc = require('node-appc');
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do this. init() is passed a 4th argument which is a node-appc instance.

exports.init = function (logger, config, cli, appc) {

You'll have to pass appc into registerHyperloopCompatibilityFixes().

var async = require('async');
var crypto = require('crypto');
var fs = require('fs');
Expand Down Expand Up @@ -45,6 +46,7 @@ exports.init = function (logger, config, cli) {
cli.on('build.pre.compile', {
post: function(builder, callback) {
scanProjectAndStartTransform(builder, logger, callback);
registerHyperloopCompatibilityFixes(cli, builder, logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

scanProjectAndStartTransform() is async. The call to registerHyperloopCompatibilityFixes() must be done in scanProjectAndStartTransform()'s callback or if there's no conflict, registerHyperloopCompatibilityFixes() could simply be run before calling scanProjectAndStartTransform().

scanProjectAndStartTransform(builder, logger, function (err) {
    if (!err) {
        registerHyperloopCompatibilityFixes(cli, builder, logger);
    }
    callback(err);
});

}
});

Expand All @@ -53,43 +55,6 @@ exports.init = function (logger, config, cli) {
scanModuleAndStartTransform(builder, logger, callback);
}
});

cli.on('build.android.dexer', {
priority: 1100,
/**
* Fixes an issue with Hyperloop 2.1.0 which causes a crash when trying to
* override the Android Support Libraries with local .aar files. Hyperloop
* 2.1.0 will always manually add our bundled Android Support Libraries
* to the dexer paths even if they were replaced by the builder. To fix this
* we check the altered dexer paths again and remove any replaced libraries.
*
* @param {Object} data Hook data
* @param {Function} callback Callback function
*/
pre: function(data, callback) {
var builder= data.ctx;
var dexerOptions = data.args[1].slice(0, 6);
var dexerPaths = data.args[1].slice(6);
var hyperloopModule = null;
builder.nativeLibModules.forEach(function (module) {
if (module.id === 'hyperloop' && module.version === '2.1.0') {
hyperloopModule = module;
}
});
if (hyperloopModule && builder.androidLibraries.length > 0) {
var fixedDexerPaths = [];
dexerPaths.forEach(function (entryPathAndFilename) {
if (!this.isExternalAndroidLibraryAvailable(entryPathAndFilename)) {
fixedDexerPaths.push(entryPathAndFilename);
} else {
logger.trace('Removed duplicate library ' + entryPathAndFilename + ' from dexer paths.');
}
}, builder);
data.args[1] = dexerOptions.concat(fixedDexerPaths);
}
callback();
}
});
};

/**
Expand Down Expand Up @@ -481,3 +446,96 @@ class SimpleFileCache {
fs.writeFileSync(this.cachePathAndFilename, dataToWrite);
}
}

/**
* Hyperloop versions below 2.2.0 have their own AAR handling which we need to
* disable by hooking into the affected hooks and reverting the changes Hyperloop
* made.
*
* @param {Object} cli CLI instance
* @param {Object} builder Builder instance
* @param {Object} logger Logger instance
*/
function registerHyperloopCompatibilityFixes(cli, builder, logger) {
var hyperloopModule = null;
builder.nativeLibModules.forEach(function (module) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a builder.nativeLibModules.some() call and when you find the hyperloop module, return true; to break the looping.

if (module.id === 'hyperloop' && appc.version.lt(module.version, '2.2.0')) {
hyperloopModule = module;
}
});
if (hyperloopModule === null) {
return;
}

var hyperloopBuildPath = path.join(builder.projectDir, 'build/hyperloop/android');

cli.on('build.android.aapt', {
priority: 1100,
/**
* Remove parameters passed to AAPT which are not required anymore since 6.1.0
*
* @param {Object} data Hook data
* @param {Function} callback Callback function
*/
pre: function(data, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our JavaScript coding style mandates there be a space between constructs and parenthesis. This rule applies to function and while as well as if.

logger.trace('Cleaning AAPT options from changes made by Hyperloop');
var aaptOptions = data.args[1];
var extraPackagesIndex = aaptOptions.indexOf('--extra-packages') + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is insufficient. indexof() returns -1 if --extra-packages is not found. You have no error handling around this. If it is not found, then extraPackages will always be set to aaptOptions[0].

var extraPackages = aaptOptions[extraPackagesIndex];
var parameterIndex = aaptOptions.indexOf('-S');
while(parameterIndex !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space after while.

var resourcePath = aaptOptions[parameterIndex + 1];
if (resourcePath.indexOf(hyperloopBuildPath) !== -1) {
var manifestPathAndFilename = path.join(resourcePath, '../AndroidManifest.xml');
if (fs.existsSync(manifestPathAndFilename)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, fs.existsSync() has been deprecated. It's fine for now because it's everywhere else and we'd have to polyfill it anyways. The preferred way is calling fs.statSync() with a try/catch.

var manifestContent = fs.readFileSync(manifestPathAndFilename).toString();
var packageNameMatch = manifestContent.match(/package="(.*)"/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is in a loop, it is preferred that the regex be compiled ahead of time.

if (packageNameMatch !== null) {
var packageName = packageNameMatch[1];
extraPackages = extraPackages.replace(':' + packageName, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

It's safer to split on the separator and re-join:

extraPackages = extraPackages.split(':').filter(n => n === packageName).join(':');

logger.trace('Removed package ' + packageName + ' from AAPT --extra-packages option');
}
}
aaptOptions.splice(parameterIndex, 2);
logger.trace('Removed AAPT -S resource path ' + resourcePath);
parameterIndex = aaptOptions.indexOf('-S', parameterIndex);
} else {
parameterIndex = aaptOptions.indexOf('-S', parameterIndex + 1);
}
}
aaptOptions[extraPackagesIndex] = extraPackages;
callback();
}
});


cli.on('build.android.dexer', {
priority: 1100,
/**
* Fixes repeated adding of the same JAR to the dexer to avoid crashing
* with a dreaded "already added" exception
*
* @param {Object} data Hook data
* @param {Function} callback Callback function
*/
pre: function(data, callback) {
logger.trace('Cleaning dexer paths from changes made by Hyperloop');
var builder = data.ctx;
var dexerOptions = data.args[1].slice(0, 6);
var dexerPaths = data.args[1].slice(6);
if (builder.androidLibraries.length > 0) {
var fixedDexerPaths = [];
dexerPaths.forEach(function (entryPathAndFilename) {
var isHyperloopExtractedAarPath = entryPathAndFilename.indexOf(hyperloopBuildPath) !== -1;
if (builder.isExternalAndroidLibraryAvailable(entryPathAndFilename) || isHyperloopExtractedAarPath) {
logger.trace('Removed ' + entryPathAndFilename + ' from dexer paths.');
} else {
fixedDexerPaths.push(entryPathAndFilename);
}
}, builder);
data.args[1] = dexerOptions.concat(fixedDexerPaths);
}
callback();
}
});
}