(poke, due to email)
On Wed, May 7, 2014 at 2:45 PM, Michal Mocny <[email protected]> wrote: > Added experimental flag to CLI ("--experimental"). Please wrap your > handling of save & restore commands with "if (opts.experimental)" > > > On Wed, May 7, 2014 at 2:05 PM, Michal Mocny <[email protected]> wrote: > >> ..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 <[email protected]> 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 >>> <[email protected]>wrote: >>> >>>> Any updates for me? >>>> -- >>>> Gorkem >>>> >>>> >>>> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <[email protected]> >>>> wrote: >>>> >>>> > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan <[email protected] >>>> > >wrote: >>>> > >>>> > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <[email protected]> >>>> > 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 <[email protected]> >>>> > wrote: >>>> > > > >>>> > > > > Yeah, that looks weird. >>>> > > > > >>>> > > > > @purplecabbage >>>> > > > > risingj.com >>>> > > > > >>>> > > > > >>>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <[email protected]> >>>> 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 [email protected] or file >>>> a JIRA >>>> > > > > ticket >>>> > > > > > with INFRA. >>>> > > > > > --- >>>> > > > > > >>>> > > > > >>>> > > > >>>> > > >>>> > >>>> >>> >>> >> >
