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.

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

Conversation

erisu
Copy link
Member

@erisu erisu commented Sep 4, 2018

Platforms affected

Android

What does this PR do?

Fixes, refactors, and improves the build and run process that came from the builder refactor.

  • 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
  • Use the nobuild flag option to control the run build.
  • Updated test spec to reflect the changes.

What testing has been done on this change?

General Tests

  • npm test
  • npm run eslint

Default Template

  • cordova prepare android
  • cordova compile android

Builds Tested with Device

  • cordova build android
  • cordova build android --release --device
  • cordova build android --debug --device
  • cordova run android --device

Builds Tested on Emulator

  • cordova build android
  • cordova build android --release --emulator
  • cordova build android --debug --emulator
  • cordova run android --emulator

Checklist

@erisu
Copy link
Member Author

erisu commented Sep 4, 2018

Remaining Tasks

  • Revet Build Order

Removing the first build process would have been a better workflow but affects all platforms.
At this time, its best to keep the first build process and remove the second build process. This minimize the amount of refactoring needed at this time. Additionally, the second build process will be replaced with fetching of the buildResults (if available) and passed in to the run process to install onto the emulator or device.

  • Run Remaining Tests

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
Member

Choose a reason for hiding this comment

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

additional dot at the end

@@ -143,7 +143,7 @@ function writeProjectProperties (projectPath, target_api) {
// This makes no sense, what if you're building with a different build system?
function prepBuildFiles (projectPath, builder) {
Copy link
Member

Choose a reason for hiding this comment

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

second param probably not needed any more here

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));
return Object.keys(this.binDirs).reduce((result, builderName) => {
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 binDirs and this code be simplified to just a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I also think this can be simplified.

Originally I was going to but decided to wait and clean up the other other areas first. Thanks for reminding me this one. I will take a look at it again.

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.

@codecov-io
Copy link

codecov-io commented Sep 5, 2018

Codecov Report

Merging #490 into master will increase coverage by 1.73%.
The diff coverage is 78.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage    59.9%   61.64%   +1.73%     
==========================================
  Files          16       16              
  Lines        1973     1945      -28     
  Branches      380      363      -17     
==========================================
+ Hits         1182     1199      +17     
+ Misses        791      746      -45
Impacted Files Coverage Δ
...n/templates/cordova/lib/builders/ProjectBuilder.js 45.12% <100%> (+5.33%) ⬆️
bin/templates/cordova/lib/run.js 100% <100%> (ø) ⬆️
bin/templates/cordova/lib/pluginHandlers.js 85.71% <100%> (-1%) ⬇️
bin/templates/cordova/lib/builders/builders.js 100% <100%> (ø) ⬆️
bin/templates/cordova/lib/emulator.js 89.92% <100%> (+0.3%) ⬆️
bin/templates/cordova/lib/build.js 28.46% <20%> (+15.41%) ⬆️
bin/templates/cordova/Api.js 43.61% <42.85%> (+2.03%) ⬆️
bin/lib/create.js 54.69% <66.66%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ab0cf1...1769a9d. Read the comment docs.

- 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.
@erisu erisu changed the title WIP: Fixes build & run related bugs from builder refactor Fixes build & run related bugs from builder refactor Sep 5, 2018
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.

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

👍 from me

Copy link
Contributor

@Menardi Menardi left a comment

Choose a reason for hiding this comment

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

Looks good, nice work @erisu. I tested running an Android project, and it fixes #488. 👍

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.

module.exports.getBuilder = function (builderType, projectRoot) {
if (!knownBuilders[builderType]) { throw new CordovaError('Builder ' + builderType + ' is not supported.'); }

module.exports.getBuilder = function () {
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants