Skip to content

Commit

Permalink
Fixes build & run related bugs from PR apache#461
Browse files Browse the repository at this point in the history
- General Code Refactor
- Removed builder type argument from getBuilder API
- Removed any reference of conditional statements around builder type
- Remove plugin handler install and uninstall option flag android_studio
- Remove --gradle flag references
- Fixed plugin handler install and uninstall pathing issues
- Added parseBuildOptions export so run can get build related options.
- Use the nobuild flag option to control the run build.
- Updated test spec to reflect the changes.
  • Loading branch information
erisu committed Sep 5, 2018
1 parent 7ab0cf1 commit 4d06957
Show file tree
Hide file tree
Showing 13 changed files with 81 additions and 188 deletions.
6 changes: 3 additions & 3 deletions bin/lib/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ function writeProjectProperties (projectPath, target_api) {
}

// This makes no sense, what if you're building with a different build system?
function prepBuildFiles (projectPath, builder) {
function prepBuildFiles (projectPath) {
var buildModule = require(path.resolve(projectPath, 'cordova/lib/builders/builders'));
buildModule.getBuilder(builder).prepBuildFiles();
buildModule.getBuilder().prepBuildFiles();
}

function copyBuildRules (projectPath, isLegacy) {
Expand Down Expand Up @@ -329,7 +329,7 @@ exports.create = function (project_path, config, options, events) {
});
// Link it to local android install.
exports.writeProjectProperties(project_path, target_api);
exports.prepBuildFiles(project_path, 'studio');
exports.prepBuildFiles(project_path);
events.emit('log', generateDoneMessage('create', options.link));
}).thenResolve(project_path);
};
Expand Down
48 changes: 19 additions & 29 deletions bin/templates/cordova/Api.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,23 +55,23 @@ function setupEvents (externalEventEmitter) {
function Api (platform, platformRootDir, events) {
this.platform = PLATFORM;
this.root = path.resolve(__dirname, '..');
this.builder = 'studio';

setupEvents(events);

var self = this;
const appMain = path.join(this.root, 'app', 'src', 'main');
const appRes = path.join(appMain, 'res');

this.locations = {
root: self.root,
www: path.join(self.root, 'app/src/main/assets/www'),
res: path.join(self.root, 'app/src/main/res'),
platformWww: path.join(self.root, 'platform_www'),
configXml: path.join(self.root, 'app/src/main/res/xml/config.xml'),
defaultConfigXml: path.join(self.root, 'cordova/defaults.xml'),
strings: path.join(self.root, 'app/src/main/res/values/strings.xml'),
manifest: path.join(self.root, 'app/src/main/AndroidManifest.xml'),
build: path.join(self.root, 'build'),
javaSrc: path.join(self.root, 'app/src/main/java/'),
root: this.root,
www: path.join(appMain, 'assets', 'www'),
res: appRes,
platformWww: path.join(this.root, 'platform_www'),
configXml: path.join(appRes, 'xml', 'config.xml'),
defaultConfigXml: path.join(this.root, 'cordova', 'defaults.xml'),
strings: path.join(appRes, 'values', 'strings.xml'),
manifest: path.join(appMain, 'AndroidManifest.xml'),
build: path.join(this.root, 'build'),
javaSrc: path.join(appMain, 'java'),
// NOTE: Due to platformApi spec we need to return relative paths here
cordovaJs: 'bin/templates/project/assets/www/cordova.js',
cordovaJsSrc: 'cordova-js-src'
Expand Down Expand Up @@ -208,17 +208,13 @@ Api.prototype.addPlugin = function (plugin, installOptions) {
installOptions.variables.PACKAGE_NAME = project.getPackageName();
}

if (this.android_studio === true) {
installOptions.android_studio = true;
}

return Q().then(function () {
return PluginManager.get(self.platform, self.locations, project).addPlugin(plugin, installOptions);
}).then(function () {
if (plugin.getFrameworks(this.platform).length === 0) return;
selfEvents.emit('verbose', 'Updating build files since android plugin contained <framework>');
// This should pick the correct builder, not just get gradle
require('./lib/builders/builders').getBuilder(this.builder).prepBuildFiles();
require('./lib/builders/builders').getBuilder().prepBuildFiles();
}.bind(this))
// CB-11022 Return truthy value to prevent running prepare after
.thenResolve(true);
Expand All @@ -240,9 +236,8 @@ Api.prototype.addPlugin = function (plugin, installOptions) {
Api.prototype.removePlugin = function (plugin, uninstallOptions) {
var project = AndroidProject.getProjectFile(this.root);

if (uninstallOptions && uninstallOptions.usePlatformWww === true && this.android_studio === true) {
if (uninstallOptions && uninstallOptions.usePlatformWww === true) {
uninstallOptions.usePlatformWww = false;
uninstallOptions.android_studio = true;
}

return PluginManager.get(this.platform, this.locations, project)
Expand All @@ -251,7 +246,7 @@ Api.prototype.removePlugin = function (plugin, uninstallOptions) {
if (plugin.getFrameworks(this.platform).length === 0) return;

selfEvents.emit('verbose', 'Updating build files since android plugin contained <framework>');
require('./lib/builders/builders').getBuilder(this.builder).prepBuildFiles();
require('./lib/builders/builders').getBuilder().prepBuildFiles();
}.bind(this))
// CB-11022 Return truthy value to prevent running prepare after
.thenResolve(true);
Expand Down Expand Up @@ -304,9 +299,7 @@ Api.prototype.removePlugin = function (plugin, uninstallOptions) {
*/
Api.prototype.build = function (buildOptions) {
var self = this;
if (this.android_studio) {
buildOptions.studio = true;
}

return require('./lib/check_reqs').run().then(function () {
return require('./lib/build').run.call(self, buildOptions);
}).then(function (buildResults) {
Expand Down Expand Up @@ -350,12 +343,9 @@ Api.prototype.run = function (runOptions) {
*/
Api.prototype.clean = function (cleanOptions) {
var self = this;
if (this.android_studio) {
// This will lint, checking for null won't
if (typeof cleanOptions === 'undefined') {
cleanOptions = {};
}
cleanOptions.studio = true;
// This will lint, checking for null won't
if (typeof cleanOptions === 'undefined') {
cleanOptions = {};
}

return require('./lib/check_reqs').run().then(function () {
Expand Down
32 changes: 8 additions & 24 deletions bin/templates/cordova/lib/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,10 @@ var events = require('cordova-common').events;
var spawn = require('cordova-common').superspawn.spawn;
var CordovaError = require('cordova-common').CordovaError;

module.exports.parseBuildOptions = parseOpts;
function parseOpts (options, resolvedTarget, projectRoot) {
options = options || {};
options.argv = nopt({
gradle: Boolean,
studio: Boolean,
prepenv: Boolean,
versionCode: String,
minSdkVersion: String,
Expand All @@ -50,26 +49,13 @@ function parseOpts (options, resolvedTarget, projectRoot) {
// Android Studio Build method is the default
var ret = {
buildType: options.release ? 'release' : 'debug',
buildMethod: process.env.ANDROID_BUILD || 'studio',
prepEnv: options.argv.prepenv,
arch: resolvedTarget && resolvedTarget.arch,
extraArgs: []
};

if (options.argv.gradle || options.argv.studio) {
ret.buildMethod = options.argv.studio ? 'studio' : 'gradle';
}

// This comes from cordova/run
if (options.studio) ret.buildMethod = 'studio';
if (options.gradle) ret.buildMethod = 'gradle';

if (options.nobuild) ret.buildMethod = 'none';

if (options.argv.versionCode) { ret.extraArgs.push('-PcdvVersionCode=' + options.argv.versionCode); }

if (options.argv.minSdkVersion) { ret.extraArgs.push('-PcdvMinSdkVersion=' + options.argv.minSdkVersion); }

if (options.argv.gradleArg) {
ret.extraArgs = ret.extraArgs.concat(options.argv.gradleArg);
}
Expand Down Expand Up @@ -129,7 +115,8 @@ function parseOpts (options, resolvedTarget, projectRoot) {
*/
module.exports.runClean = function (options) {
var opts = parseOpts(options, null, this.root);
var builder = builders.getBuilder(opts.buildMethod);
var builder = builders.getBuilder();

return builder.prepEnv(opts).then(function () {
return builder.clean(opts);
});
Expand All @@ -149,8 +136,8 @@ module.exports.runClean = function (options) {
*/
module.exports.run = function (options, optResolvedTarget) {
var opts = parseOpts(options, optResolvedTarget, this.root);
console.log(opts.buildMethod);
var builder = builders.getBuilder(opts.buildMethod);
var builder = builders.getBuilder();

return builder.prepEnv(opts).then(function () {
if (opts.prepEnv) {
events.emit('verbose', 'Build file successfully prepared.');
Expand All @@ -161,8 +148,7 @@ module.exports.run = function (options, optResolvedTarget) {
events.emit('log', 'Built the following apk(s): \n\t' + apkPaths.join('\n\t'));
return {
apkPaths: apkPaths,
buildType: opts.buildType,
buildMethod: opts.buildMethod
buildType: opts.buildType
};
});
});
Expand Down Expand Up @@ -276,12 +262,10 @@ module.exports.help = function () {
console.log('Flags:');
console.log(' \'--debug\': will build project in debug mode (default)');
console.log(' \'--release\': will build project for release');
console.log(' \'--ant\': will build project with ant');
console.log(' \'--gradle\': will build project with gradle (default)');
console.log(' \'--nobuild\': will skip build process (useful when using run command)');
console.log(' \'--prepenv\': don\'t build, but copy in build scripts where necessary');
console.log(' \'--versionCode=#\': Override versionCode for this build. Useful for uploading multiple APKs. Requires --gradle.');
console.log(' \'--minSdkVersion=#\': Override minSdkVersion for this build. Useful for uploading multiple APKs. Requires --gradle.');
console.log(' \'--versionCode=#\': Override versionCode for this build. Useful for uploading multiple APKs.');
console.log(' \'--minSdkVersion=#\': Override minSdkVersion for this build. Useful for uploading multiple APKs.');
console.log(' \'--gradleArg=<gradle command line arg>\': Extra args to pass to the gradle command. Use one flag per arg. Ex. --gradleArg=-PcdvBuildMultipleApks=true');
console.log('');
console.log('Signed APK flags (overwrites debug/release-signing.proprties) :');
Expand Down
22 changes: 11 additions & 11 deletions bin/templates/cordova/lib/builders/ProjectBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,9 @@ const TEMPLATE =
'# Do not modify this file -- ' + MARKER + '\n';

class ProjectBuilder {
constructor (projectRoot) {
this.root = projectRoot || path.resolve(__dirname, '../../..');
this.binDirs = {
studio: path.join(this.root, 'app', 'build', 'outputs', 'apk'),
gradle: path.join(this.root, 'app', 'build', 'outputs', 'apk')
};
constructor (rootDirectory) {
this.root = rootDirectory || path.resolve(__dirname, '../../..');
this.binDir = path.join(this.root, 'app', 'build', 'outputs', 'apk');
}

getArgs (cmd, opts) {
Expand Down Expand Up @@ -295,11 +292,14 @@ class ProjectBuilder {
}

findOutputApks (build_type, arch) {
var self = this;
return Object.keys(this.binDirs).reduce(function (result, builderName) {
var binDir = self.binDirs[builderName];
return result.concat(findOutputApksHelper(binDir, build_type, builderName === 'ant' ? null : arch));
}, []).sort(apkSorter);
return findOutputApksHelper(this.binDir, build_type, arch).sort(apkSorter);
}

fetchBuildResults (build_type, arch) {
return {
apkPaths: this.findOutputApks(build_type, arch),
buildType: build_type
};
}
}

Expand Down
25 changes: 7 additions & 18 deletions bin/templates/cordova/lib/builders/builders.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,18 @@
under the License.
*/

var CordovaError = require('cordova-common').CordovaError;

var knownBuilders = {
gradle: 'ProjectBuilder',
studio: 'ProjectBuilder'
};
const CordovaError = require('cordova-common').CordovaError;

/**
* Helper method that instantiates and returns a builder for specified build
* type.
* Helper method that instantiates and returns a builder for specified build type.
*
* @param {String} builderType Builder name to construct and return. Must
* be one of gradle' or 'studio'
*
* @return {Builder} A builder instance for specified build type.
* @return {Builder} A builder instance for specified build type.
*/
module.exports.getBuilder = function (builderType, projectRoot) {
if (!knownBuilders[builderType]) { throw new CordovaError('Builder ' + builderType + ' is not supported.'); }

module.exports.getBuilder = function () {
try {
var Builder = require('./' + knownBuilders[builderType]);
return new Builder(projectRoot);
const Builder = require('./ProjectBuilder');
return new Builder();
} catch (err) {
throw new CordovaError('Failed to instantiate ' + knownBuilders[builderType] + ' builder: ' + err);
throw new CordovaError('Failed to instantiate ProjectBuilder builder: ' + err);
}
};
9 changes: 3 additions & 6 deletions bin/templates/cordova/lib/emulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -434,12 +434,9 @@ module.exports.install = function (givenTarget, buildResults) {

var target;
// We need to find the proper path to the Android Manifest
var manifestPath = path.join(__dirname, '..', '..', 'app', 'src', 'main', 'AndroidManifest.xml');
if (buildResults.buildMethod === 'gradle') {
manifestPath = path.join(__dirname, '../../AndroidManifest.xml');
}
var manifest = new AndroidManifest(manifestPath);
var pkgName = manifest.getPackageId();
const manifestPath = path.join(__dirname, '..', '..', 'app', 'src', 'main', 'AndroidManifest.xml');
const manifest = new AndroidManifest(manifestPath);
const pkgName = manifest.getPackageId();

// resolve the target emulator
return Promise.resolve().then(function () {
Expand Down
35 changes: 6 additions & 29 deletions bin/templates/cordova/lib/pluginHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,7 @@ var handlers = {
if (!obj.src) throw new CordovaError(generateAttributeError('src', 'source-file', plugin.id));
if (!obj.targetDir) throw new CordovaError(generateAttributeError('target-dir', 'source-file', plugin.id));

var dest = path.join(obj.targetDir, path.basename(obj.src));

// TODO: This code needs to be replaced, since the core plugins need to be re-mapped to a different location in
// a later plugins release. This is for legacy plugins to work with Cordova.

if (options && options.android_studio === true) {
dest = studioPathRemap(obj);
}
var dest = studioPathRemap(obj);

if (options && options.force) {
copyFile(plugin.dir, obj.src, project.projectDir, dest, !!(options && options.link));
Expand All @@ -42,11 +35,7 @@ var handlers = {
}
},
uninstall: function (obj, plugin, project, options) {
var dest = path.join(obj.targetDir, path.basename(obj.src));

if (options && options.android_studio === true) {
dest = studioPathRemap(obj);
}
var dest = studioPathRemap(obj);

// TODO: Add Koltin extension to uninstall, since they are handled like Java files
if (obj.src.endsWith('java')) {
Expand All @@ -59,33 +48,21 @@ var handlers = {
},
'lib-file': {
install: function (obj, plugin, project, options) {
var dest = path.join('libs', path.basename(obj.src));
if (options && options.android_studio === true) {
dest = path.join('app/libs', path.basename(obj.src));
}
var dest = path.join('app/libs', path.basename(obj.src));
copyFile(plugin.dir, obj.src, project.projectDir, dest, !!(options && options.link));
},
uninstall: function (obj, plugin, project, options) {
var dest = path.join('libs', path.basename(obj.src));
if (options && options.android_studio === true) {
dest = path.join('app/libs', path.basename(obj.src));
}
var dest = path.join('app/libs', path.basename(obj.src));
removeFile(project.projectDir, dest);
}
},
'resource-file': {
install: function (obj, plugin, project, options) {
var dest = path.normalize(obj.target);
if (options && options.android_studio === true) {
dest = path.join('app/src/main', dest);
}
var dest = path.join('app', 'src', 'main', obj.target);
copyFile(plugin.dir, obj.src, project.projectDir, dest, !!(options && options.link));
},
uninstall: function (obj, plugin, project, options) {
var dest = path.normalize(obj.target);
if (options && options.android_studio === true) {
dest = path.join('app/src/main', dest);
}
var dest = path.join('app', 'src', 'main', obj.target);
removeFile(project.projectDir, dest);
}
},
Expand Down
9 changes: 7 additions & 2 deletions bin/templates/cordova/lib/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
*/

var path = require('path');
var build = require('./build');
var emulator = require('./emulator');
var device = require('./device');
var Q = require('q');
Expand Down Expand Up @@ -50,6 +49,7 @@ function getInstallTarget (runOptions) {
* @return {Promise}
*/
module.exports.run = function (runOptions) {
runOptions = runOptions || {};

var self = this;
var install_target = getInstallTarget(runOptions);
Expand Down Expand Up @@ -105,12 +105,17 @@ module.exports.run = function (runOptions) {
// build results (according to platformApi spec) so they are in different
// format than emulator.install expects.
// TODO: Update emulator/device.install to handle this change
return build.run.call(self, runOptions, resolvedTarget).then(function (buildResults) {
return new Promise((resolve) => {
const builder = require('./builders/builders').getBuilder();
const buildOptions = require('./build').parseBuildOptions(runOptions, null, self.root);
resolve(builder.fetchBuildResults(buildOptions.buildType, buildOptions.arch));
}).then(function (buildResults) {
if (resolvedTarget && resolvedTarget.isEmulator) {
return emulator.wait_for_boot(resolvedTarget.target).then(function () {
return emulator.install(resolvedTarget, buildResults);
});
}

return device.install(resolvedTarget, buildResults);
});
});
Expand Down
Loading

0 comments on commit 4d06957

Please sign in to comment.