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 AF5B1FD47 for ; Fri, 25 Apr 2014 20:38:14 +0000 (UTC) Received: (qmail 83040 invoked by uid 500); 25 Apr 2014 20:38:14 -0000 Delivered-To: apmail-cordova-dev-archive@cordova.apache.org Received: (qmail 82991 invoked by uid 500); 25 Apr 2014 20:38:13 -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 82983 invoked by uid 99); 25 Apr 2014 20:38:13 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 25 Apr 2014 20:38:13 +0000 X-ASF-Spam-Status: No, hits=2.5 required=5.0 tests=FREEMAIL_REPLY,HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of gorkem.ercan@gmail.com designates 209.85.213.47 as permitted sender) Received: from [209.85.213.47] (HELO mail-yh0-f47.google.com) (209.85.213.47) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 25 Apr 2014 20:38:08 +0000 Received: by mail-yh0-f47.google.com with SMTP id 29so4025015yhl.20 for ; Fri, 25 Apr 2014 13:37:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=Zrn4odiTWNB1SwiFqWtLM59C6g35R97MY75l7G5BlwM=; b=Pnrs5VV+EKqA3FWIRDq3xRY81BdeCWe+5D7yIUI1UXGfDLKZMyG+CfTOv/9Bjcw8tb G2j/emL6rYBBNI3F+GCSSGoXFTfYYZGzSFzF2EvPizxlgEN+QuKO5iq1IcWcbepTdapE MVdl+KL0Zp3C0c5jHcag+l2E5CKuEG/iq2xzTgFUS5vHWtvYV6wCPLkmf0dnLcF1joF9 l4WH/+HTurePknQ+/LF/hNs4PmNi4Weif4wy8UF4fPR+f2bVC9gwTsp1w13HvJZAw0wP QrErXI0+pAvK82lGf0tIEzGO8jk7vONyqAb0OGbtYTQgpRQLbQckkwjOOTODoXhgDhhv kTsg== MIME-Version: 1.0 X-Received: by 10.236.130.178 with SMTP id k38mr5790970yhi.132.1398458264547; Fri, 25 Apr 2014 13:37:44 -0700 (PDT) Received: by 10.170.85.66 with HTTP; Fri, 25 Apr 2014 13:37:44 -0700 (PDT) In-Reply-To: References: <20140417214643.E0C2B9923CF@tyr.zones.apache.org> Date: Fri, 25 Apr 2014 16:37:44 -0400 Message-ID: Subject: Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm... From: Gorkem Ercan To: "dev@cordova.apache.org" Content-Type: multipart/alternative; boundary=20cf3005deae17760f04f7e3f069 X-Virus-Checked: Checked by ClamAV on apache.org --20cf3005deae17760f04f7e3f069 Content-Type: text/plain; charset=UTF-8 FYI. Just updated the PR with separated save & restore commands. -- Gorkem On Thu, Apr 24, 2014 at 7:21 PM, Gorkem Ercan 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 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 >> 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 >> wrote: >> > >> >> >> >> >> >> >> >> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan > >wrote: >> >> >> >>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny >> >>> 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 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. >> >>> > >> >>> > >> >>> 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 > > >> >>> > 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. >> >>> > > > > > --- >> >>> > > > > > >> >>> > > > > >> >>> > > > >> >>> > > >> >>> > >> >>> >> >> >> >> >> > >> > > --20cf3005deae17760f04f7e3f069--