..also, seems its not warning when I attempt to remove a previously saved plugin. Not sure if that is a bug I introduced in my manual patch or if it existed before.
-Michal On Wed, May 7, 2014 at 2:03 PM, Michal Mocny <mmo...@chromium.org> wrote: > So, this patch needs updating now that cordova-lib was split out. I've > manually applied the patch locally to test this feature (pasted below, so > you don't have to redo the work), but please update the exiting PR to only > contain changes to cli.js and submit a new PR to the cordova-lib repo ( > https://github.com/apache/cordova-lib) with what I have below + whatever > changes you want. I didn't move your spec tests so those need re-adding. > > Comments: > - I like that you can "cordova restore plugins" when platforms/ and > plugins/ folders don't exist (so its useful from a fresh git clone of a > cordova app). > - Your save/restore feature is saving/restoring the full list of currently > installed plugins and the specific versions they are at currently -- it is > not storing the list of "cordova plugin add FOO" the user actually typed. > I think these are two very different things, akin perhaps to npm > shrinkwrap. > - For this reason, I think we should land this very useful feature, but > leave it experimental for a while, until we debate exactly what we want > long term. > > I will add an --experimental flag to CLI, and then we can hide features > like this behind that flag for now. Then at least it isn't stuck in a PR. > > Okay? > > Patch to cordova-lib: > ======== > > diff --git a/cordova-lib/src/cordova/ConfigParser.js > b/cordova-lib/src/cordova/ConfigParser.js > index 262e75f..4cea08a 100644 > --- a/cordova-lib/src/cordova/ConfigParser.js > +++ b/cordova-lib/src/cordova/ConfigParser.js > @@ -155,6 +155,22 @@ ConfigParser.prototype = { > > return ret; > }, > + /** > + *This does not check for duplicate feature entries > + */ > + addFeature: function (name, params){ > + var el = new et.Element('feature'); > + el.attrib.name = name; > + if(params){ > + params.forEach(function(param){ > + var p = new et.Element('param'); > + p.attrib.name = param.name; > + p.attrib.value = param.value; > + el.append(p); > + }); > + } > + this.doc.getroot().append(el); > + }, > write:function() { > fs.writeFileSync(this.path, this.doc.write({indent: 4}), 'utf-8'); > } > diff --git a/cordova-lib/src/cordova/cordova.js > b/cordova-lib/src/cordova/cordova.js > index abd6709..6fcb05e 100644 > --- a/cordova-lib/src/cordova/cordova.js > +++ b/cordova-lib/src/cordova/cordova.js > @@ -64,5 +64,7 @@ addModuleProperty(module, 'platforms', './platform', > true); > addModuleProperty(module, 'compile', './compile', true); > addModuleProperty(module, 'run', './run', true); > addModuleProperty(module, 'info', './info', true); > +addModuleProperty(module, 'save', './save', true); > +addModuleProperty(module, 'restore', './restore', true); > > > diff --git a/cordova-lib/src/cordova/plugin.js > b/cordova-lib/src/cordova/plugin.js > index d488e1e..11f4686 100644 > --- a/cordova-lib/src/cordova/plugin.js > +++ b/cordova-lib/src/cordova/plugin.js > @@ -27,6 +27,8 @@ var cordova_util = require('./util'), > Q = require('q'), > CordovaError = require('../CordovaError'), > PluginInfo = require('../PluginInfo'), > + ConfigParser = require('./ConfigParser'), > + fs = require('fs'), > events = require('./events'); > > // Returns a promise. > @@ -173,6 +175,15 @@ module.exports = function plugin(command, targets, > opts) { > var platformRoot = path.join(projectRoot, > 'platforms', platform); > var platforms = require('./platforms'); > var parser = new > platforms[platform].parser(platformRoot); > + //check if plugin is restorable and warn > + var configPath = > cordova_util.projectConfig(projectRoot); > + if(fs.existsSync(configPath)){//should not > happen with real life but needed for tests > + var configXml = new > ConfigParser(configPath); > + var features = > configXml.doc.findall('./feature/param[@name="id"][@value="'+target+'"]/..'); > + if(features && features.length){ > + events.emit('results','"'+target + '" > plugin is restorable, call "cordova save plugins" to remove it from > restorable plugins list'); > + } > + } > events.emit('verbose', 'Calling > plugman.uninstall on plugin "' + target + '" for platform "' + platform + > '"'); > return > plugman.raw.uninstall.uninstallPlatform(platform, platformRoot, target, > path.join(projectRoot, 'plugins')); > }); > diff --git a/cordova-lib/src/cordova/restore.js > b/cordova-lib/src/cordova/restore.js > new file mode 100644 > index 0000000..6414e00 > --- /dev/null > +++ b/cordova-lib/src/cordova/restore.js > @@ -0,0 +1,76 @@ > +/** > + Licensed to the Apache Software Foundation (ASF) under one > + or more contributor license agreements. See the NOTICE file > + distributed with this work for additional information > + regarding copyright ownership. The ASF licenses this file > + to you under the Apache License, Version 2.0 (the > + "License"); you may not use this file except in compliance > + with the License. You may obtain a copy of the License at > + > + http://www.apache.org/licenses/LICENSE-2.0 > + > + Unless required by applicable law or agreed to in writing, > + software distributed under the License is distributed on an > + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY > + KIND, either express or implied. See the License for the > + specific language governing permissions and limitations > + under the License. > +*/ > + > +var cordova_util = require('./util'), > + ConfigParser = require('./ConfigParser'), > + path = require('path'), > + xml = require('../util/xml-helpers') > + Q = require('q'), > + fs = require('fs'), > + plugin = require('./plugin'), > + events = require('./events'); > + > +module.exports = function restore(target){ > + var projectHome = cordova_util.cdProjectRoot(); > + var configPath = cordova_util.projectConfig(projectHome); > + var configXml = new ConfigParser(configPath); > + return installPluginsFromConfigXML(configXml); > +} > + > + > +//returns a Promise > +function installPluginsFromConfigXML(cfg){ > + //Install plugins that are listed on config.xml > + var pluginsFromConfig = new Array(); > + var projectRoot = cordova_util.cdProjectRoot(); > + var plugins_dir = path.join(projectRoot, 'plugins'); > + > + var features = cfg.doc.findall('feature'); > + features.forEach(function(feature){ > + var params = feature.findall('param'); > + var pluginId = ""; > + var pluginVersion = ""; > + for( var i =0; i < params.length; i++){ > + if(params[i].attrib.name === 'id'){ > + pluginId = params[i].attrib.value; > + } > + if(params[i].attrib.name === 'version'){ > + pluginVersion = params[i].attrib.value; > + } > + } > + var pluginPath = path.join(plugins_dir,pluginId); > + // contents of the plugins folder takes precedence hence > + // we ignore if the correct version is installed or not. > + if(pluginId !== "" && !fs.existsSync(pluginPath)){ > + if( pluginVersion !== ""){ > + pluginId = pluginId +"@"+pluginVersion; > + } > + events.emit('log', "Discovered "+ pluginId + " in config.xml. > Installing to the project") > + pluginsFromConfig.push(pluginId); > + } > + > + }) > + > + //Use cli instead of plugman directly ensuring all the hooks > + // to get fired. > + if(pluginsFromConfig.length >0){ > + return plugin("add",pluginsFromConfig); > + } > + return Q.all("No config.xml plugins to install"); > +} > diff --git a/cordova-lib/src/cordova/save.js > b/cordova-lib/src/cordova/save.js > new file mode 100644 > index 0000000..b767caa > --- /dev/null > +++ b/cordova-lib/src/cordova/save.js > @@ -0,0 +1,79 @@ > +/** > + Licensed to the Apache Software Foundation (ASF) under one > + or more contributor license agreements. See the NOTICE file > + distributed with this work for additional information > + regarding copyright ownership. The ASF licenses this file > + to you under the Apache License, Version 2.0 (the > + "License"); you may not use this file except in compliance > + with the License. You may obtain a copy of the License at > + > + http://www.apache.org/licenses/LICENSE-2.0 > + > + Unless required by applicable law or agreed to in writing, > + software distributed under the License is distributed on an > + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY > + KIND, either express or implied. See the License for the > + specific language governing permissions and limitations > + under the License. > +*/ > + > +var cordova_util = require('./util'), > + ConfigParser = require('./ConfigParser'), > + path = require('path'), > + xml = require('../util/xml-helpers') > + Q = require('q'), > + events = require('./events'); > + > +module.exports = function save(target){ > + var projectHome = cordova_util.cdProjectRoot(); > + var configPath = cordova_util.projectConfig(projectHome); > + var configXml = new ConfigParser(configPath); > + var pluginsPath = path.join(projectHome, 'plugins'); > + var plugins = cordova_util.findPlugins(pluginsPath); > + var features = configXml.doc.findall('./feature/param[@name="id"]/..'); > + //clear obsolete features with id params. > + for(var i=0; i<features.length; i++){ > + //somehow elementtree remove fails on me > + var childs = configXml.doc.getroot().getchildren(); > + var idx = childs.indexOf(features[i]); > + if(idx > -1){ > + childs.splice(idx,1); > + } > + } > + // persist the removed features here if there are no plugins > + // to be added to config.xml otherwise we can delay the > + // persist to add feature > + if((!plugins || plugins.length<1) && > + (features && features.length)){ > + configXml.write(); > + } > + > + return Q.all(plugins.map(function(plugin){ > + var currentPluginPath = path.join(pluginsPath,plugin); > + var name = readPluginName(currentPluginPath); > + var id = plugin; > + var version = readPluginVersion(currentPluginPath); > + var params = [{name:"id", value:id},{name:"version", value: version}]; > + configXml.addFeature(name,params); > + configXml.write(); > + events.emit('results', 'Saved plugin info for "'+plugin+'" to > config.xml'); > + return Q(); > + })); > +} > + > +function readPluginName(pluginPath){ > + var xml_path = path.join(pluginPath, 'plugin.xml'); > + var et = xml.parseElementtreeSync(xml_path); > + var el = et.getroot().find('name'); > + if(el && el.text){ > + return el.text.trim(); > + } > + return ""; > +} > + > +function readPluginVersion(pluginPath){ > + var xml_path = path.join(pluginPath, 'plugin.xml'); > + var et = xml.parseElementtreeSync(xml_path); > + var version = et.getroot().attrib.version; > + return version; > +} > > > > On Tue, May 6, 2014 at 11:20 AM, Gorkem Ercan <gorkem.er...@gmail.com>wrote: > >> Any updates for me? >> -- >> Gorkem >> >> >> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mmo...@chromium.org> >> wrote: >> >> > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan <gorkem.er...@gmail.com >> > >wrote: >> > >> > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <mmo...@chromium.org> >> > wrote: >> > > >> > > > Took a quick glance. General questions: >> > > > - why the need for save? Why not just alter the list on each >> cordova >> > > > plugin add/rm? >> > > > >> > > >> > > I do not want to force this workflow. Calling save does not bring much >> > > overhead, considering plugin list does not probably change constantly. >> > > >> > >> > If you are going to make this choice, then I would not like to >> > automatically install on prepare. There should be an explicit "load" >> > command. (those names are wrong, but you get the point). >> > >> > Either we automatically manage plugin installs, or explicitly manage >> them. >> > I'm happy to start with explicit and support opt-in to automatic >> handling >> > later once we have confidence in the feature. >> > >> > >> > > >> > > >> > > > - wouldn't "cordova plugin rm foo && cordova prepare" re-install >> that >> > > > plugin right now? >> > > > >> > > >> > > This workflow would require an explicit update to config.xml if a >> plugin >> > is >> > > persisted in there. This is a good point, I shall update plugins rm to >> > > print a warning about it. >> > >> > >> > > >> > > > - why the name <feature> and not <dependency> ? I think this >> > > functionality >> > > > should overlap with the plugin.xml spec. >> > > > >> > > > >> > > feature tag lives in the w3c widget spec which has advantages for >> those >> > > (like myself) who does provide tools for editing config,xml. >> > > >> > >> > We are no longer using that spec, and I as we move more functionality >> from >> > plugins.xml into config.xml we should strive to keep them in line. It >> also >> > makes our docs easier. >> > >> > >> > > >> > > >> > > >> > > > I haven't put the PR through testing yet. >> > > > >> > > > >> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <purplecabb...@gmail.com> >> > wrote: >> > > > >> > > > > Yeah, that looks weird. >> > > > > >> > > > > @purplecabbage >> > > > > risingj.com >> > > > > >> > > > > >> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <g...@git.apache.org> >> wrote: >> > > > > >> > > > > > Github user kamrik commented on a diff in the pull request: >> > > > > > >> > > > > > >> > > > https://github.com/apache/cordova-cli/pull/165#discussion_r11755812 >> > > > > > >> > > > > > --- Diff: src/save.js --- >> > > > > > @@ -0,0 +1,71 @@ >> > > > > > +/** >> > > > > > + Licensed to the Apache Software Foundation (ASF) under >> one >> > > > > > + or more contributor license agreements. See the NOTICE >> > file >> > > > > > + distributed with this work for additional information >> > > > > > + regarding copyright ownership. The ASF licenses this >> file >> > > > > > + to you under the Apache License, Version 2.0 (the >> > > > > > + "License"); you may not use this file except in >> compliance >> > > > > > + with the License. You may obtain a copy of the >> License at >> > > > > > + >> > > > > > + http://www.apache.org/licenses/LICENSE-2.0 >> > > > > > + >> > > > > > + Unless required by applicable law or agreed to in >> writing, >> > > > > > + software distributed under the License is distributed >> on >> > an >> > > > > > + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY >> > > > > > + KIND, either express or implied. See the License for >> the >> > > > > > + specific language governing permissions and limitations >> > > > > > + under the License. >> > > > > > +*/ >> > > > > > + >> > > > > > +var cordova_util = require('./util'), >> > > > > > + ConfigParser = require('./ConfigParser'), >> > > > > > + path = require('path'), >> > > > > > + xml =require('./xml-helpers') >> > > > > > + Q = require('q'), >> > > > > > + events = require('./events'); >> > > > > > + >> > > > > > +; >> > > > > > + >> > > > > > + >> > > > > > +module.exports = function save(target){ >> > > > > > + var projectHome = cordova_util.cdProjectRoot(); >> > > > > > + var configPath = cordova_util.projectConfig(projectHome); >> > > > > > + var configXml = new ConfigParser(configPath); >> > > > > > + var pluginsPath = path.join(projectHome, 'plugins'); >> > > > > > + var plugins = cordova_util.findPlugins(pluginsPath); >> > > > > > + >> > > > > > + return Q.all(plugins.map(function(plugin){ >> > > > > > + var currentPluginPath = path.join(pluginsPath,plugin); >> > > > > > + var name = readPluginName(currentPluginPath); >> > > > > > + var id = plugin; >> > > > > > + var version = readPluginVersion(currentPluginPath); >> > > > > > + var features = configXml.doc.findall('feature'); >> > > > > > + for(var i=0; i<features.length; i++){ >> > > > > > + if(features[i].attrib.name === name){ >> > > > > > + events.emit('results', 'An entry for "'+ plugin+ >> '" >> > > > > already >> > > > > > exists'); >> > > > > > + return Q(); >> > > > > > + } >> > > > > > + } >> > > > > > + configXml.addFeature(name, JSON.parse('[{"name":"id", >> > > > > > "value":"'+id+'"},{"name":"version", "value":"'+version+'"}]')); >> > > > > > --- End diff -- >> > > > > > >> > > > > > I might be missing something, but why JSON.parse() rather >> than >> > > just >> > > > > > literal array of objects? >> > > > > > >> > > > > > >> > > > > > --- >> > > > > > 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 infrastruct...@apache.org or file a >> JIRA >> > > > > ticket >> > > > > > with INFRA. >> > > > > > --- >> > > > > > >> > > > > >> > > > >> > > >> > >> > >
