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 Mon, 12 May 2014 15:21:56 GMT
(poke, due to email)


On Wed, May 7, 2014 at 2:45 PM, Michal Mocny <mmocny@chromium.org> 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 <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