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

Fixes build & run related bugs from builder refactor #490

Merged
merged 1 commit into from
Sep 6, 2018
Merged

Fixes build & run related bugs from builder refactor #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions bin/lib/create.js
View file
Open in desktop
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
View file
Open in desktop
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();
Copy link
Member

Choose a reason for hiding this comment

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

Probably for the next round of cleaning, but there's a bunch of stuff around here that could be simplified with arrow functions and could use native Promises.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also agree. Some of the things I was thinking about in the clean up PR would be:

  • arrow functions
  • native promises
  • let and const
  • Use fs-extra to replace things like shell.cp etc.
  • Remove builders/builders.js

Copy link
Member

Choose a reason for hiding this comment

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

(Best create a cleanup issue with these tasks so they won't be forgotten)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

}.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
View file
Open in desktop
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.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these flags still work with an Android Studio build?

Copy link
Member Author

Choose a reason for hiding this comment

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

These flag should still work.

It appears that there is an issue where the flags are not being accepted. I am not sure when this issue was introduced but it appears that it also existed in the latest release.

I will create a separate ticket and PR to fix CLI parameter issue.

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
View file
Open in desktop
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
View file
Open in desktop
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 () {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you completely get rid of this? Or is this used from externally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will scan across our repos to verify.

I believe this entire file could be safely removed and point directly to the ProjectBuilder. I am not sure though if anyone uses this in their own personal build system but shouldn't affect us from removing if we don't use.

It might also be possible to do this type of task in separate PR as a final cleanup PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I scanned across repos and it seems only Android uses it. I will say its safe to remove, but I believe a cleanup PR will be better for this so we do not delay the fix.

I tried changing them and it ended up breaking around 70 tests. I would like to focus on that after.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, makes sense to do this individually as it is a major rip-out operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's best to leave this for another PR, as it would require a large refactor across the rest of the codebase.

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
View file
Open in desktop
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
View file
Open in desktop
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
Loading