You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by omefire <gi...@git.apache.org> on 2015/07/21 23:53:36 UTC
[GitHub] cordova-lib pull request: CB-9278: BugFix: Restoring multiple plat...
GitHub user omefire opened a pull request:
https://github.com/apache/cordova-lib/pull/266
CB-9278: BugFix: Restoring multiple platforms fails
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-9278
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/266.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 #266
----
commit 853f4f3d469238e3c9bb2a5b231b30878fc2fccf
Author: Omar Mefire <om...@gmail.com>
Date: 2015-07-21T08:25:59Z
CB-9278: BugFix: Restoring multiple platforms fails
commit 7105f7146c394df71616668df7a68935eb252ef5
Author: Omar Mefire <om...@gmail.com>
Date: 2015-07-21T08:30:36Z
CB-9278: Remove unused variable
----
---
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-9278: BugFix: Restoring multiple plat...
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/266#discussion_r35200762
--- Diff: cordova-lib/src/util/promise-util.js ---
@@ -33,4 +33,21 @@ function Q_chainmap(args, func) {
});
}
+// Behaves similar to Q_chainmap but gracefully handles failures.
+// When a promise in the chain is rejected, it will call the failureCallback and then continue the processing, instead of stopping
--- End diff --
Nit: The line is twice as longer than previous one. It might look better if split into two lines.
Another nit: Since this method is 'public', the comment might be rewritten using JSDoc notation.
---
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-9278: BugFix: Restoring multiple plat...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126643184
Also tried on OS X with two platforms, and it worked. +1 for adding a test case for 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-9278: BugFix: Restoring multiple plat...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-123661743
LGTM. I would also avoid unnecessary whitespace and formatting changes.
---
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-9278: BugFix: Restoring multiple plat...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-123498882
@nikhilkh, log level updated.
---
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-9278: BugFix: Restoring multiple plat...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126738584
@TimBarham , I've explored that possibility before landing with the current solution.
I think messing around with a queue-based solution or the event loop is going to take more time and more testing.
My goal is to have this issue fixed and checked in ASAP since it impacts such a basic scenario.
I can revisit making the change to npmhelper in the future.
---
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-9278: BugFix: Restoring multiple plat...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/266#discussion_r35160288
--- Diff: cordova-lib/src/cordova/restore-util.js ---
@@ -56,24 +57,23 @@ function installPlatformsFromConfigXML(platforms) {
if (!targets || !targets.length) {
return Q.all('No platforms are listed in config.xml to restore');
}
- // Run platform add for all the platforms seperately
+
+
+ // Run `platform add` for all the platforms separately
// so that failure on one does not affect the other.
- var promises = targets.map(function (target) {
+
+ // CB-9278 : Run `platform add` serially, one platform after another
+ // Otherwise, we get a bug where the following line: https://github.com/apache/cordova-lib/blob/0b0dee5e403c2c6d4e7262b963babb9f532e7d27/cordova-lib/src/util/npm-helper.js#L39
+ // gets executed more simultaneously by each platform and leads to an exception being thrown
+ return promiseutil.Q_chainmap_graceful(targets, function(target) {
if (target) {
events.emit('log', 'Restoring platform ' + target + ' referenced on config.xml');
return cordova.raw.platform('add', target);
}
return Q();
+ }, function(err) {
+ events.emit('log', err);
--- End diff --
Should these be logged as 'error' instead of 'log'
---
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-9278: BugFix: Restoring multiple plat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/266
---
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-9278: BugFix: Restoring multiple plat...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-123493951
One small comment. LGTM otherwise.
---
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-9278: BugFix: Restoring multiple plat...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126516900
@nikhilkh , it's ready to be merged in.
---
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-9278: BugFix: Restoring multiple plat...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126670748
Made this comment offline, but decided it should be included here too... I don't think this is the right fix - I think a more general approach would serve use better in the long run. There may be other scenarios (now or in the future) where we end up calling into `npm-helper` with multiple `npm` actions that aren't triggered sequentially. I think it would be better/safer to update `npm-helper` to handle that properly, rather than having to modify existing cases (or remember to always do it sequentially in the future).
Since `npm-helper`'s `loadWithSettingsThenRestore()` method ensures we always restore (even if something fails), if subsequent calls come in before we've restored from a previous call (which is what currently triggers the error), we can queue up those subsequent calls and process each them (and hence fulfill the corresponding promise) only once we've restored for all the calls that preceded it.
This shouldn't be too hard to code up, and provides us with a more robust solution at the root of the problem.
---
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-9278: BugFix: Restoring multiple plat...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126519728
Thinking about it - wonder if you can add test case for this to prevent future regression. It's really a basic use case here.
---
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-9278: BugFix: Restoring multiple plat...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126740006
@TimBarham , I've explored that possibility before landing with the current solution.
I think messing around with a queue-based solution or the event loop is going to take more time and more testing.
My goal is to have this issue fixed and checked in ASAP since it impacts such a basic scenario.
I can revisit making the change to npmhelper in the future. we can even go ahead and create another JIRA bug to track that refactoring, but I don't think it should stop us from moving forward with the fix to this issue.
---
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-9278: BugFix: Restoring multiple plat...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/266#issuecomment-126516413
@omefire - Is this ready to be merged? It will be good to get this merged before the next cordova-lib release.
---
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-9278: BugFix: Restoring multiple plat...
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/266#discussion_r35200567
--- Diff: cordova-lib/src/cordova/restore-util.js ---
@@ -56,24 +57,23 @@ function installPlatformsFromConfigXML(platforms) {
if (!targets || !targets.length) {
return Q.all('No platforms are listed in config.xml to restore');
}
- // Run platform add for all the platforms seperately
+
+
+ // Run `platform add` for all the platforms separately
// so that failure on one does not affect the other.
- var promises = targets.map(function (target) {
+
+ // CB-9278 : Run `platform add` serially, one platform after another
+ // Otherwise, we get a bug where the following line: https://github.com/apache/cordova-lib/blob/0b0dee5e403c2c6d4e7262b963babb9f532e7d27/cordova-lib/src/util/npm-helper.js#L39
+ // gets executed simultaneously by each platform and leads to an exception being thrown
+ return promiseutil.Q_chainmap_graceful(targets, function(target) {
if (target) {
--- End diff --
nit: I wonder if `target` can be falsy? Probably this condition is not required.
---
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