From 0763f9bca0b01f005220d609a2fee94438dcdf2e Mon Sep 17 00:00:00 2001 From: Georgi Alexandrov Date: Thu, 4 Jun 2015 17:30:35 +0300 Subject: [PATCH 1/2] Enable Plugman to run hooks defined in plugin.xml --- cordova-lib/spec-cordova/HooksRunner.spec.js | 7 ---- cordova-lib/spec-plugman/install.spec.js | 29 +++++++++++-- .../org.test.plugin-with-hooks/plugin.xml | 16 ++++++++ .../scripts/afterInstall.js | 2 + .../scripts/beforeInstall.js | 2 + .../scripts/beforeUninstall.js | 2 + cordova-lib/spec-plugman/uninstall.spec.js | 29 ++++++++++--- cordova-lib/src/hooks/HooksRunner.js | 41 +++++++++++++++---- cordova-lib/src/hooks/scriptsFinder.js | 5 +++ cordova-lib/src/plugman/install.js | 9 ++-- cordova-lib/src/plugman/plugman.js | 5 ++- cordova-lib/src/plugman/uninstall.js | 10 ++--- 12 files changed, 122 insertions(+), 35 deletions(-) create mode 100644 cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/plugin.xml create mode 100644 cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/afterInstall.js create mode 100644 cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeInstall.js create mode 100644 cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeUninstall.js diff --git a/cordova-lib/spec-cordova/HooksRunner.spec.js b/cordova-lib/spec-cordova/HooksRunner.spec.js index 14508ea11..80cc5d8e7 100644 --- a/cordova-lib/spec-cordova/HooksRunner.spec.js +++ b/cordova-lib/spec-cordova/HooksRunner.spec.js @@ -81,12 +81,6 @@ describe('HooksRunner', function() { process.chdir(path.join(__dirname, '..')); // Non e2e tests assume CWD is repo root. }); - it('should throw if provided directory is not a cordova project', function() { - expect(function() { - new HooksRunner(tmpDir); - }).toThrow(); - }); - it('should not throw if provided directory is a cordova project', function() { expect(function () { new HooksRunner(project); @@ -116,7 +110,6 @@ describe('HooksRunner', function() { return Q(); }); - // Add the testing platform. cordova.raw.platform('add', [helpers.testPlatform]).fail(function (err) { expect(err).toBeUndefined(); diff --git a/cordova-lib/spec-plugman/install.spec.js b/cordova-lib/spec-plugman/install.spec.js index c50e907e7..402133e42 100644 --- a/cordova-lib/spec-plugman/install.spec.js +++ b/cordova-lib/spec-plugman/install.spec.js @@ -25,6 +25,7 @@ var install = require('../src/plugman/install'), events = require('../src/events'), plugman = require('../src/plugman/plugman'), platforms = require('../src/plugman/platforms/common'), + HooksRunner = require('../src/hooks/HooksRunner'), common = require('./common'), fs = require('fs'), os = require('os'), @@ -47,6 +48,7 @@ var install = require('../src/plugman/install'), 'org.test.plugins.childbrowser' : path.join(plugins_dir, 'org.test.plugins.childbrowser'), 'com.adobe.vars' : path.join(plugins_dir, 'com.adobe.vars'), 'org.test.defaultvariables' : path.join(plugins_dir, 'org.test.defaultvariables'), + 'org.test.plugin-with-hooks' : path.join(plugins_dir, 'org.test.plugin-with-hooks'), 'A' : path.join(plugins_dir, 'dependencies', 'A'), 'B' : path.join(plugins_dir, 'dependencies', 'B'), 'C' : path.join(plugins_dir, 'dependencies', 'C'), @@ -154,7 +156,7 @@ describe('start', function() { }); describe('install', function() { - var chmod, exec, add_to_queue, prepare, cp, rm, fetchSpy; + var chmod, exec, add_to_queue, prepare, cp, rm, fetchSpy, fire; var spawnSpy; beforeEach(function() { @@ -173,11 +175,32 @@ describe('install', function() { spyOn(fs, 'writeFileSync').andReturn(true); cp = spyOn(shell, 'cp').andReturn(true); rm = spyOn(shell, 'rm').andReturn(true); - add_to_queue = spyOn(PlatformJson.prototype, 'addInstalledPluginToPrepareQueue'); - done = false; + add_to_queue = spyOn(PlatformJson.prototype, 'addInstalledPluginToPrepareQueue'); + fire = spyOn(HooksRunner.prototype, 'fire').andReturn(Q()); + done = false; }); describe('success', function() { + it('should fire hooks on plugin successful install', function() { + runs(function() { + installPromise( install('android', project, plugins['org.test.plugin-with-hooks']) ); + }); + waitsFor(function() { return done; }, 'install promise never resolved', 200); + runs(function() { + expect(fire).toHaveBeenCalled(); + }); + }); + + it('should not fire hooks on plugin successful install when run_hooks=false', function() { + runs(function() { + installPromise( install('android', project, plugins['org.test.plugin-with-hooks'], {run_hooks:false}) ); + }); + waitsFor(function() { return done; }, 'install promise never resolved', 200); + runs(function() { + expect(fire).not.toHaveBeenCalled(); + }); + }); + it('should call prepare after a successful install', function() { expect(results['prepareCount']).toBe(5); }); diff --git a/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/plugin.xml b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/plugin.xml new file mode 100644 index 000000000..e456df79e --- /dev/null +++ b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/plugin.xml @@ -0,0 +1,16 @@ + + + + Plugin With Hooks + Commercial + cordova,device + + + + + + + + diff --git a/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/afterInstall.js b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/afterInstall.js new file mode 100644 index 000000000..23b1a8410 --- /dev/null +++ b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/afterInstall.js @@ -0,0 +1,2 @@ +module.exports = function(context) { +} \ No newline at end of file diff --git a/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeInstall.js b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeInstall.js new file mode 100644 index 000000000..23b1a8410 --- /dev/null +++ b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeInstall.js @@ -0,0 +1,2 @@ +module.exports = function(context) { +} \ No newline at end of file diff --git a/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeUninstall.js b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeUninstall.js new file mode 100644 index 000000000..23b1a8410 --- /dev/null +++ b/cordova-lib/spec-plugman/plugins/org.test.plugin-with-hooks/scripts/beforeUninstall.js @@ -0,0 +1,2 @@ +module.exports = function(context) { +} \ No newline at end of file diff --git a/cordova-lib/spec-plugman/uninstall.spec.js b/cordova-lib/spec-plugman/uninstall.spec.js index 552ee0065..3e65c5e9e 100644 --- a/cordova-lib/spec-plugman/uninstall.spec.js +++ b/cordova-lib/spec-plugman/uninstall.spec.js @@ -24,7 +24,8 @@ var uninstall = require('../src/plugman/uninstall'), actions = require('../src/plugman/util/action-stack'), PlatformJson = require('../src/plugman/util/PlatformJson'), events = require('../src/events'), - plugman = require('../src/plugman/plugman'), + plugman = require('../src/plugman/plugman'), + HooksRunner = require('../src/hooks/HooksRunner'), common = require('./common'), fs = require('fs'), path = require('path'), @@ -41,7 +42,8 @@ var uninstall = require('../src/plugman/uninstall'), plugins_install_dir2 = path.join(project2, 'cordova', 'plugins'), plugins = { - 'org.test.plugins.dummyplugin' : path.join(plugins_dir, 'org.test.plugins.dummyplugin'), + 'org.test.plugins.dummyplugin' : path.join(plugins_dir, 'org.test.plugins.dummyplugin'), + 'org.test.plugin-with-hooks' : path.join(plugins_dir, 'org.test.plugin-with-hooks'), 'A' : path.join(plugins_dir, 'dependencies', 'A'), 'C' : path.join(plugins_dir, 'dependencies', 'C') }, @@ -64,6 +66,8 @@ describe('start', function() { promise = Q() .then(function(){ return install('android', project, plugins['org.test.plugins.dummyplugin']); + }).then(function(){ + return install('android', project, plugins['org.test.plugin-with-hooks']); }).then(function(){ return install('android', project, plugins['A']); }).then( function(){ @@ -97,7 +101,7 @@ describe('uninstallPlatform', function() { add_to_queue = spyOn(PlatformJson.prototype, 'addUninstalledPluginToPrepareQueue'); done = false; }); - describe('success', function() { + describe('success', function() { it('should call prepare after a successful uninstall', function() { runs(function() { uninstallPromise(uninstall.uninstallPlatform('android', project, dummy_id)); @@ -177,7 +181,7 @@ describe('uninstallPlugin', function() { done = false; }); describe('with dependencies', function() { - + it('should delete all dependent plugins', function() { runs(function() { uninstallPromise( uninstall.uninstallPlugin('A', plugins_install_dir) ); @@ -234,15 +238,26 @@ describe('uninstallPlugin', function() { }); describe('uninstall', function() { - var fsWrite, rm, add_to_queue; + var fsWrite, rm, add_to_queue, fire; beforeEach(function() { fsWrite = spyOn(fs, 'writeFileSync').andReturn(true); rm = spyOn(shell, 'rm').andReturn(true); add_to_queue = spyOn(PlatformJson.prototype, 'addUninstalledPluginToPrepareQueue'); + fire = spyOn(HooksRunner.prototype, 'fire').andReturn(Q()); done = false; }); describe('success', function() { + it('should fire hooks on plugin successful uninstalled', function() { + runs(function() { + uninstallPromise( uninstall('android', project, plugins['org.test.plugin-with-hooks']) ); + }); + waitsFor(function() { return done; }, 'promise never resolved', 200); + runs(function() { + expect(fire).toHaveBeenCalled(); + }); + }); + it('should call the config-changes module\'s add_uninstalled_plugin_to_prepare_queue method after processing an install', function() { runs(function() { uninstallPromise( uninstall('android', project, plugins['org.test.plugins.dummyplugin']) ); @@ -285,6 +300,10 @@ describe('end', function() { function(){ return uninstall('android', project, plugins['org.test.plugins.dummyplugin']); } + ).then( + function(){ + return uninstall('android', project, plugins['org.test.plugin-with-hooks']); + } ).then( function(){ // Fails... A depends on diff --git a/cordova-lib/src/hooks/HooksRunner.js b/cordova-lib/src/hooks/HooksRunner.js index 42129d86f..66a905442 100644 --- a/cordova-lib/src/hooks/HooksRunner.js +++ b/cordova-lib/src/hooks/HooksRunner.js @@ -33,17 +33,21 @@ var isWindows = os.platform().slice(0, 3) === 'win'; * Tries to create a HooksRunner for passed project root. * @constructor */ -function HooksRunner(projectRoot) { - var root = cordovaUtil.isCordova(projectRoot); - if (!root) throw new CordovaError('Not a Cordova project ("' + projectRoot + '"), can\'t use hooks.'); - else this.projectRoot = root; +function DefaultHooksRunner(projectRoot) { + this.projectRoot = projectRoot; } +DefaultHooksRunner.prototype.prepareOptions = function(opts) { + opts = opts || {}; + opts.projectRoot = this.projectRoot; + return opts; +}; + /** * Fires all event handlers and scripts for a passed hook type. * Returns a promise. */ -HooksRunner.prototype.fire = function fire(hook, opts) { +DefaultHooksRunner.prototype.fire = function fire(hook, opts) { // args check if (!hook) { throw new Error('hook type is not specified'); @@ -59,12 +63,24 @@ HooksRunner.prototype.fire = function fire(hook, opts) { }); }; +/** + * Tries to create a CordovaHooksRunner for passed project root. + * @constructor + */ +function CordovaHooksRunner(projectRoot) { + DefaultHooksRunner.call(this, projectRoot); + var root = cordovaUtil.isCordova(projectRoot); + if (!root) throw new CordovaError('Not a Cordova project ("' + projectRoot + '"), can\'t use hooks.'); + else this.projectRoot = root; +} + +require('util').inherits(CordovaHooksRunner, DefaultHooksRunner); + /** * Refines passed options so that all required parameters are set. */ -HooksRunner.prototype.prepareOptions = function(opts) { - opts = opts || {}; - opts.projectRoot = this.projectRoot; +CordovaHooksRunner.prototype.prepareOptions = function(opts) { + opts = DefaultHooksRunner.prototype.prepareOptions.call(this, opts); opts.cordova = opts.cordova || {}; opts.cordova.platforms = opts.cordova.platforms || opts.platforms || cordovaUtil.listPlatforms(opts.projectRoot); opts.cordova.platforms = opts.cordova.platforms.map(function(platform) { return platform.split('@')[0]; } ); @@ -80,6 +96,15 @@ HooksRunner.prototype.prepareOptions = function(opts) { return opts; }; +function HooksRunner(projectRoot) { + var root = cordovaUtil.isCordova(projectRoot); + this.hooksRunner = root ? new CordovaHooksRunner(root) : new DefaultHooksRunner(projectRoot); +} + +HooksRunner.prototype.fire = function fire(hook, opts) { + return this.hooksRunner.fire(hook, opts); +}; + module.exports = HooksRunner; /** diff --git a/cordova-lib/src/hooks/scriptsFinder.js b/cordova-lib/src/hooks/scriptsFinder.js index edaf64c95..5da569788 100644 --- a/cordova-lib/src/hooks/scriptsFinder.js +++ b/cordova-lib/src/hooks/scriptsFinder.js @@ -105,6 +105,11 @@ function getApplicationHookScriptsFromDir(dir) { */ function getScriptsFromConfigXml(hook, opts) { var configPath = cordovaUtil.projectConfig(opts.projectRoot); + + if (!(fs.existsSync(configPath))) { + return []; + } + var configXml = new ConfigParser(configPath); return configXml.getHookScripts(hook, opts.cordova.platforms).map(function(scriptElement) { diff --git a/cordova-lib/src/plugman/install.js b/cordova-lib/src/plugman/install.js index d8c199730..d08b24197 100644 --- a/cordova-lib/src/plugman/install.js +++ b/cordova-lib/src/plugman/install.js @@ -336,16 +336,15 @@ function runInstall(actions, platform, project_dir, plugin_dir, plugins_dir, opt } ).then( function(){ - var install_plugin_dir = path.join(plugins_dir, pluginInfo.id); + var install_plugin_dir = path.join(plugins_dir, pluginInfo.id), + run_hooks = options.hasOwnProperty('run_hooks') ? options.run_hooks : true; // may need to copy to destination... if ( !fs.existsSync(install_plugin_dir) ) { copyPlugin(plugin_dir, plugins_dir, options.link, pluginInfoProvider); } - var projectRoot = cordovaUtil.isCordova(); - - if(projectRoot) { + if(run_hooks) { // using unified hooksRunner var hookOptions = { cordova: { platforms: [ platform ] }, @@ -357,7 +356,7 @@ function runInstall(actions, platform, project_dir, plugin_dir, plugins_dir, opt } }; - var hooksRunner = new HooksRunner(projectRoot); + var hooksRunner = new HooksRunner(cordovaUtil.isCordova() || project_dir); return hooksRunner.fire('before_plugin_install', hookOptions).then(function() { return handleInstall(actions, pluginInfo, platform, project_dir, plugins_dir, install_plugin_dir, filtered_variables, options); diff --git a/cordova-lib/src/plugman/plugman.js b/cordova-lib/src/plugman/plugman.js index 0a8cd8d19..fdab175a6 100644 --- a/cordova-lib/src/plugman/plugman.js +++ b/cordova-lib/src/plugman/plugman.js @@ -106,7 +106,8 @@ plugman.commands = { cli_variables: cli_variables, www_dir: cli_opts.www, searchpath: cli_opts.searchpath, - link: cli_opts.link + link: cli_opts.link, + run_hooks: !cli_opts.nohooks }; var p = Q(); @@ -130,7 +131,7 @@ plugman.commands = { var p = Q(); cli_opts.plugin.forEach(function (pluginSrc) { p = p.then(function () { - return plugman.raw.uninstall(cli_opts.platform, cli_opts.project, pluginSrc, cli_opts.plugins_dir, { www_dir: cli_opts.www }); + return plugman.raw.uninstall(cli_opts.platform, cli_opts.project, pluginSrc, cli_opts.plugins_dir, { www_dir: cli_opts.www, run_hooks: !cli_opts.nohooks }); }); }); diff --git a/cordova-lib/src/plugman/uninstall.js b/cordova-lib/src/plugman/uninstall.js index 9fdc2e4e6..f75ca6033 100644 --- a/cordova-lib/src/plugman/uninstall.js +++ b/cordova-lib/src/plugman/uninstall.js @@ -220,7 +220,9 @@ module.exports.uninstallPlugin = function(id, plugins_dir, options) { // possible options: cli_variables, www_dir, is_top_level // Returns a promise function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugins_dir, options) { - var pluginInfoProvider = options.pluginInfoProvider; + var pluginInfoProvider = options.pluginInfoProvider, + run_hooks = options.hasOwnProperty('run_hooks') ? options.run_hooks : true; + // If this plugin is not really installed, return (CB-7004). if (!fs.existsSync(plugin_dir)) { return Q(); @@ -268,9 +270,7 @@ function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugin promise = Q(); } - var projectRoot = cordovaUtil.isCordova(); - - if(projectRoot) { + if(run_hooks) { // using unified hooksRunner var hooksRunnerOptions = { @@ -283,7 +283,7 @@ function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugin } }; - var hooksRunner = new HooksRunner(projectRoot); + var hooksRunner = new HooksRunner(cordovaUtil.isCordova() || project_dir); return promise.then(function() { return hooksRunner.fire('before_plugin_uninstall', hooksRunnerOptions); From 3ba3c3285010955e980dd452db9d03ebd4fd8772 Mon Sep 17 00:00:00 2001 From: Georgi Alexandrov Date: Wed, 2 Sep 2015 14:55:28 +0300 Subject: [PATCH 2/2] Address some comments --- cordova-lib/src/hooks/HooksRunner.js | 13 ++++++++++--- cordova-lib/src/hooks/scriptsFinder.js | 1 + 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/cordova-lib/src/hooks/HooksRunner.js b/cordova-lib/src/hooks/HooksRunner.js index 66a905442..e0e611679 100644 --- a/cordova-lib/src/hooks/HooksRunner.js +++ b/cordova-lib/src/hooks/HooksRunner.js @@ -70,8 +70,12 @@ DefaultHooksRunner.prototype.fire = function fire(hook, opts) { function CordovaHooksRunner(projectRoot) { DefaultHooksRunner.call(this, projectRoot); var root = cordovaUtil.isCordova(projectRoot); - if (!root) throw new CordovaError('Not a Cordova project ("' + projectRoot + '"), can\'t use hooks.'); - else this.projectRoot = root; + if (!root) { + throw new CordovaError('Not a Cordova project ("' + projectRoot + '"), can\'t use hooks.'); + } + else { + this.projectRoot = root; + } } require('util').inherits(CordovaHooksRunner, DefaultHooksRunner); @@ -95,7 +99,10 @@ CordovaHooksRunner.prototype.prepareOptions = function(opts) { return opts; }; - +/** + * Construct the appropriate HooksRunner for the specified project. + * The project can be multi-platform project created with Cordova CLI or can be platform specific created using one of the cordova-{platform} scripts. + */ function HooksRunner(projectRoot) { var root = cordovaUtil.isCordova(projectRoot); this.hooksRunner = root ? new CordovaHooksRunner(root) : new DefaultHooksRunner(projectRoot); diff --git a/cordova-lib/src/hooks/scriptsFinder.js b/cordova-lib/src/hooks/scriptsFinder.js index 5da569788..1df765619 100644 --- a/cordova-lib/src/hooks/scriptsFinder.js +++ b/cordova-lib/src/hooks/scriptsFinder.js @@ -106,6 +106,7 @@ function getApplicationHookScriptsFromDir(dir) { function getScriptsFromConfigXml(hook, opts) { var configPath = cordovaUtil.projectConfig(opts.projectRoot); + // When plugman is used the config xml in project root folder will not exist if (!(fs.existsSync(configPath))) { return []; }