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:45:41 GMT
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 <mmocny@chromium.org> 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 <mmocny@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.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