You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by gorkem <gi...@git.apache.org> on 2014/04/17 22:53:04 UTC
[GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
GitHub user gorkem opened a pull request:
https://github.com/apache/cordova-cli/pull/165
CB-6469 - Restore plugins from config.xml
Adds a save command to save the state of the installed plugins. Changes prepare so that config.xml is read and any missing plugins are installed. Adds some tests for the new features
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gorkem/cordova-cli plugin-reprovision
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-cli/pull/165.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #165
----
commit e424693dfb556771e308a2129881fe1cedc1d247
Author: Gorkem Ercan <go...@gmail.com>
Date: 2014-04-17T00:33:22Z
Initial implementation for automatic plugin installation
Modifies prepare phase so that all features(plugins) that are in the config.xml are added as plugins to the project.
Also adds a new save command to CLI which persists the currently added features to config.xml. Also adds a
new function to ConfigParser for adding features.
commit d8dfaadaa4cdf96692093bc96bfe33a01d39026c
Author: Gorkem Ercan <go...@gmail.com>
Date: 2014-04-17T20:49:14Z
initial set of tests
add tests for new save command and adds test for the ConfigParser.addFeature
----
---
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.
---
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
New PRs created
https://github.com/apache/cordova-lib/pull/6
https://github.com/apache/cordova-cli/pull/176
On Tue, May 13, 2014 at 9:07 PM, gorkem <gi...@git.apache.org> wrote:
> Github user gorkem closed the pull request at:
>
> https://github.com/apache/cordova-cli/pull/165
>
>
> ---
> 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.
> ---
>
[GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by gorkem <gi...@git.apache.org>.
Github user gorkem closed the pull request at:
https://github.com/apache/cordova-cli/pull/165
---
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.
---
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
I have also added a --shrinkwrap flag and if it is not specified it will
not save the version information. This will in turn cause the restore to
get the latest version from registry. The plan is to keep using the same
flag for platforms as well.
--
Gorkem
On Mon, May 12, 2014 at 11:21 AM, Michal Mocny <mm...@chromium.org> wrote:
> (poke, due to email)
>
>
> On Wed, May 7, 2014 at 2:45 PM, Michal Mocny <mm...@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 <mm...@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 <mm...@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 <mm...@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 <gi...@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.
> >>>> > > > > > ---
> >>>> > > > > >
> >>>> > > > >
> >>>> > > >
> >>>> > >
> >>>> >
> >>>>
> >>>
> >>>
> >>
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
(poke, due to email)
On Wed, May 7, 2014 at 2:45 PM, Michal Mocny <mm...@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 <mm...@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 <mm...@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 <go...@gmail.com>wrote:
>>>
>>>> Any updates for me?
>>>> --
>>>> Gorkem
>>>>
>>>>
>>>> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mm...@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 <mm...@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 <pu...@gmail.com>
>>>> > wrote:
>>>> > > >
>>>> > > > > Yeah, that looks weird.
>>>> > > > >
>>>> > > > > @purplecabbage
>>>> > > > > risingj.com
>>>> > > > >
>>>> > > > >
>>>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
>>>> > > > > > ---
>>>> > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>>
>>>
>>>
>>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
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 <mm...@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 <mm...@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 <go...@gmail.com>wrote:
>>
>>> Any updates for me?
>>> --
>>> Gorkem
>>>
>>>
>>> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mm...@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 <mm...@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 <pu...@gmail.com>
>>> > wrote:
>>> > > >
>>> > > > > Yeah, that looks weird.
>>> > > > >
>>> > > > > @purplecabbage
>>> > > > > risingj.com
>>> > > > >
>>> > > > >
>>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
>>> > > > > > ---
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
..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 <mm...@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 <go...@gmail.com>wrote:
>
>> Any updates for me?
>> --
>> Gorkem
>>
>>
>> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mm...@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 <mm...@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 <pu...@gmail.com>
>> > wrote:
>> > > >
>> > > > > Yeah, that looks weird.
>> > > > >
>> > > > > @purplecabbage
>> > > > > risingj.com
>> > > > >
>> > > > >
>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
>> > > > > > ---
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
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 <go...@gmail.com>wrote:
> Any updates for me?
> --
> Gorkem
>
>
> On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mm...@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 <mm...@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 <pu...@gmail.com>
> > wrote:
> > > >
> > > > > Yeah, that looks weird.
> > > > >
> > > > > @purplecabbage
> > > > > risingj.com
> > > > >
> > > > >
> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > > > > > ---
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
Any updates for me?
--
Gorkem
On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny <mm...@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 <mm...@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 <pu...@gmail.com>
> wrote:
> > >
> > > > Yeah, that looks weird.
> > > >
> > > > @purplecabbage
> > > > risingj.com
> > > >
> > > >
> > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > > > > ---
> > > > >
> > > >
> > >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
I'll take a look again today and land/comment.
On Thu, May 1, 2014 at 8:28 AM, Gorkem Ercan <go...@gmail.com> wrote:
> I would appreciate it if someone can have a final look and get this in.. I
> have doc updates to follow.
> Thanks
> --
> Gorkem
>
>
> On Fri, Apr 25, 2014 at 4:37 PM, Gorkem Ercan <gorkem.ercan@gmail.com
> >wrote:
>
> > FYI. Just updated the PR with separated save & restore commands.
> > --
> > Gorkem
> >
> >
> > On Thu, Apr 24, 2014 at 7:21 PM, Gorkem Ercan <gorkem.ercan@gmail.com
> >wrote:
> >
> >> I guess this is essentially moving save command as a flag to cordova
> >> plugin * commands and restore becomes harder to discover. We need to
> >> consider the platforms too since next stop is implementing "cordova
> >> save/restore platforms"
> >> --
> >> Gorkem
> >>
> >>
> >> On Thu, Apr 24, 2014 at 5:05 PM, Michal Mocny <mmocny@chromium.org
> >wrote:
> >>
> >>> Gorkem,
> >>>
> >>> I also found an old JIRA duplicate issue for this which had listed an
> old
> >>> suggestion I think is interesting:
> >>> https://issues.apache.org/jira/browse/CB-4624
> >>>
> >>> Specifically, instead of introducing save/restore commands, we could
> >>> mirror
> >>> "npm install" and its use of "--save":
> >>> - npm install -> cordova plugin add
> >>> - restores deps
> >>> - npm install foo -> cordova plugin add foo
> >>> - install foo, but don't add it to deps
> >>> - npm install foo --save -> cordova plugin add foo --save
> >>> - install foo, and do add it to deps
> >>> - npm install --save -> cordova plugin add --save
> >>> - don't install anything, just save the current installed plugins
> into
> >>> deps
> >>>
> >>> Just something to consider.
> >>>
> >>> (note, just tried it again and npm install --save doesn't actually do
> >>> what
> >>> I want.. wonder if its supported..)
> >>>
> >>> -Michal
> >>>
> >>>
> >>> On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mm...@chromium.org>
> >>> wrote:
> >>>
> >>> > Gorkem, as an orthogonal issue to the syntax we use, I think there is
> >>> > another small problem with this patch as-is.
> >>> >
> >>> > "cordova plugin add org.apache.cordova.file-transfer && cordova
> plugin
> >>> > save" would have my application explicitly depend on
> >>> > org.apache.cordova.file. If in the future the dependency is
> >>> > removed/moved/renamed, my app would explicitly try to install it when
> >>> > running "cordova plugin restore".
> >>> >
> >>> > As a first version I think this is acceptable, but I think we may
> want
> >>> a
> >>> > better solution eventually.
> >>> >
> >>> >
> >>> > On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org>
> >>> wrote:
> >>> >
> >>> >>
> >>> >>
> >>> >>
> >>> >> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <
> >>> gorkem.ercan@gmail.com>wrote:
> >>> >>
> >>> >>> On Fri, Apr 18, 2014 at 7: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.
> >>> >>> >
> >>> >>>
> >>> >>> Makes sense... adding a 'restore' command and removing from
> prepare.
> >>> We
> >>> >>> can
> >>> >>> add an auto-restore config in the next iteration.
> >>> >>>
> >>> >>
> >>> >> Excellent, thanks.
> >>> >>
> >>> >>
> >>> >>>
> >>> >>>
> >>> >>> >
> >>> >>> >
> >>> >>> > >
> >>> >>> > >
> >>> >>> > > > - 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.
> >>> >>> >
> >>> >>> >
> >>> >>> Tools developers are people too :) I think plugin.xml and
> config.xml
> >>> has
> >>> >>> two different audiences, I think there is a comparatively small
> >>> >>> intersection of developers who are interested on both. At the
> >>> moment, we
> >>> >>> are pretty much within the w3c spec with the top-level config.xml
> >>> and I
> >>> >>> do
> >>> >>> not see the benefit of breaking it.
> >>> >>>
> >>> >>
> >>> >> Actually, I am thinking about tools devs. Namely, our tools devs
> who
> >>> >> should be using the same config parsing and handlers for dependency
> >>> >> management, etc.
> >>> >>
> >>> >> Anyway. You are putting in the sweat and blood on this feature, so
> >>> you
> >>> >> get double the voting power on this as far as I'm concerned.
> Still, I
> >>> >> think we should bring this to the attention of the list to see how
> >>> everyone
> >>> >> feels. Sound fair? I'll start a quick thread.
> >>> >>
> >>> >>
> >>> >>>
> >>> >>>
> >>> >>> >
> >>> >>> > >
> >>> >>> > >
> >>> >>> > >
> >>> >>> > > > 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.
> >>> >>> > > > > > ---
> >>> >>> > > > > >
> >>> >>> > > > >
> >>> >>> > > >
> >>> >>> > >
> >>> >>> >
> >>> >>>
> >>> >>
> >>> >>
> >>> >
> >>>
> >>
> >>
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
I would appreciate it if someone can have a final look and get this in.. I
have doc updates to follow.
Thanks
--
Gorkem
On Fri, Apr 25, 2014 at 4:37 PM, Gorkem Ercan <go...@gmail.com>wrote:
> FYI. Just updated the PR with separated save & restore commands.
> --
> Gorkem
>
>
> On Thu, Apr 24, 2014 at 7:21 PM, Gorkem Ercan <go...@gmail.com>wrote:
>
>> I guess this is essentially moving save command as a flag to cordova
>> plugin * commands and restore becomes harder to discover. We need to
>> consider the platforms too since next stop is implementing "cordova
>> save/restore platforms"
>> --
>> Gorkem
>>
>>
>> On Thu, Apr 24, 2014 at 5:05 PM, Michal Mocny <mm...@chromium.org>wrote:
>>
>>> Gorkem,
>>>
>>> I also found an old JIRA duplicate issue for this which had listed an old
>>> suggestion I think is interesting:
>>> https://issues.apache.org/jira/browse/CB-4624
>>>
>>> Specifically, instead of introducing save/restore commands, we could
>>> mirror
>>> "npm install" and its use of "--save":
>>> - npm install -> cordova plugin add
>>> - restores deps
>>> - npm install foo -> cordova plugin add foo
>>> - install foo, but don't add it to deps
>>> - npm install foo --save -> cordova plugin add foo --save
>>> - install foo, and do add it to deps
>>> - npm install --save -> cordova plugin add --save
>>> - don't install anything, just save the current installed plugins into
>>> deps
>>>
>>> Just something to consider.
>>>
>>> (note, just tried it again and npm install --save doesn't actually do
>>> what
>>> I want.. wonder if its supported..)
>>>
>>> -Michal
>>>
>>>
>>> On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mm...@chromium.org>
>>> wrote:
>>>
>>> > Gorkem, as an orthogonal issue to the syntax we use, I think there is
>>> > another small problem with this patch as-is.
>>> >
>>> > "cordova plugin add org.apache.cordova.file-transfer && cordova plugin
>>> > save" would have my application explicitly depend on
>>> > org.apache.cordova.file. If in the future the dependency is
>>> > removed/moved/renamed, my app would explicitly try to install it when
>>> > running "cordova plugin restore".
>>> >
>>> > As a first version I think this is acceptable, but I think we may want
>>> a
>>> > better solution eventually.
>>> >
>>> >
>>> > On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org>
>>> wrote:
>>> >
>>> >>
>>> >>
>>> >>
>>> >> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <
>>> gorkem.ercan@gmail.com>wrote:
>>> >>
>>> >>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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.
>>> >>> >
>>> >>>
>>> >>> Makes sense... adding a 'restore' command and removing from prepare.
>>> We
>>> >>> can
>>> >>> add an auto-restore config in the next iteration.
>>> >>>
>>> >>
>>> >> Excellent, thanks.
>>> >>
>>> >>
>>> >>>
>>> >>>
>>> >>> >
>>> >>> >
>>> >>> > >
>>> >>> > >
>>> >>> > > > - 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.
>>> >>> >
>>> >>> >
>>> >>> Tools developers are people too :) I think plugin.xml and config.xml
>>> has
>>> >>> two different audiences, I think there is a comparatively small
>>> >>> intersection of developers who are interested on both. At the
>>> moment, we
>>> >>> are pretty much within the w3c spec with the top-level config.xml
>>> and I
>>> >>> do
>>> >>> not see the benefit of breaking it.
>>> >>>
>>> >>
>>> >> Actually, I am thinking about tools devs. Namely, our tools devs who
>>> >> should be using the same config parsing and handlers for dependency
>>> >> management, etc.
>>> >>
>>> >> Anyway. You are putting in the sweat and blood on this feature, so
>>> you
>>> >> get double the voting power on this as far as I'm concerned. Still, I
>>> >> think we should bring this to the attention of the list to see how
>>> everyone
>>> >> feels. Sound fair? I'll start a quick thread.
>>> >>
>>> >>
>>> >>>
>>> >>>
>>> >>> >
>>> >>> > >
>>> >>> > >
>>> >>> > >
>>> >>> > > > 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 <gi...@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.
>>> >>> > > > > > ---
>>> >>> > > > > >
>>> >>> > > > >
>>> >>> > > >
>>> >>> > >
>>> >>> >
>>> >>>
>>> >>
>>> >>
>>> >
>>>
>>
>>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
FYI. Just updated the PR with separated save & restore commands.
--
Gorkem
On Thu, Apr 24, 2014 at 7:21 PM, Gorkem Ercan <go...@gmail.com>wrote:
> I guess this is essentially moving save command as a flag to cordova
> plugin * commands and restore becomes harder to discover. We need to
> consider the platforms too since next stop is implementing "cordova
> save/restore platforms"
> --
> Gorkem
>
>
> On Thu, Apr 24, 2014 at 5:05 PM, Michal Mocny <mm...@chromium.org> wrote:
>
>> Gorkem,
>>
>> I also found an old JIRA duplicate issue for this which had listed an old
>> suggestion I think is interesting:
>> https://issues.apache.org/jira/browse/CB-4624
>>
>> Specifically, instead of introducing save/restore commands, we could
>> mirror
>> "npm install" and its use of "--save":
>> - npm install -> cordova plugin add
>> - restores deps
>> - npm install foo -> cordova plugin add foo
>> - install foo, but don't add it to deps
>> - npm install foo --save -> cordova plugin add foo --save
>> - install foo, and do add it to deps
>> - npm install --save -> cordova plugin add --save
>> - don't install anything, just save the current installed plugins into
>> deps
>>
>> Just something to consider.
>>
>> (note, just tried it again and npm install --save doesn't actually do what
>> I want.. wonder if its supported..)
>>
>> -Michal
>>
>>
>> On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mm...@chromium.org>
>> wrote:
>>
>> > Gorkem, as an orthogonal issue to the syntax we use, I think there is
>> > another small problem with this patch as-is.
>> >
>> > "cordova plugin add org.apache.cordova.file-transfer && cordova plugin
>> > save" would have my application explicitly depend on
>> > org.apache.cordova.file. If in the future the dependency is
>> > removed/moved/renamed, my app would explicitly try to install it when
>> > running "cordova plugin restore".
>> >
>> > As a first version I think this is acceptable, but I think we may want a
>> > better solution eventually.
>> >
>> >
>> > On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org>
>> wrote:
>> >
>> >>
>> >>
>> >>
>> >> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <gorkem.ercan@gmail.com
>> >wrote:
>> >>
>> >>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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.
>> >>> >
>> >>>
>> >>> Makes sense... adding a 'restore' command and removing from prepare.
>> We
>> >>> can
>> >>> add an auto-restore config in the next iteration.
>> >>>
>> >>
>> >> Excellent, thanks.
>> >>
>> >>
>> >>>
>> >>>
>> >>> >
>> >>> >
>> >>> > >
>> >>> > >
>> >>> > > > - 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.
>> >>> >
>> >>> >
>> >>> Tools developers are people too :) I think plugin.xml and config.xml
>> has
>> >>> two different audiences, I think there is a comparatively small
>> >>> intersection of developers who are interested on both. At the moment,
>> we
>> >>> are pretty much within the w3c spec with the top-level config.xml and
>> I
>> >>> do
>> >>> not see the benefit of breaking it.
>> >>>
>> >>
>> >> Actually, I am thinking about tools devs. Namely, our tools devs who
>> >> should be using the same config parsing and handlers for dependency
>> >> management, etc.
>> >>
>> >> Anyway. You are putting in the sweat and blood on this feature, so you
>> >> get double the voting power on this as far as I'm concerned. Still, I
>> >> think we should bring this to the attention of the list to see how
>> everyone
>> >> feels. Sound fair? I'll start a quick thread.
>> >>
>> >>
>> >>>
>> >>>
>> >>> >
>> >>> > >
>> >>> > >
>> >>> > >
>> >>> > > > 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 <gi...@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.
>> >>> > > > > > ---
>> >>> > > > > >
>> >>> > > > >
>> >>> > > >
>> >>> > >
>> >>> >
>> >>>
>> >>
>> >>
>> >
>>
>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
I guess this is essentially moving save command as a flag to cordova plugin
* commands and restore becomes harder to discover. We need to consider the
platforms too since next stop is implementing "cordova save/restore
platforms"
--
Gorkem
On Thu, Apr 24, 2014 at 5:05 PM, Michal Mocny <mm...@chromium.org> wrote:
> Gorkem,
>
> I also found an old JIRA duplicate issue for this which had listed an old
> suggestion I think is interesting:
> https://issues.apache.org/jira/browse/CB-4624
>
> Specifically, instead of introducing save/restore commands, we could mirror
> "npm install" and its use of "--save":
> - npm install -> cordova plugin add
> - restores deps
> - npm install foo -> cordova plugin add foo
> - install foo, but don't add it to deps
> - npm install foo --save -> cordova plugin add foo --save
> - install foo, and do add it to deps
> - npm install --save -> cordova plugin add --save
> - don't install anything, just save the current installed plugins into
> deps
>
> Just something to consider.
>
> (note, just tried it again and npm install --save doesn't actually do what
> I want.. wonder if its supported..)
>
> -Michal
>
>
> On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mm...@chromium.org>
> wrote:
>
> > Gorkem, as an orthogonal issue to the syntax we use, I think there is
> > another small problem with this patch as-is.
> >
> > "cordova plugin add org.apache.cordova.file-transfer && cordova plugin
> > save" would have my application explicitly depend on
> > org.apache.cordova.file. If in the future the dependency is
> > removed/moved/renamed, my app would explicitly try to install it when
> > running "cordova plugin restore".
> >
> > As a first version I think this is acceptable, but I think we may want a
> > better solution eventually.
> >
> >
> > On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org>
> wrote:
> >
> >>
> >>
> >>
> >> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <gorkem.ercan@gmail.com
> >wrote:
> >>
> >>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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.
> >>> >
> >>>
> >>> Makes sense... adding a 'restore' command and removing from prepare. We
> >>> can
> >>> add an auto-restore config in the next iteration.
> >>>
> >>
> >> Excellent, thanks.
> >>
> >>
> >>>
> >>>
> >>> >
> >>> >
> >>> > >
> >>> > >
> >>> > > > - 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.
> >>> >
> >>> >
> >>> Tools developers are people too :) I think plugin.xml and config.xml
> has
> >>> two different audiences, I think there is a comparatively small
> >>> intersection of developers who are interested on both. At the moment,
> we
> >>> are pretty much within the w3c spec with the top-level config.xml and I
> >>> do
> >>> not see the benefit of breaking it.
> >>>
> >>
> >> Actually, I am thinking about tools devs. Namely, our tools devs who
> >> should be using the same config parsing and handlers for dependency
> >> management, etc.
> >>
> >> Anyway. You are putting in the sweat and blood on this feature, so you
> >> get double the voting power on this as far as I'm concerned. Still, I
> >> think we should bring this to the attention of the list to see how
> everyone
> >> feels. Sound fair? I'll start a quick thread.
> >>
> >>
> >>>
> >>>
> >>> >
> >>> > >
> >>> > >
> >>> > >
> >>> > > > I haven't put the PR through testing yet.
> >>> > > >
> >>> > > >
> >>> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com>
> >>> > wrote:
> >>> > > >
> >>> > > > > Yeah, that looks weird.
> >>> > > > >
> >>> > > > > @purplecabbage
> >>> > > > > risingj.com
> >>> > > > >
> >>> > > > >
> >>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> >>> > > > > > ---
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> >>
> >>
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
Gorkem,
I also found an old JIRA duplicate issue for this which had listed an old
suggestion I think is interesting:
https://issues.apache.org/jira/browse/CB-4624
Specifically, instead of introducing save/restore commands, we could mirror
"npm install" and its use of "--save":
- npm install -> cordova plugin add
- restores deps
- npm install foo -> cordova plugin add foo
- install foo, but don't add it to deps
- npm install foo --save -> cordova plugin add foo --save
- install foo, and do add it to deps
- npm install --save -> cordova plugin add --save
- don't install anything, just save the current installed plugins into
deps
Just something to consider.
(note, just tried it again and npm install --save doesn't actually do what
I want.. wonder if its supported..)
-Michal
On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mm...@chromium.org> wrote:
> Gorkem, as an orthogonal issue to the syntax we use, I think there is
> another small problem with this patch as-is.
>
> "cordova plugin add org.apache.cordova.file-transfer && cordova plugin
> save" would have my application explicitly depend on
> org.apache.cordova.file. If in the future the dependency is
> removed/moved/renamed, my app would explicitly try to install it when
> running "cordova plugin restore".
>
> As a first version I think this is acceptable, but I think we may want a
> better solution eventually.
>
>
> On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org> wrote:
>
>>
>>
>>
>> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <go...@gmail.com>wrote:
>>
>>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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 <mm...@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.
>>> >
>>>
>>> Makes sense... adding a 'restore' command and removing from prepare. We
>>> can
>>> add an auto-restore config in the next iteration.
>>>
>>
>> Excellent, thanks.
>>
>>
>>>
>>>
>>> >
>>> >
>>> > >
>>> > >
>>> > > > - 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.
>>> >
>>> >
>>> Tools developers are people too :) I think plugin.xml and config.xml has
>>> two different audiences, I think there is a comparatively small
>>> intersection of developers who are interested on both. At the moment, we
>>> are pretty much within the w3c spec with the top-level config.xml and I
>>> do
>>> not see the benefit of breaking it.
>>>
>>
>> Actually, I am thinking about tools devs. Namely, our tools devs who
>> should be using the same config parsing and handlers for dependency
>> management, etc.
>>
>> Anyway. You are putting in the sweat and blood on this feature, so you
>> get double the voting power on this as far as I'm concerned. Still, I
>> think we should bring this to the attention of the list to see how everyone
>> feels. Sound fair? I'll start a quick thread.
>>
>>
>>>
>>>
>>> >
>>> > >
>>> > >
>>> > >
>>> > > > I haven't put the PR through testing yet.
>>> > > >
>>> > > >
>>> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com>
>>> > wrote:
>>> > > >
>>> > > > > Yeah, that looks weird.
>>> > > > >
>>> > > > > @purplecabbage
>>> > > > > risingj.com
>>> > > > >
>>> > > > >
>>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
>>> > > > > > ---
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
My plan is to add a warning to "cordova plugin rm" that will remind to
explicitly call "cordova save plugins" to refresh the plugin dependencies.
In a future iteration we can add a "keep my dependencies always restorable"
flag so that cordova plugin rm/add also do the save. (and prepare does the
auto restore)
--
Gorkem
On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mm...@chromium.org> wrote:
> Gorkem, as an orthogonal issue to the syntax we use, I think there is
> another small problem with this patch as-is.
>
> "cordova plugin add org.apache.cordova.file-transfer && cordova plugin
> save" would have my application explicitly depend on
> org.apache.cordova.file. If in the future the dependency is
> removed/moved/renamed, my app would explicitly try to install it when
> running "cordova plugin restore".
>
> As a first version I think this is acceptable, but I think we may want a
> better solution eventually.
>
>
> On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org> wrote:
>
> >
> >
> >
> > On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <gorkem.ercan@gmail.com
> >wrote:
> >
> >> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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 <mm...@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.
> >> >
> >>
> >> Makes sense... adding a 'restore' command and removing from prepare. We
> >> can
> >> add an auto-restore config in the next iteration.
> >>
> >
> > Excellent, thanks.
> >
> >
> >>
> >>
> >> >
> >> >
> >> > >
> >> > >
> >> > > > - 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.
> >> >
> >> >
> >> Tools developers are people too :) I think plugin.xml and config.xml has
> >> two different audiences, I think there is a comparatively small
> >> intersection of developers who are interested on both. At the moment, we
> >> are pretty much within the w3c spec with the top-level config.xml and I
> do
> >> not see the benefit of breaking it.
> >>
> >
> > Actually, I am thinking about tools devs. Namely, our tools devs who
> > should be using the same config parsing and handlers for dependency
> > management, etc.
> >
> > Anyway. You are putting in the sweat and blood on this feature, so you
> > get double the voting power on this as far as I'm concerned. Still, I
> > think we should bring this to the attention of the list to see how
> everyone
> > feels. Sound fair? I'll start a quick thread.
> >
> >
> >>
> >>
> >> >
> >> > >
> >> > >
> >> > >
> >> > > > I haven't put the PR through testing yet.
> >> > > >
> >> > > >
> >> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com>
> >> > wrote:
> >> > > >
> >> > > > > Yeah, that looks weird.
> >> > > > >
> >> > > > > @purplecabbage
> >> > > > > risingj.com
> >> > > > >
> >> > > > >
> >> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> >> > > > > > ---
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
Gorkem, as an orthogonal issue to the syntax we use, I think there is
another small problem with this patch as-is.
"cordova plugin add org.apache.cordova.file-transfer && cordova plugin
save" would have my application explicitly depend on
org.apache.cordova.file. If in the future the dependency is
removed/moved/renamed, my app would explicitly try to install it when
running "cordova plugin restore".
As a first version I think this is acceptable, but I think we may want a
better solution eventually.
On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mm...@chromium.org> wrote:
>
>
>
> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <go...@gmail.com>wrote:
>
>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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 <mm...@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.
>> >
>>
>> Makes sense... adding a 'restore' command and removing from prepare. We
>> can
>> add an auto-restore config in the next iteration.
>>
>
> Excellent, thanks.
>
>
>>
>>
>> >
>> >
>> > >
>> > >
>> > > > - 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.
>> >
>> >
>> Tools developers are people too :) I think plugin.xml and config.xml has
>> two different audiences, I think there is a comparatively small
>> intersection of developers who are interested on both. At the moment, we
>> are pretty much within the w3c spec with the top-level config.xml and I do
>> not see the benefit of breaking it.
>>
>
> Actually, I am thinking about tools devs. Namely, our tools devs who
> should be using the same config parsing and handlers for dependency
> management, etc.
>
> Anyway. You are putting in the sweat and blood on this feature, so you
> get double the voting power on this as far as I'm concerned. Still, I
> think we should bring this to the attention of the list to see how everyone
> feels. Sound fair? I'll start a quick thread.
>
>
>>
>>
>> >
>> > >
>> > >
>> > >
>> > > > I haven't put the PR through testing yet.
>> > > >
>> > > >
>> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com>
>> > wrote:
>> > > >
>> > > > > Yeah, that looks weird.
>> > > > >
>> > > > > @purplecabbage
>> > > > > risingj.com
>> > > > >
>> > > > >
>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
>> > > > > > ---
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <go...@gmail.com>wrote:
> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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 <mm...@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.
> >
>
> Makes sense... adding a 'restore' command and removing from prepare. We can
> add an auto-restore config in the next iteration.
>
Excellent, thanks.
>
>
> >
> >
> > >
> > >
> > > > - 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.
> >
> >
> Tools developers are people too :) I think plugin.xml and config.xml has
> two different audiences, I think there is a comparatively small
> intersection of developers who are interested on both. At the moment, we
> are pretty much within the w3c spec with the top-level config.xml and I do
> not see the benefit of breaking it.
>
Actually, I am thinking about tools devs. Namely, our tools devs who
should be using the same config parsing and handlers for dependency
management, etc.
Anyway. You are putting in the sweat and blood on this feature, so you get
double the voting power on this as far as I'm concerned. Still, I think we
should bring this to the attention of the list to see how everyone feels.
Sound fair? I'll start a quick thread.
>
>
> >
> > >
> > >
> > >
> > > > I haven't put the PR through testing yet.
> > > >
> > > >
> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com>
> > wrote:
> > > >
> > > > > Yeah, that looks weird.
> > > > >
> > > > > @purplecabbage
> > > > > risingj.com
> > > > >
> > > > >
> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > > > > > ---
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mm...@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 <mm...@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.
>
Makes sense... adding a 'restore' command and removing from prepare. We can
add an auto-restore config in the next iteration.
>
>
> >
> >
> > > - 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.
>
>
Tools developers are people too :) I think plugin.xml and config.xml has
two different audiences, I think there is a comparatively small
intersection of developers who are interested on both. At the moment, we
are pretty much within the w3c spec with the top-level config.xml and I do
not see the benefit of breaking it.
>
> >
> >
> >
> > > I haven't put the PR through testing yet.
> > >
> > >
> > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com>
> wrote:
> > >
> > > > Yeah, that looks weird.
> > > >
> > > > @purplecabbage
> > > > risingj.com
> > > >
> > > >
> > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > > > > ---
> > > > >
> > > >
> > >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan <go...@gmail.com>wrote:
> On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <mm...@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 <pu...@gmail.com> wrote:
> >
> > > Yeah, that looks weird.
> > >
> > > @purplecabbage
> > > risingj.com
> > >
> > >
> > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > > > ---
> > > >
> > >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Gorkem Ercan <go...@gmail.com>.
On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <mm...@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.
> - 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.
> I haven't put the PR through testing yet.
>
>
> On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com> wrote:
>
> > Yeah, that looks weird.
> >
> > @purplecabbage
> > risingj.com
> >
> >
> > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > > ---
> > >
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Michal Mocny <mm...@chromium.org>.
Took a quick glance. General questions:
- why the need for save? Why not just alter the list on each cordova
plugin add/rm?
- wouldn't "cordova plugin rm foo && cordova prepare" re-install that
plugin right now?
- why the name <feature> and not <dependency> ? I think this functionality
should overlap with the plugin.xml spec.
I haven't put the PR through testing yet.
On Thu, Apr 17, 2014 at 5:54 PM, Jesse <pu...@gmail.com> wrote:
> Yeah, that looks weird.
>
> @purplecabbage
> risingj.com
>
>
> On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> > ---
> >
>
Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by Jesse <pu...@gmail.com>.
Yeah, that looks weird.
@purplecabbage
risingj.com
On Thu, Apr 17, 2014 at 2:46 PM, kamrik <gi...@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.
> ---
>
[GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by kamrik <gi...@git.apache.org>.
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.
---
[GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by gorkem <gi...@git.apache.org>.
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-cli/pull/165#issuecomment-43031932
new PR coming
---
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.
---
[GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Posted by gorkem <gi...@git.apache.org>.
Github user gorkem commented on the pull request:
https://github.com/apache/cordova-cli/pull/165#issuecomment-40837864
JSON parse is a Copy/paste problem. I have based the implemantation of save from an earlier prototype I had that did a bower like json file. Updated now.
---
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.
---