You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by bso-intel <gi...@git.apache.org> on 2016/02/21 07:00:14 UTC
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
GitHub user bso-intel opened a pull request:
https://github.com/apache/cordova-lib/pull/396
CB-10314 avoid fetching plugins when oldId is already fetched
implement the fix to the issue in CB-10314.
Before start fetching the plugin, first check if the plugin with alias id is already fetched in the project.
Otherwise, it is a waste of time fetching the plugin with newId and then decide not to install.
This pull request also includes a fix to the issue of reporting the bogus mismatched id error which is caused by fetching the new-Id when old-Id is renamed to fetch. (checkID function in fetch.js)
After the fix, the plugin add is changed as follows:
{code}
[CB-10314] cordova plugin
cordova-plugin-whitelist 1.2.1 "Whitelist"
org.apache.cordova.device 0.3.0 "Device"
[CB-10314] cordova plugin add org.apache.cordova.device
Plugin "org.apache.cordova.device" already installed on ios.
{code}
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bso-intel/cordova-lib CB-10314
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/396.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 #396
----
commit e88e2a1050a770e594a8ac8e6b179836ca819323
Author: Byoungro So <by...@intel.com>
Date: 2016-02-21T05:48:13Z
CB-10314 avoid fetching plugins when oldId is already fetched
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54223240
--- Diff: cordova-lib/src/plugman/install.js ---
@@ -79,21 +79,36 @@ module.exports = function installPlugin(platform, project_dir, id, plugins_dir,
}
var current_stack = new action_stack();
-
// Split @Version from the plugin id if it exists.
var splitVersion = id.split('@');
//Check if a mapping exists for the plugin id
//if it does, convert id to new name id
- var newId = pluginMapper[splitVersion[0]];
+ var newId = pluginMapper.oldToNew[splitVersion[0]];
if(newId) {
- events.emit('warn', 'Notice: ' + id + ' has been automatically converted to ' + newId + ' and fetched from npm. This is due to our old plugins registry shutting down.');
if(splitVersion[1]) {
id = newId +'@'+splitVersion[1];
} else {
id = newId;
}
- }
- return possiblyFetch(id, plugins_dir, options)
+ }
+ var P;
+ var plugin_dir = path.join(plugins_dir, splitVersion[0]);
+ // if the plugin has already been fetched, use it.
+ if (fs.existsSync(plugin_dir)) {
+ P = Q(plugin_dir);
+ } else {
+ var alias = pluginMapper.newToOld[splitVersion[0]] || newId;
+ // if the plugin alias has already been fetched, use it.
+ if (alias && fs.existsSync(path.join(plugins_dir, alias))) {
+ P = Q(path.join(plugins_dir, alias));
--- End diff --
Maybe we should show a message when we're going to use already fetched plugin with old id despite that user tries to install the one with new id?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54328707
--- Diff: cordova-lib/src/plugman/install.js ---
@@ -79,21 +79,36 @@ module.exports = function installPlugin(platform, project_dir, id, plugins_dir,
}
var current_stack = new action_stack();
-
// Split @Version from the plugin id if it exists.
var splitVersion = id.split('@');
//Check if a mapping exists for the plugin id
//if it does, convert id to new name id
- var newId = pluginMapper[splitVersion[0]];
+ var newId = pluginMapper.oldToNew[splitVersion[0]];
if(newId) {
- events.emit('warn', 'Notice: ' + id + ' has been automatically converted to ' + newId + ' and fetched from npm. This is due to our old plugins registry shutting down.');
if(splitVersion[1]) {
id = newId +'@'+splitVersion[1];
} else {
id = newId;
}
- }
- return possiblyFetch(id, plugins_dir, options)
+ }
+ var P;
--- End diff --
Yes, that's a good idea.
I revised the code as you suggested.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-192624166
Sorry for delay, @bso-intel. Merging
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-189612511
@bso-intel, thanks. This looks good apart from the logic duplication in `plugman/install` and `plugman/fetch`. However, this looks like is out of the scope of this PR and could be addressed later.
Do you want me to merge this?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-189578539
Hi Vladimir,
I updated the code to accommodate your comments.
Thanks.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54331174
--- Diff: cordova-lib/src/plugman/fetch.js ---
@@ -135,13 +135,32 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
var splitVersion = plugin_src.split('@');
var newID = pluginMapperotn[splitVersion[0]];
if(newID) {
- events.emit('warn', 'Notice: ' + splitVersion[0] + ' has been automatically converted to ' + newID + ' to be fetched from npm. This is due to our old plugins registry shutting down.');
plugin_src = newID;
if (splitVersion[1]) {
plugin_src += '@'+splitVersion[1];
}
- }
- return registry.fetch([plugin_src])
+ }
+ var P, skipCopyingPlugin;
+ plugin_dir = path.join(plugins_dir, splitVersion[0]);
--- End diff --
> In fact, cordova-plugin.js invokes plugman-fetch first and then plugman-install.
This is probably excess and could be reworked to just call `plugman/install` as it calls `tryFetch` anyway.
> because plugman-install can be invoked directly from plugin CLI
I guess you meant `plugman/fetch`, right? This might be a problem, but since this is user command, could we shift the responsibility for this to the user?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-192567709
@vladimir-kotikov ,
Just a friendly reminder. This is still not merged.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54224163
--- Diff: cordova-lib/src/plugman/fetch.js ---
@@ -135,13 +135,32 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
var splitVersion = plugin_src.split('@');
var newID = pluginMapperotn[splitVersion[0]];
if(newID) {
- events.emit('warn', 'Notice: ' + splitVersion[0] + ' has been automatically converted to ' + newID + ' to be fetched from npm. This is due to our old plugins registry shutting down.');
plugin_src = newID;
if (splitVersion[1]) {
plugin_src += '@'+splitVersion[1];
}
- }
- return registry.fetch([plugin_src])
+ }
+ var P, skipCopyingPlugin;
+ plugin_dir = path.join(plugins_dir, splitVersion[0]);
--- End diff --
It looks like these checks were already done in `plugman/install.js`. If we get here then neither `plugins_dir/<oldId>` nor `plugins_dir/<newId>`shouldn't exist, or i'm missing something?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54334968
--- Diff: cordova-lib/src/plugman/fetch.js ---
@@ -135,13 +135,32 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
var splitVersion = plugin_src.split('@');
var newID = pluginMapperotn[splitVersion[0]];
if(newID) {
- events.emit('warn', 'Notice: ' + splitVersion[0] + ' has been automatically converted to ' + newID + ' to be fetched from npm. This is due to our old plugins registry shutting down.');
plugin_src = newID;
if (splitVersion[1]) {
plugin_src += '@'+splitVersion[1];
}
- }
- return registry.fetch([plugin_src])
+ }
+ var P, skipCopyingPlugin;
+ plugin_dir = path.join(plugins_dir, splitVersion[0]);
--- End diff --
I agree.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-192706090
Thank you for merging, @vladimir-kotikov
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54328698
--- Diff: cordova-lib/src/plugman/fetch.js ---
@@ -135,13 +135,32 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
var splitVersion = plugin_src.split('@');
var newID = pluginMapperotn[splitVersion[0]];
if(newID) {
- events.emit('warn', 'Notice: ' + splitVersion[0] + ' has been automatically converted to ' + newID + ' to be fetched from npm. This is due to our old plugins registry shutting down.');
plugin_src = newID;
if (splitVersion[1]) {
plugin_src += '@'+splitVersion[1];
}
- }
- return registry.fetch([plugin_src])
+ }
+ var P, skipCopyingPlugin;
+ plugin_dir = path.join(plugins_dir, splitVersion[0]);
--- End diff --
In fact, cordova-plugin.js invokes plugman-fetch first and then plugman-install.
So, we need to check existence of the plugin here.
The same checking in plugman-install is still needed because plugman-install can be invoked directly from plugin CLI.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/396
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-189672357
Yes, please merge this for me.
I don't have committer privilege, which I wish to get. :)
Thanks, Vladimir.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54222593
--- Diff: cordova-lib/src/plugman/install.js ---
@@ -79,21 +79,36 @@ module.exports = function installPlugin(platform, project_dir, id, plugins_dir,
}
var current_stack = new action_stack();
-
// Split @Version from the plugin id if it exists.
var splitVersion = id.split('@');
//Check if a mapping exists for the plugin id
//if it does, convert id to new name id
- var newId = pluginMapper[splitVersion[0]];
+ var newId = pluginMapper.oldToNew[splitVersion[0]];
if(newId) {
- events.emit('warn', 'Notice: ' + id + ' has been automatically converted to ' + newId + ' and fetched from npm. This is due to our old plugins registry shutting down.');
if(splitVersion[1]) {
id = newId +'@'+splitVersion[1];
} else {
id = newId;
}
- }
- return possiblyFetch(id, plugins_dir, options)
+ }
+ var P;
--- End diff --
I'd move the chunk below to `possiblyFetch` function. This way you could just `return Q(plugin_dir)` instead of using intermediate variable. Also `possiblyFetch` already contains logic to check existence of plugin directory.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/396#discussion_r54328731
--- Diff: cordova-lib/src/plugman/install.js ---
@@ -79,21 +79,36 @@ module.exports = function installPlugin(platform, project_dir, id, plugins_dir,
}
var current_stack = new action_stack();
-
// Split @Version from the plugin id if it exists.
var splitVersion = id.split('@');
//Check if a mapping exists for the plugin id
//if it does, convert id to new name id
- var newId = pluginMapper[splitVersion[0]];
+ var newId = pluginMapper.oldToNew[splitVersion[0]];
if(newId) {
- events.emit('warn', 'Notice: ' + id + ' has been automatically converted to ' + newId + ' and fetched from npm. This is due to our old plugins registry shutting down.');
if(splitVersion[1]) {
id = newId +'@'+splitVersion[1];
} else {
id = newId;
}
- }
- return possiblyFetch(id, plugins_dir, options)
+ }
+ var P;
+ var plugin_dir = path.join(plugins_dir, splitVersion[0]);
+ // if the plugin has already been fetched, use it.
+ if (fs.existsSync(plugin_dir)) {
+ P = Q(plugin_dir);
+ } else {
+ var alias = pluginMapper.newToOld[splitVersion[0]] || newId;
+ // if the plugin alias has already been fetched, use it.
+ if (alias && fs.existsSync(path.join(plugins_dir, alias))) {
+ P = Q(path.join(plugins_dir, alias));
--- End diff --
I actually added a warning message in the fetch.js file.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-10314 avoid fetching plugins when old...
Posted by bso-intel <gi...@git.apache.org>.
Github user bso-intel commented on the pull request:
https://github.com/apache/cordova-lib/pull/396#issuecomment-189123404
@vladimir-kotikov
Could you review this?
Thanks.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org