You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by br...@apache.org on 2013/10/21 22:19:48 UTC
git commit: Overhaul dependency uninstallation
Updated Branches:
refs/heads/master 45c9fd4f9 -> 0596e47f8
Overhaul dependency uninstallation
This fixes several long-standing bugs with plugman deleting
dependencies. The new rules are to uninstall all of the target plugin's
dependencies, provided they are:
- Not installed at the top level.
- Not depended on by any other plugins.
Similarly, plugin directories are only deleted when no plugin depends on
them, and they are not top-level, on any platform.
Fixes CB-5056, CB-5060, CB-4992, and CB-4189.
Project: http://git-wip-us.apache.org/repos/asf/cordova-plugman/repo
Commit: http://git-wip-us.apache.org/repos/asf/cordova-plugman/commit/0596e47f
Tree: http://git-wip-us.apache.org/repos/asf/cordova-plugman/tree/0596e47f
Diff: http://git-wip-us.apache.org/repos/asf/cordova-plugman/diff/0596e47f
Branch: refs/heads/master
Commit: 0596e47f870a13d8658ebb27f01f70fffcec5e23
Parents: 45c9fd4
Author: Braden Shepherdson <br...@gmail.com>
Authored: Thu Oct 17 12:33:00 2013 -0400
Committer: Braden Shepherdson <br...@gmail.com>
Committed: Mon Oct 21 16:18:51 2013 -0400
----------------------------------------------------------------------
spec/uninstall.spec.js | 40 +++++++++-------
src/uninstall.js | 105 +++++++++++++++++++++++++-----------------
src/util/dependencies.js | 39 ++++++++++++++++
src/util/plugins.js | 15 ++++++
4 files changed, 139 insertions(+), 60 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/0596e47f/spec/uninstall.spec.js
----------------------------------------------------------------------
diff --git a/spec/uninstall.spec.js b/spec/uninstall.spec.js
index 9d9add4..733b7b0 100644
--- a/spec/uninstall.spec.js
+++ b/spec/uninstall.spec.js
@@ -3,6 +3,7 @@ var uninstall = require('../src/uninstall'),
config_changes = require('../src/util/config-changes'),
dependencies = require('../src/util/dependencies'),
xml_helpers = require('../src/util/xml-helpers'),
+ plugins = require('../src/util/plugins.js'),
plugman = require('../plugman'),
fs = require('fs'),
os = require('osenv'),
@@ -79,7 +80,7 @@ describe('uninstallPlatform', function() {
});
describe('with dependencies', function() {
- var parseET, emit;
+ var parseET, emit, danglers, dependents;
beforeEach(function() {
emit = spyOn(plugman, 'emit');
parseET = spyOn(xml_helpers, 'parseElementtreeSync').andReturn({
@@ -89,8 +90,10 @@ describe('uninstallPlatform', function() {
find:function() { return null },
findall:function() { return [] }
});
+ danglers = spyOn(dependencies, 'danglers');
+ dependents = spyOn(dependencies, 'dependents').andReturn([]);
});
- it('should uninstall "dangling" dependencies', function() {
+ it('should uninstall "dangling" dependencies', function(done) {
// TODO: this is a terrible way to do conditional mocking, i am sorry.
// if you have a better idea on how to mock out this recursive function, plz patch it
var counter = 0;
@@ -115,13 +118,14 @@ describe('uninstallPlatform', function() {
return obj;
});
- runs(function() {
- uninstallPromise(uninstall.uninstallPlatform('android', temp, dummyplugin, plugins_dir, {}));
- });
- waitsFor(function() { return done; }, 'promise never resolved', 500);
- runs(function() {
+ danglers.andReturn(['one', 'two']);
+
+ uninstall.uninstallPlatform('android', temp, dummyplugin, plugins_dir, {})
+ .then(function() {
expect(emit).toHaveBeenCalledWith('log', 'Uninstalling 2 dangling dependent plugins.');
- });
+ }, function(err) {
+ expect(err).toBeUndefined();
+ }).done(done);
});
it('should not uninstall any dependencies that are relied on by other plugins');
});
@@ -184,7 +188,7 @@ describe('uninstallPlugin', function() {
});
});
describe('with dependencies', function() {
- var parseET, emit;
+ var parseET, emit, findPlugins;
beforeEach(function() {
emit = spyOn(plugman, 'emit');
var counter = 0;
@@ -203,15 +207,17 @@ describe('uninstallPlugin', function() {
}
}
});
+
+ findPlugins = spyOn(plugins, 'findPlugins').andReturn([dummyplugin, 'somedependent']);
});
- it('should recurse if dependent plugins are detected', function() {
- runs(function() {
- uninstallPromise(uninstall.uninstallPlugin(dummyplugin, plugins_dir));
- });
- waitsFor(function() { return done; }, 'promise never resolved', 500);
- runs(function() {
- expect(uninstall_plugin).toHaveBeenCalledWith('somedependent', plugins_dir);
- });
+ it('should delete all dangling plugins', function(done) {
+ uninstall.uninstallPlugin(dummyplugin, plugins_dir)
+ .then(function() {
+ expect(rm).toHaveBeenCalledWith('-rf', path.join(plugins_dir, dummyplugin));
+ expect(rm).toHaveBeenCalledWith('-rf', path.join(plugins_dir, 'somedependent'));
+ }, function(err) {
+ expect(err).toBeUndefined();
+ }).done(done);
});
});
});
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/0596e47f/src/uninstall.js
----------------------------------------------------------------------
diff --git a/src/uninstall.js b/src/uninstall.js
index 0fad5cc..4ef7501 100644
--- a/src/uninstall.js
+++ b/src/uninstall.js
@@ -9,13 +9,14 @@ var path = require('path'),
dependencies = require('./util/dependencies'),
underscore = require('underscore'),
Q = require('q'),
+ plugins = require('./util/plugins'),
platform_modules = require('./platforms');
// possible options: cli_variables, www_dir
// Returns a promise.
module.exports = function(platform, project_dir, id, plugins_dir, options) {
return module.exports.uninstallPlatform(platform, project_dir, id, plugins_dir, options)
- .then(function() {
+ .then(function(uninstalled) {
return module.exports.uninstallPlugin(id, plugins_dir);
});
}
@@ -49,73 +50,90 @@ module.exports.uninstallPlugin = function(id, plugins_dir) {
, plugin_et = xml_helpers.parseElementtreeSync(xml_path);
require('../plugman').emit('log', 'Deleting plugin ' + id);
- // Check for dependents
- var dependencies = plugin_et.findall('dependency');
- if (dependencies && dependencies.length) {
- require('../plugman').emit('verbose', 'Dependencies detected, iterating through them and removing them first.');
- return dependencies.reduce(function(soFar, dep) {
- return soFar.then(function() {
- return module.exports.uninstallPlugin(dep.attrib.id, plugins_dir);
- });
- }, Q())
- .then(function() {
- shell.rm('-rf', plugin_dir);
- require('../plugman').emit('verbose', id + ' deleted.');
+
+ var doDelete = function(id) {
+ var plugin_dir = path.join(plugins_dir, id);
+ if (!fs.existsSync(plugin_dir)) return;
+ shell.rm('-rf', plugin_dir);
+ require('../plugman').emit('verbose', id + ' deleted.');
+ };
+
+ // We've now lost the metadata for the plugins that have been uninstalled, so we can't use that info.
+ // Instead, we list all dependencies of the target plugin, and check the remaining metadata to see if
+ // anything depends on them, or if they're listed as top-level.
+ // If neither, they can be deleted.
+ var toDelete = plugin_et.findall('dependency');
+ toDelete = toDelete && toDelete.length ? toDelete.map(function(p) { return p.attrib.id; }) : [];
+ toDelete.push(id);
+
+ // Okay, now we check if any of these are depended on, or top-level.
+ // Find the installed platforms by whether they have a metadata file.
+ var platforms = Object.keys(platform_modules).filter(function(plat) {
+ return fs.existsSync(path.join(plugins_dir, plat + '.json'));
+ });
+
+ var found = [];
+ platforms.forEach(function(plat) {
+ var tlps = dependencies.generate_dependency_info(plugins_dir, plat).top_level_plugins;
+ toDelete.forEach(function(plugin) {
+ if (tlps.indexOf(plugin) >= 0 || dependencies.dependents(plugin, plugins_dir, plat).length) {
+ found.push(plugin);
+ }
});
+ });
+
+ var danglers = underscore.difference(plugins.findPlugins(plugins_dir), found);
+ if (danglers && danglers.length) {
+ require('../plugman').emit('log', 'Found ' + danglers.length + ' removable plugins. Deleting them.');
+ danglers.forEach(doDelete);
} else {
- // axe the directory
- shell.rm('-rf', plugin_dir);
- require('../plugman').emit('verbose', 'Deleted "' + plugin_dir + '".');
- return Q();
+ require('../plugman').emit('log', 'No dangling plugins to remove.');
}
+
+ return Q();
};
// possible options: cli_variables, www_dir, is_top_level
-// Returns a promise.
+// Returns a promise
function runUninstall(actions, platform, project_dir, plugin_dir, plugins_dir, options) {
var xml_path = path.join(plugin_dir, 'plugin.xml')
, plugin_et = xml_helpers.parseElementtreeSync(xml_path);
var plugin_id = plugin_et._root.attrib['id'];
options = options || {};
+ // Check that this plugin has no dependents.
+ var dependents = dependencies.dependents(plugin_id, plugins_dir, platform);
+ if(options.is_top_level && dependents && dependents.length > 0) {
+ require('../plugman').emit('verbose', 'Other top-level plugins (' + dependents.join(', ') + ') depend on ' + plugin_id + ', skipping uninstallation.');
+ return Q();
+ }
+
+ // Check how many dangling dependencies this plugin has.
var dependency_info = dependencies.generate_dependency_info(plugins_dir, platform);
- var graph = dependency_info.graph;
- var dependents = graph.getChain(plugin_id);
-
- var tlps = dependency_info.top_level_plugins;
- var diff_arr = [];
- tlps.forEach(function(tlp) {
- if (tlp != plugin_id) {
- var ds = graph.getChain(tlp);
- if (options.is_top_level && ds.indexOf(plugin_id) > -1) {
- throw new Error('Another top-level plugin (' + tlp + ') relies on plugin ' + plugin_id + ', therefore aborting uninstallation.');
- }
- diff_arr.push(ds);
- }
- });
+ var deps = dependency_info.graph.getChain(plugin_id);
+ var danglers = dependencies.danglers(plugin_id, plugins_dir, platform);
- // if this plugin has dependencies, do a set difference to determine which dependencies are not required by other existing plugins
- diff_arr.unshift(dependents);
- var danglers = underscore.difference.apply(null, diff_arr);
- if (dependents.length && danglers && danglers.length) {
+ var promise;
+ if (deps && deps.length && danglers && danglers.length) {
require('../plugman').emit('log', 'Uninstalling ' + danglers.length + ' dangling dependent plugins.');
- return Q.all(
+ promise = Q.all(
danglers.map(function(dangler) {
var dependent_path = path.join(plugins_dir, dangler);
var opts = {
www_dir: options.www_dir,
cli_variables: options.cli_variables,
- is_top_level: tlps.indexOf(dangler) > -1
+ is_top_level: dependency_info.top_level_plugins.indexOf(dangler) > -1
};
return runUninstall(actions, platform, project_dir, dependent_path, plugins_dir, opts);
})
- ).then(function() {
- return handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, options.www_dir, plugins_dir, plugin_dir, options.is_top_level);
- });
+ );
} else {
- // this plugin can get axed by itself, gogo!
- return handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, options.www_dir, plugins_dir, plugin_dir, options.is_top_level);
+ promise = Q();
}
+
+ return promise.then(function() {
+ return handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, options.www_dir, plugins_dir, plugin_dir, options.is_top_level);
+ });
}
// Returns a promise.
@@ -169,3 +187,4 @@ function handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, w
require('./../plugman').prepare(project_dir, platform, plugins_dir);
});
}
+
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/0596e47f/src/util/dependencies.js
----------------------------------------------------------------------
diff --git a/src/util/dependencies.js b/src/util/dependencies.js
index 32e07eb..09a4ea5 100644
--- a/src/util/dependencies.js
+++ b/src/util/dependencies.js
@@ -1,6 +1,7 @@
var dep_graph = require('dep-graph'),
path = require('path'),
config_changes = require('./config-changes'),
+ underscore = require('underscore'),
xml_helpers = require('./xml-helpers');
module.exports = {
@@ -25,9 +26,47 @@ module.exports = {
graph.add(plug, id);
});
});
+
return {
graph:graph,
top_level_plugins:tlps
};
+ },
+
+ // Returns a list of top-level plugins which are (transitively) dependent on the given plugin.
+ dependents: function(plugin_id, plugins_dir, platform) {
+ var dependency_info = module.exports.generate_dependency_info(plugins_dir, platform);
+ var graph = dependency_info.graph;
+
+ var tlps = dependency_info.top_level_plugins;
+ var dependents = tlps.filter(function(tlp) {
+ return tlp != plugin_id && graph.getChain(tlp).indexOf(plugin_id) >= 0;
+ });
+
+ return dependents;
+ },
+
+ // Returns a list of plugins which the given plugin depends on, for which it is the only dependent.
+ // In other words, if the given plugin were deleted, these dangling dependencies should be deleted too.
+ danglers: function(plugin_id, plugins_dir, platform) {
+ var dependency_info = module.exports.generate_dependency_info(plugins_dir, platform);
+ var graph = dependency_info.graph;
+ var dependencies = graph.getChain(plugin_id);
+
+ var tlps = dependency_info.top_level_plugins;
+ var diff_arr = [];
+ tlps.forEach(function(tlp) {
+ if (tlp != plugin_id) {
+ diff_arr.push(graph.getChain(tlp));
+ }
+ });
+
+ // if this plugin has dependencies, do a set difference to determine which dependencies are not required by other existing plugins
+ diff_arr.unshift(dependencies);
+ var danglers = underscore.difference.apply(null, diff_arr);
+
+ // Ensure no top-level plugins are tagged as danglers.
+ danglers = danglers && danglers.filter(function(x) { return tlps.indexOf(x) < 0; });
+ return danglers;
}
};
http://git-wip-us.apache.org/repos/asf/cordova-plugman/blob/0596e47f/src/util/plugins.js
----------------------------------------------------------------------
diff --git a/src/util/plugins.js b/src/util/plugins.js
index 07c9994..9b5861d 100644
--- a/src/util/plugins.js
+++ b/src/util/plugins.js
@@ -82,6 +82,21 @@ module.exports = {
require('../../plugman').emit('verbose', 'Plugin "' + plugin_id + '" fetched.');
return plugin_dir;
});
+ },
+
+ // List the directories in the path, ignoring any files, .svn, etc.
+ findPlugins:function(plugins_dir) {
+ var plugins = [],
+ stats;
+
+ if (fs.existsSync(plugins_dir)) {
+ plugins = fs.readdirSync(plugins_dir).filter(function (fileName) {
+ stats = fs.statSync(path.join(plugins_dir, fileName));
+ return fileName != '.svn' && fileName != 'CVS' && stats.isDirectory();
+ });
+ }
+
+ return plugins;
}
};