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 <[email protected]> Authored: Sun Mar 9 18:31:01 2014 -0400 Committer: Andrew Grieve <[email protected]> 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(
