Return-Path: X-Original-To: apmail-cordova-dev-archive@www.apache.org Delivered-To: apmail-cordova-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 1257311638 for ; Sun, 11 May 2014 20:37:57 +0000 (UTC) Received: (qmail 25604 invoked by uid 500); 10 May 2014 23:28:13 -0000 Delivered-To: apmail-cordova-dev-archive@cordova.apache.org Received: (qmail 66409 invoked by uid 500); 10 May 2014 23:16:35 -0000 Mailing-List: contact dev-help@cordova.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cordova.apache.org Delivered-To: mailing list dev@cordova.apache.org Received: (qmail 57529 invoked by uid 99); 10 May 2014 23:08:04 -0000 Received: from Unknown (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 10 May 2014 23:08:04 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of mmocny@google.com designates 209.85.220.182 as permitted sender) Received: from [209.85.220.182] (HELO mail-vc0-f182.google.com) (209.85.220.182) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 07 May 2014 18:04:13 +0000 Received: by mail-vc0-f182.google.com with SMTP id la4so1796118vcb.13 for ; Wed, 07 May 2014 11:03:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:content-type; bh=3bHTp0M9zjC1evx4YnD+81ng1o4aeNvPbCoxgqVfsTc=; b=RQoT5TMkxAutzngx4EVmVWzut8ZKBh9nA3oo3YN0D7btpe53bO2VkRiK/Sldonc9D9 DCv1ZcL3HvW8shfxq6QbKDDLXqQXq8sKHkomXuSRho8CG1/COUOcLmAuFaYb5QnbV8ES Ruy4r6d8rNkzVBc+E9K+XC0kQWZOZ/hRB4iqqNjpU43nNr7Tblt1QBCz1rgy1k/WTQxk Tr6E6r0nzvrJBNzC2OCY/UypDLHTzUB4LJY7zpqNfDOlHlChfAtic7btmBEXcL9sDbZS flUd14ujscJInL/ngJ9VE8khksOKtCceGp3f1c++R1npcNlfIZC3bdXvj5ZgmTYEc6t8 30Pw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:content-type; bh=3bHTp0M9zjC1evx4YnD+81ng1o4aeNvPbCoxgqVfsTc=; b=LUg/2eNMEzMFhm55Iz7guOpjWYYmzFU0OMmY2uFeY3M2jdnXo21o+RsEplbBEfpU/8 /As3Q0i7wliTyEuq1nOmYJ+GmnftOuy6KPhZzjbEcWMs4FZpdrel/WJVUH8TEjQ7rWlm JW0VWYkBtwtqzhGnyEaf23NexPNpiBYDHBsDo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:content-type; bh=3bHTp0M9zjC1evx4YnD+81ng1o4aeNvPbCoxgqVfsTc=; b=fMq9UFqPrRMnqQr1AYrWWQwlrz9S5h54UirZFx6Xtoye1t+m2x59YUKuWdunKoDsCc O6op7DuJJIyq1AsHXanZXw/7D4OTFPDXsIY8ehR1y2cdnWiUE+e0DzGehbF/1Y2zDSWk UVKaNxuV0J/jIVgjdljLtv7jd/ER6pSiWPoIMykXljPszyZVL0qNG0hHzonfC5PpfRJ1 tekXQp+l1cckPB8rAxM4/QzKRYLZ+3nx9JyUq0+ox0PpbU4I3GXBG2X3JlP9Nskdkiqa Gv143M2M8NYpBTyYRX8wvGa0zBjEmITEjTuLo9IKCxnQtikCuGjgKfyVUDkcHMPzHD/h ZANA== X-Gm-Message-State: ALoCoQlIG7rdkrhNExjRq+zbZAEgPmNFC6FW+Ox1GNopBwWggZOcLnZx56R6UB1hLsN16fpLzmXg X-Received: by 10.220.183.4 with SMTP id ce4mr2010516vcb.54.1399485829775; Wed, 07 May 2014 11:03:49 -0700 (PDT) MIME-Version: 1.0 Sender: mmocny@google.com Received: by 10.52.31.101 with HTTP; Wed, 7 May 2014 11:03:29 -0700 (PDT) In-Reply-To: References: <20140417214643.E0C2B9923CF@tyr.zones.apache.org> From: Michal Mocny Date: Wed, 7 May 2014 14:03:29 -0400 X-Google-Sender-Auth: IeS4i7_d8LWjheAyZ18nDKX11uk Message-ID: Subject: Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm... To: dev Content-Type: multipart/alternative; boundary=089e0139fe9ec0a16804f8d32f45 X-Virus-Checked: Checked by ClamAV on apache.org --089e0139fe9ec0a16804f8d32f45 Content-Type: text/plain; charset=UTF-8 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 -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 wrote: > Any updates for me? > -- > Gorkem > > > On Fri, Apr 18, 2014 at 10:21 PM, Michal Mocny > wrote: > > > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan > >wrote: > > > > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny > > 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 and not ? 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 > > wrote: > > > > > > > > > Yeah, that looks weird. > > > > > > > > > > @purplecabbage > > > > > risingj.com > > > > > > > > > > > > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik > 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 > > > > > + 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. > > > > > > --- > > > > > > > > > > > > > > > > > > > > > --089e0139fe9ec0a16804f8d32f45--