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.
>>> > > > > > ---
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>
