cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Mocny <mmo...@chromium.org>
Subject Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Date Wed, 07 May 2014 18:03:29 GMT
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.ercan@gmail.com>wrote:

> Any updates for me?
> --
> Gorkem
>
>
> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mmocny@chromium.org>
> wrote:
>
> > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan <gorkem.ercan@gmail.com
> > >wrote:
> >
> > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <mmocny@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 <purplecabbage@gmail.com>
> > wrote:
> > > >
> > > > > Yeah, that looks weird.
> > > > >
> > > > > @purplecabbage
> > > > > risingj.com
> > > > >
> > > > >
> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <git@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 infrastructure@apache.org or file
a
> JIRA
> > > > > ticket
> > > > > > with INFRA.
> > > > > > ---
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message