You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by jasongin <gi...@git.apache.org> on 2016/04/19 23:01:45 UTC
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
GitHub user jasongin opened a pull request:
https://github.com/apache/cordova-android/pull/295
CB-11117: Use FileUpdater to optimize prepare for android platform
This uses the FileUpdater module added in https://github.com/apache/cordova-lib/pull/429 to optionally skip copying files that didn't change. Some refactoring was required because previously the target directories would just be wiped before copying; now we need to map out the source and target directories so the FileUpdater has the necessary information to determine the optimal set of file operations.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jasongin/cordova-android master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-android/pull/295.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #295
----
commit c8e504de768aa373874fd2f8895b497c5581652b
Author: Jason Ginchereau <ja...@microsoft.com>
Date: 2016-04-19T20:28:13Z
CB-11117: Use FileUpdater to optimize prepare for android platform
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61388176
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -84,29 +101,49 @@ function updateConfigFilesFrom(sourceConfig, configMunger, locations) {
}
/**
+ * Logs all file operations via the verbose event stream, indented.
+ */
+function logFileOp(message) {
+ events.emit('verbose', ' ' + message);
+}
+
+/**
* Updates platform 'www' directory by replacing it with contents of
* 'platform_www' and app www. Also copies project's overrides' folder into
* the platform 'www' folder
*
* @param {Object} cordovaProject An object which describes cordova project.
- * @param {Object} destinations An object that contains destination
- * paths for www files.
*/
-function updateWwwFrom(cordovaProject, destinations) {
- shell.rm('-rf', destinations.www);
- shell.mkdir('-p', destinations.www);
- // Copy source files from project's www directory
- shell.cp('-rf', path.join(cordovaProject.locations.www, '*'), destinations.www);
- // Override www sources by files in 'platform_www' directory
- shell.cp('-rf', path.join(destinations.platformWww, '*'), destinations.www);
+function updateWww(cordovaProject) {
+ var sourceDirs = [
+ path.relative(cordovaProject.root, cordovaProject.locations.www),
+ path.relative(cordovaProject.root, this.locations.platformWww)
+ ];
// If project contains 'merges' for our platform, use them as another overrides
var merges_path = path.join(cordovaProject.root, 'merges', 'android');
if (fs.existsSync(merges_path)) {
events.emit('verbose', 'Found "merges" for android platform. Copying over existing "www" files.');
- var overrides = path.join(merges_path, '*');
- shell.cp('-rf', overrides, destinations.www);
+ sourceDirs.push(path.join('merges', 'android'));
}
+
+ var targetDir = path.relative(cordovaProject.root, this.locations.www);
+ events.emit(
+ 'verbose', "Merging and updating files from [" + sourceDirs.join(", ") + "] to " + targetDir);
+ FileUpdater.mergeAndUpdateDir(
+ sourceDirs, targetDir, { rootDir: cordovaProject.root }, logFileOp);
+}
+
+/**
+ * Cleans all files from the platform 'www' directory.
+ */
+function cleanWww(projectRoot) {
+ var targetDir = path.relative(projectRoot, this.locations.www);
+ events.emit('verbose', "Cleaning " + targetDir);
+
+ // No source paths are specified, so mergeAndUpdateDir() will clear the target directory.
+ FileUpdater.mergeAndUpdateDir(
+ [], targetDir, { rootDir: projectRoot, all: true }, logFileOp);
--- End diff --
Also is there any advantage of using `FileUpdater` rather than just calling `shell.rm()`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61776793
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
--- End diff --
After discussion with Nikhil, we'd really like to keep the clean functionality. I'm adding a check for invocation via the non-CLI clean script; I hope that addresses your concern.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61462710
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
--- End diff --
I had been using `this` in a few more places, before I moved some things around. I agree it is not really needed now; I'll clean that up everywhere.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on the pull request:
https://github.com/apache/cordova-android/pull/295#issuecomment-215256784
@vladimir-kotikov @infil00p please review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61502339
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
--- End diff --
In CLI world i think there is not much sense in cleaning www folder because we treat platforms as build artifacts and hence this is it's something that is out of user control - instead we just need to ensure that every time when 'prepare' completes the user's 'www' dir and platform's 'www' dir are in sync.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61393298
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -295,25 +344,47 @@ function handleIcons(projectConfig, platformRoot) {
// The source paths for icons and splashes are relative to
// project's config.xml location, so we use it as base path.
- var projectRoot = path.dirname(projectConfig.path);
- var destination = path.join(platformRoot, 'res');
for (var density in android_icons) {
- copyImage(path.join(projectRoot, android_icons[density].src), destination, density, 'icon.png');
+ var targetPath = getImageResourcePath(
+ platformResourcesDir, density, 'icon.png', path.basename(android_icons[density].src));
+ resourceMap[targetPath] = android_icons[density].src;
}
+
// There's no "default" drawable, so assume default == mdpi.
if (default_icon && !android_icons.mdpi) {
- copyImage(path.join(projectRoot, default_icon.src), destination, 'mdpi', 'icon.png');
+ var targetPath = getImageResourcePath(
+ platformResourcesDir, 'mdpi', 'icon.png', path.basename(default_icon.src));
+ resourceMap[targetPath] = default_icon.src;
}
+
+ events.emit('verbose', 'Updating icons at ' + platformResourcesDir);
+ FileUpdater.updatePaths(
+ resourceMap, { rootDir: cordovaProject.root }, logFileOp);
}
-// remove the default resource name from all drawable folders
-function deleteDefaultResourceAt(baseDir, resourceName) {
- shell.ls(path.join(baseDir, 'res/drawable-*'))
+function cleanIcons(projectRoot, projectConfig, platformResourcesDir) {
+ var icons = projectConfig.getIcons('android');
+ if (icons.length > 0) {
+ var resourceMap = mapImageResources(projectRoot, platformResourcesDir, 'icon.png');
+ events.emit('verbose', 'Cleaning icons at ' + platformResourcesDir);
+
+ // No source paths are specified in the map, so updatePaths() will delete the target files.
+ FileUpdater.updatePaths(
--- End diff --
Just like in `cleanWww` - is there any advantage of calling this rather than doing `shell.rm` on these files? This would eliminate the need to know how `mapImageResources` and `FileUpdater.updatePaths` works internally and IMO improve readability a lot.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61383391
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
--- End diff --
I'd rather not to pass `this` reference to this method, especially if the only thing we need is a couple of properties from 'locations' object (`this.locations.platformWww` and `this.locations.www` at L120). Maybe just pass these properties as an arguments, or revert to previous signature?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-android/pull/295#issuecomment-216759612
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61504885
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -84,29 +101,49 @@ function updateConfigFilesFrom(sourceConfig, configMunger, locations) {
}
/**
+ * Logs all file operations via the verbose event stream, indented.
+ */
+function logFileOp(message) {
+ events.emit('verbose', ' ' + message);
+}
+
+/**
* Updates platform 'www' directory by replacing it with contents of
* 'platform_www' and app www. Also copies project's overrides' folder into
* the platform 'www' folder
*
* @param {Object} cordovaProject An object which describes cordova project.
- * @param {Object} destinations An object that contains destination
- * paths for www files.
*/
-function updateWwwFrom(cordovaProject, destinations) {
- shell.rm('-rf', destinations.www);
- shell.mkdir('-p', destinations.www);
- // Copy source files from project's www directory
- shell.cp('-rf', path.join(cordovaProject.locations.www, '*'), destinations.www);
- // Override www sources by files in 'platform_www' directory
- shell.cp('-rf', path.join(destinations.platformWww, '*'), destinations.www);
+function updateWww(cordovaProject) {
+ var sourceDirs = [
+ path.relative(cordovaProject.root, cordovaProject.locations.www),
+ path.relative(cordovaProject.root, this.locations.platformWww)
+ ];
// If project contains 'merges' for our platform, use them as another overrides
var merges_path = path.join(cordovaProject.root, 'merges', 'android');
if (fs.existsSync(merges_path)) {
events.emit('verbose', 'Found "merges" for android platform. Copying over existing "www" files.');
- var overrides = path.join(merges_path, '*');
- shell.cp('-rf', overrides, destinations.www);
+ sourceDirs.push(path.join('merges', 'android'));
}
+
+ var targetDir = path.relative(cordovaProject.root, this.locations.www);
+ events.emit(
+ 'verbose', "Merging and updating files from [" + sourceDirs.join(", ") + "] to " + targetDir);
+ FileUpdater.mergeAndUpdateDir(
+ sourceDirs, targetDir, { rootDir: cordovaProject.root }, logFileOp);
+}
+
+/**
+ * Cleans all files from the platform 'www' directory.
+ */
+function cleanWww(projectRoot) {
+ var targetDir = path.relative(projectRoot, this.locations.www);
+ events.emit('verbose', "Cleaning " + targetDir);
+
+ // No source paths are specified, so mergeAndUpdateDir() will clear the target directory.
+ FileUpdater.mergeAndUpdateDir(
+ [], targetDir, { rootDir: projectRoot, all: true }, logFileOp);
--- End diff --
I was mostly going for consistent logging: why log some file operations but not others? Our customer feedback indicates they would like more visibility into the cordova build process, and this kind of verbose logging will help with that.
Anyway this is going away now that I'm removing the clean function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61461619
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
--- End diff --
I guess I don't understand the non-CLI workflow very well. Why would 'cordova clean' be called in a non-CLI workflow, rather than 'gradle clean' or equivalent? Doesn't "non-CLI workflow" mean the cordova CLI commands like 'cordova clean' aren't being used?
In a normal cordova CLI workflow, I think users would expect that the files copied by prepare would be cleaned up by 'cordova clean' without any options; I was surprised to find they were not already.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61391425
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
+ var projectRoot = path.resolve(this.root, "../..");
+ var projectConfig = new ConfigParser(this.locations.configXml);
+ var platformResourcesDir = path.relative(projectRoot, path.join(this.locations.root, 'res'));
+
+ var self = this;
+ return Q().then(function () {
+ cleanWww.call(self, projectRoot);
+ cleanIcons.call(self, projectRoot, projectConfig, platformResourcesDir);
+ cleanSplashes.call(self, projectRoot, projectConfig, platformResourcesDir);
--- End diff --
Same here - `this` reference in not used inside of the functions above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61383879
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
--- End diff --
Same as above. I can't find any usage of `this` inside of these functions, so binding them to `this` is redundant
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61501081
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
--- End diff --
In fact there is no big difference between calling `gradle clean` and `cordova/clean.bat`but non-CLi workflow assumes that there is a contract between platform and user, according to which platform offers a set of shell scripts to control the aspects related to build system, and the `clean` is one of these scripts just as `build`, `run`, etc - you can find them inside of 'android/cordova' folder
One of advantages of calling `clean` rather that `gradle clean` is that you might not have `gradle` available in path, but `clean` contains some logic (similar to build) to find gradle wrapper, and call it with appropriate arguments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the pull request:
https://github.com/apache/cordova-android/pull/295#issuecomment-215259559
## [Current coverage][cc-pull] is **100%**
> Merging [#295][cc-pull] into [master][cc-base-branch] will not change coverage
```diff
@@ master #295 diff @@
========================================
Files 11 11
Lines 1707 1710 +3
Methods 207 208 +1
Messages 0 0
Branches 169 169
========================================
+ Hits 1707 1710 +3
Misses 0 0
Partials 0 0
```
> Powered by [Codecov](https://codecov.io?src=pr). Last updated by 610fa4a
[cc-base-branch]: https://codecov.io/gh/apache/cordova-android/branch/master?src=pr
[cc-pull]: https://codecov.io/gh/apache/cordova-android/pull/295?src=pr
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61503079
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
--- End diff --
I don't think everyone using CLI workflow treats platforms as build artifacts. Regardless, I think this kind of clean behavior is not needed now that incremental prepare does a stronger sync (copying different file times rather than just newer times). So I'll remove it for now. We can discuss the merits of something like 'cordova prepare --clean' later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61392785
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -26,26 +26,43 @@ var AndroidManifest = require('./AndroidManifest');
var xmlHelpers = require('cordova-common').xmlHelpers;
var CordovaError = require('cordova-common').CordovaError;
var ConfigParser = require('cordova-common').ConfigParser;
+var FileUpdater = require('cordova-common').FileUpdater;
-module.exports.prepare = function (cordovaProject) {
+module.exports.prepare = function (cordovaProject, options) {
var self = this;
+ var platformResourcesDir = path.relative(cordovaProject.root, path.join(this.locations.root, 'res'));
this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);
// Update own www dir with project's www assets and plugins' assets and js-files
- return Q.when(updateWwwFrom(cordovaProject, this.locations))
+ return Q.when(updateWww.call(self, cordovaProject))
.then(function () {
// update project according to config.xml changes.
return updateProjectAccordingTo(self._config, self.locations);
})
.then(function () {
- handleIcons(cordovaProject.projectConfig, self.root);
- handleSplashes(cordovaProject.projectConfig, self.root);
+ updateIcons.call(self, cordovaProject, platformResourcesDir);
+ updateSplashes.call(self, cordovaProject, platformResourcesDir);
})
.then(function () {
- events.emit('verbose', 'updated project successfully');
+ events.emit('verbose', 'Prepared project successfully');
+ });
+};
+
+module.exports.clean = function (options) {
+ // Unfortunately the cordovaProject isn't passed into the clean() function,
+ // but the project root dir and config can be resolved here easily.
--- End diff --
The `cordovaProject` is not passed here because the `clean` command is also accessible in non-CLI workflow through shell script (as opposed to `prepare`, which is intended to be called from CLI only). In this case resolving project's root is a bad idea, as there might not be any project at all.
Also, calling this in non-CLI project would destroy the project, as these files are not copied from the root project but were added by developer manually. IMO this functionality should be hidden behind the flag, so user would not wipe these files unintentionally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on the pull request:
https://github.com/apache/cordova-android/pull/295#issuecomment-216320625
I updated the PR based on feedback. As mentioned in the other comment I've kept the clean functionality, but it is now skipped when not invoked via the CLI.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimiz...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-android/pull/295#discussion_r61388100
--- Diff: bin/templates/cordova/lib/prepare.js ---
@@ -84,29 +101,49 @@ function updateConfigFilesFrom(sourceConfig, configMunger, locations) {
}
/**
+ * Logs all file operations via the verbose event stream, indented.
+ */
+function logFileOp(message) {
+ events.emit('verbose', ' ' + message);
+}
+
+/**
* Updates platform 'www' directory by replacing it with contents of
* 'platform_www' and app www. Also copies project's overrides' folder into
* the platform 'www' folder
*
* @param {Object} cordovaProject An object which describes cordova project.
- * @param {Object} destinations An object that contains destination
- * paths for www files.
*/
-function updateWwwFrom(cordovaProject, destinations) {
- shell.rm('-rf', destinations.www);
- shell.mkdir('-p', destinations.www);
- // Copy source files from project's www directory
- shell.cp('-rf', path.join(cordovaProject.locations.www, '*'), destinations.www);
- // Override www sources by files in 'platform_www' directory
- shell.cp('-rf', path.join(destinations.platformWww, '*'), destinations.www);
+function updateWww(cordovaProject) {
+ var sourceDirs = [
+ path.relative(cordovaProject.root, cordovaProject.locations.www),
+ path.relative(cordovaProject.root, this.locations.platformWww)
+ ];
// If project contains 'merges' for our platform, use them as another overrides
var merges_path = path.join(cordovaProject.root, 'merges', 'android');
if (fs.existsSync(merges_path)) {
events.emit('verbose', 'Found "merges" for android platform. Copying over existing "www" files.');
- var overrides = path.join(merges_path, '*');
- shell.cp('-rf', overrides, destinations.www);
+ sourceDirs.push(path.join('merges', 'android'));
}
+
+ var targetDir = path.relative(cordovaProject.root, this.locations.www);
+ events.emit(
+ 'verbose', "Merging and updating files from [" + sourceDirs.join(", ") + "] to " + targetDir);
+ FileUpdater.mergeAndUpdateDir(
+ sourceDirs, targetDir, { rootDir: cordovaProject.root }, logFileOp);
+}
+
+/**
+ * Cleans all files from the platform 'www' directory.
+ */
+function cleanWww(projectRoot) {
+ var targetDir = path.relative(projectRoot, this.locations.www);
+ events.emit('verbose', "Cleaning " + targetDir);
+
+ // No source paths are specified, so mergeAndUpdateDir() will clear the target directory.
+ FileUpdater.mergeAndUpdateDir(
+ [], targetDir, { rootDir: projectRoot, all: true }, logFileOp);
--- End diff --
Passing logger function here would produce a lot of output if `www` contains a lot of files. IMO everything that end user would need to know is that we're going to wipe all `www` contents
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request: CB-11117: Use FileUpdater to optimize prepare ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-android/pull/295
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org