You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by ag...@apache.org on 2014/03/10 21:30:50 UTC
[4/4] git commit: Fix to never remove top-level plugins that are
dependencies + tests.
Fix to never remove top-level plugins that are dependencies + tests.
github: close #58
Project: http://git-wip-us.apache.org/repos/asf/cordova-plugman/repo
Commit: http://git-wip-us.apache.org/repos/asf/cordova-plugman/commit/a4a51d7c
Tree: http://git-wip-us.apache.org/repos/asf/cordova-plugman/tree/a4a51d7c
Diff: http://git-wip-us.apache.org/repos/asf/cordova-plugman/diff/a4a51d7c
Branch: refs/heads/master
Commit: a4a51d7ce8200594bf90ecc524afbfc899c4e361
Parents: f58db0a
Author: jbondc <jb...@openmv.com>
Authored: Sun Mar 9 18:31:01 2014 -0400
Committer: Andrew Grieve <ag...@chromium.org>
Committed: Mon Mar 10 16:30:30 2014 -0400
----------------------------------------------------------------------
spec/common.js | 41 ++++++++++++++++-------
spec/install.spec.js | 9 ++---
spec/uninstall.spec.js | 80 ++++++++++++++++++++++++++++++++++++---------
src/uninstall.js | 49 ++++++++++++++++++++-------
4 files changed, 135 insertions(+), 44 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/a4a51d7c/spec/common.js
----------------------------------------------------------------------
diff --git a/spec/common.js b/spec/common.js
index 2991806..6a87e23 100644
--- a/spec/common.js
+++ b/spec/common.js
@@ -29,15 +29,34 @@ if(opt.debug) {
plugman.on('log', console.log);
}
-module.exports = {
- spy: {
- getInstall: function(emitSpy){
- var install = [], i;
- for(i in emitSpy.argsForCall) {
- if(emitSpy.argsForCall[i][1].substr(0, 13) === 'Install start')
- install.push(emitSpy.argsForCall[i][1]);
- }
- return install;
- }
- }
+module.exports = common = {
+ spy: {
+ getInstall: function(emitSpy){
+ return common.spy.startsWith(emitSpy, 'Install start');
+ },
+
+ getDeleted: function(emitSpy){
+ return common.spy.startsWith(emitSpy, 'Deleted');
+ },
+
+ startsWith: function(emitSpy, string)
+ {
+ var match = [], i;
+ for(i in emitSpy.argsForCall) {
+ if(emitSpy.argsForCall[i][1].substr(0, string.length) === string)
+ match.push(emitSpy.argsForCall[i][1]);
+ }
+ return match;
+ },
+
+ contains: function(emitSpy, string)
+ {
+ var match = [], i;
+ for(i in emitSpy.argsForCall) {
+ if(emitSpy.argsForCall[i][1].indexOf(string) >= 0)
+ match.push(emitSpy.argsForCall[i][1]);
+ }
+ return match;
+ }
+ }
};
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/a4a51d7c/spec/install.spec.js
----------------------------------------------------------------------
diff --git a/spec/install.spec.js b/spec/install.spec.js
index 5a40de3..f552c49 100644
--- a/spec/install.spec.js
+++ b/spec/install.spec.js
@@ -411,17 +411,14 @@ describe('end', function() {
it('end', function() {
done = false;
- var finish = function(err){
+
+ promise.fin(function(err){
if(err)
events.emit('error', err);
shell.rm('-rf', project);
done = true;
- }
-
- promise.then(
-
- ).fin(finish);
+ });
waitsFor(function() { return done; }, 'promise never resolved', 500);
});
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/a4a51d7c/spec/uninstall.spec.js
----------------------------------------------------------------------
diff --git a/spec/uninstall.spec.js b/spec/uninstall.spec.js
index 7ae8a14..bf9aa7f 100644
--- a/spec/uninstall.spec.js
+++ b/spec/uninstall.spec.js
@@ -13,9 +13,12 @@ var uninstall = require('../src/uninstall'),
done = false,
srcProject = path.join(spec, 'projects', 'android_uninstall'),
project = path.join(spec, 'projects', 'android_uninstall.test'),
+ project2 = path.join(spec, 'projects', 'android_uninstall.test2'),
plugins_dir = path.join(spec, 'plugins'),
plugins_install_dir = path.join(project, 'cordova', 'plugins'),
+ plugins_install_dir2 = path.join(project2, 'cordova', 'plugins'),
+
plugins = {
'DummyPlugin' : path.join(plugins_dir, 'DummyPlugin'),
'A' : path.join(plugins_dir, 'dependencies', 'A'),
@@ -32,7 +35,9 @@ describe('start', function() {
it('start', function() {
shell.rm('-rf', project);
+ shell.rm('-rf', project2);
shell.cp('-R', path.join(srcProject, '*'), project);
+ shell.cp('-R', path.join(srcProject, '*'), project2);
done = false;
promise = Q()
@@ -41,6 +46,10 @@ describe('start', function() {
).then(
function(){ return install('android', project, plugins['A']) }
).then(
+ function(){ return install('android', project2, plugins['C']) }
+ ).then(
+ function(){ return install('android', project2, plugins['A']) }
+ ).then(
function(){ done = true; }
);
waitsFor(function() { return done; }, 'promise never resolved', 500);
@@ -134,28 +143,67 @@ describe('uninstallPlatform', function() {
});
describe('uninstallPlugin', function() {
- var rm, fsWrite, rmstack = [];
+ var rm, fsWrite, rmstack = [], emit;
beforeEach(function() {
fsWrite = spyOn(fs, 'writeFileSync').andReturn(true);
rm = spyOn(shell, 'rm').andCallFake(function(f,p) { rmstack.push(p); return true});
rmstack = [];
+ emit = spyOn(events, 'emit');
done = false;
});
describe('with dependencies', function() {
- it('should delete all dangling plugins', function() {
-
+ it('should delete all dependent plugins', function() {
runs(function() {
uninstallPromise( uninstall.uninstallPlugin('A', plugins_install_dir) );
});
waitsFor(function() { return done; }, 'promise never resolved', 200);
runs(function() {
- expect(done).toEqual(true);
- expect(rmstack[0]).toBe( path.join(plugins_install_dir, 'C') );
- expect(rmstack[1]).toBe( path.join(plugins_install_dir, 'D') );
- expect(rmstack[2]).toBe( path.join(plugins_install_dir, 'A') );
- expect(rm.calls.length).toBe(3);
+ var del = common.spy.getDeleted(emit);
+
+ expect(del).toEqual([
+ 'Deleted "C"',
+ 'Deleted "D"',
+ 'Deleted "A"'
+ ]);
+ });
+ });
+
+ it("should fail if plugin is a required dependency", function() {
+ runs(function() {
+ uninstallPromise( uninstall.uninstallPlugin('C', plugins_install_dir) );
+ });
+ waitsFor(function() { return done; }, 'promise never resolved', 200);
+ runs(function() {
+ expect(done.message).toBe('"C" is required by (A) and cannot be removed (hint: use -f or --force)');
+ });
+ });
+
+ it("allow forcefully removing a plugin", function() {
+ runs(function() {
+ uninstallPromise( uninstall.uninstallPlugin('C', plugins_install_dir, {force: true}) );
+ });
+ waitsFor(function() { return done; }, 'promise never resolved', 200);
+ runs(function() {
+ expect(done).toBe(true);
+ var del = common.spy.getDeleted(emit);
+ expect(del).toEqual(['Deleted "C"']);
+ });
+ });
+
+ it("never remove top level plugins if they are a dependency", function() {
+ runs(function() {
+ uninstallPromise( uninstall.uninstallPlugin('A', plugins_install_dir2) );
+ });
+ waitsFor(function() { return done; }, 'promise never resolved', 200);
+ runs(function() {
+ var del = common.spy.getDeleted(emit);
+
+ expect(del).toEqual([
+ 'Deleted "D"',
+ 'Deleted "A"'
+ ]);
});
});
});
@@ -208,13 +256,6 @@ describe('end', function() {
it('end', function() {
done = false;
- var finish = function(err){
- if(err)
- plugman.emit('error', err);
-
- shell.rm('-rf', project);
- done = true;
- }
promise.then(
function(){
@@ -234,7 +275,14 @@ describe('end', function() {
// dependencies on C,D ... should this only work with --recursive? prompt user..?
return uninstall('android', project, plugins['A'])
}
- ).fin(finish);
+ ).fin(function(err){
+ if(err)
+ plugman.emit('error', err);
+
+ shell.rm('-rf', project);
+ shell.rm('-rf', project2);
+ done = true;
+ });
waitsFor(function() { return done; }, 'promise never resolved', 500);
});
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/a4a51d7c/src/uninstall.js
----------------------------------------------------------------------
diff --git a/src/uninstall.js b/src/uninstall.js
index 18d7a18..98c57ad 100644
--- a/src/uninstall.js
+++ b/src/uninstall.js
@@ -61,23 +61,27 @@ module.exports.uninstallPlugin = function(id, plugins_dir, options) {
var plugin_dir = path.join(plugins_dir, id);
+ // @tests - important this event is checked spec/uninstall.spec.js
+ events.emit('log', 'Removing "'+ id +'"');
+
// If already removed, skip.
- if (!fs.existsSync(plugin_dir)) {
+ if ( !fs.existsSync(plugin_dir) ) {
+ events.emit('verbose', 'Plugin "'+ id +'" already removed ('+ plugin_dir +')');
return Q();
}
var xml_path = path.join(plugin_dir, 'plugin.xml')
, plugin_et = xml_helpers.parseElementtreeSync(xml_path);
- events.emit('log', 'Deleting "'+ id +'"');
-
var doDelete = function(id) {
var plugin_dir = path.join(plugins_dir, id);
- if ( !fs.existsSync(plugin_dir) )
- return;
+ if ( !fs.existsSync(plugin_dir) ) {
+ events.emit('verbose', 'Plugin "'+ id +'" already removed ('+ plugin_dir +')');
+ return Q();
+ }
shell.rm('-rf', plugin_dir);
- events.emit('verbose', '"'+ id +'" deleted.');
+ events.emit('verbose', 'Deleted "'+ id +'"');
};
// We've now lost the metadata for the plugins that have been uninstalled, so we can't use that info.
@@ -85,7 +89,11 @@ module.exports.uninstallPlugin = function(id, plugins_dir, options) {
// anything depends on them, or if they're listed as top-level.
// If neither, they can be deleted.
var top_plugin_id = id;
- var toDelete = plugin_et.findall('dependency');
+
+ // Recursively remove plugins which were installed as dependents (that are not top-level)
+ // optional?
+ var recursive = true;
+ var toDelete = recursive ? plugin_et.findall('dependency') : [];
toDelete = toDelete && toDelete.length ? toDelete.map(function(p) { return p.attrib.id; }) : [];
toDelete.push(top_plugin_id);
@@ -95,14 +103,26 @@ module.exports.uninstallPlugin = function(id, plugins_dir, options) {
return fs.existsSync(path.join(plugins_dir, platform + '.json'));
});
+ // Can have missing plugins on some platforms when not supported..
var dependList = {};
- platforms.forEach(function(platform) {
+ platforms.forEach(function(platform) {
var depsInfo = dependencies.generate_dependency_info(plugins_dir, platform);
var tlps = depsInfo.top_level_plugins,
deps, i;
+ // Top-level deps must always be explicitely asked to remove by user
+ tlps.forEach(function(plugin_id){
+ if(top_plugin_id == plugin_id)
+ return;
+
+ var i = toDelete.indexOf(plugin_id);
+ if(i >= 0)
+ toDelete.splice(i, 1);
+ });
+
toDelete.forEach(function(plugin) {
deps = dependencies.dependents(plugin, depsInfo);
+
var i = deps.indexOf(top_plugin_id);
if(i >= 0)
deps.splice(i, 1); // remove current/top-level plugin as blocking uninstall
@@ -122,8 +142,15 @@ module.exports.uninstallPlugin = function(id, plugins_dir, options) {
if(options.force) {
events.emit('log', msg +' but forcing removal.');
} else {
- events.emit('warn', msg +' and cannot be removed (hint: use -f or --force)');
- continue;
+ // @tests - error and event message is checked spec/uninstall.spec.js
+ msg += ' and cannot be removed (hint: use -f or --force)';
+
+ if(plugin_id == top_plugin_id) {
+ return Q.reject( new Error(msg) );
+ } else {
+ events.emit('warn', msg +' and cannot be removed (hint: use -f or --force)');
+ continue;
+ }
}
}
@@ -163,7 +190,7 @@ function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugin
var promise;
if (deps && deps.length && danglers && danglers.length) {
-
+
// @tests - important this event is checked spec/uninstall.spec.js
events.emit('log', 'Uninstalling ' + danglers.length + ' dependent plugins.');
promise = Q.all(