Exit nonzero if not everything goes to plan #322

Merged
merged 1 commit into from Mar 17, 2013

Projects

None yet

4 participants

@richo
Contributor
richo commented Mar 15, 2013

I attempted to write a testcase, but my node fu is weak.. and realistically this seems selftesting, so either

  1. Please give feedback on how to write this test
  2. Don't merge the second commit
@sindresorhus
Member

Have you seen #320?

@richo
Contributor
richo commented Mar 16, 2013

Ah, no. I'll fix this PR to work with the multiple installs case.

@richo
Contributor
richo commented Mar 16, 2013

I fixed up the pull request, again feel free not to merge the second commit or give me some feedback on how to test this.

@satazor
Member
satazor commented Mar 17, 2013

@richo I already though about doing what you are doing in this PR. This will sort of resolve the issue temporarily, but the real issue remains because an user does never know when the install command actually ends (programmatically).

Having this said, I'm happy to accept this PR until we implement a proper fix for the issue.

@richo
Contributor
richo commented Mar 17, 2013

I updated the pull request, I'll have a look into testing it.

What do you mean it's impossible to tell when it finishes programmatically? Surely it's just a matter of calling wait(2) ?

@richo
Contributor
richo commented Mar 17, 2013

Done, sorry :)

@satazor
Member
satazor commented Mar 17, 2013

@richo when one uses bower programatically like so:

var bower = require('bower');

bower.commands.install(['jquery'])
  .on('error', function (err) { console.log('install failed with error', err); })
  .on('end', function () { console.log('install succeeded!'); });
  • Multiple error events will be fired.
  • data events will still be fired after an error
  • Spawned git processes will hang around for some time

Since the end event is used only on success, one does not know when the install command actually ended when it fails

@satazor satazor commented on an outdated diff Mar 17, 2013
@@ -31,6 +32,14 @@ bower.version = pkg.version;
if (options.version) return console.log(bower.version);
if (~cmdList.indexOf(command = options.argv.remain && options.argv.remain.shift())) bower.command = command;
+
+// Temporarory fix for #22 #320 #187
+var errstatusHandler = function() {
+ process.removeEvent('exit', errstatusHandler);
+ process.exit(errors.length > 0 ? 1 : 0);
+}
+process.on('exit', errstatus_handler);
@satazor
satazor Mar 17, 2013 Member

you forgot to rename the variable here.

@satazor satazor commented on an outdated diff Mar 17, 2013
@@ -31,6 +32,14 @@ bower.version = pkg.version;
if (options.version) return console.log(bower.version);
if (~cmdList.indexOf(command = options.argv.remain && options.argv.remain.shift())) bower.command = command;
+
+// Temporarory fix for #22 #320 #187
+var errstatusHandler = function() {
+ process.removeEvent('exit', errstatusHandler);
@satazor
satazor Mar 17, 2013 Member

Sorry I think I mislead you. It's removeListener. See: http://nodejs.org/api/events.html

@richo
Contributor
richo commented Mar 17, 2013

Oh right. While I agree that sucks, I still think it's good to deal with this case when a solution is possible.

Our CI process bundles assets for deployment- it is catastrophic if we can't test if the bundling process worked.

@richo
Contributor
richo commented Mar 17, 2013

Sorry, trying to work on too many things at once. I amended again, it should be right now.

@satazor
Member
satazor commented Mar 17, 2013

@richo If you're gonna implement the test using the spawn strategy, take a look at: http://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options

@richo
Contributor
richo commented Mar 17, 2013

Amended again for removeListener. Is the other way also valid? It didn't complain when I tested it :/

@satazor
Member
satazor commented Mar 17, 2013

There's no alias to removeEvent in https://github.com/joyent/node/blob/master/lib/events.js. Is the on exit handler actually running?

@richo
Contributor
richo commented Mar 17, 2013

Exit status was 1.. let me try again without the patch.

@richo
Contributor
richo commented Mar 17, 2013

Correct behaviour. I guess the error was just dropped silently :/

@satazor
Member
satazor commented Mar 17, 2013

Yea, makes sense since the process is nearly exiting.

@richo
Contributor
richo commented Mar 17, 2013

In any case, I think this should be good to merge?

@satazor
Member
satazor commented Mar 17, 2013

I've merged locally, and adding some tests.

@satazor satazor merged commit 3e422f2 into bower:master Mar 17, 2013

1 check passed

default The Travis build passed
Details
@satazor
Member
satazor commented Mar 17, 2013

Merged.

@satazor
Member
satazor commented Mar 17, 2013

Thanks @richo !

@richo
Contributor
richo commented Mar 17, 2013

No problem! Thankyou for bearing with me through a few iterations, and for writing some tests.

@antoine-richard

Hi,

Thanks for this temporary fix, it'll be helpful.
I guess this patch will ship in 0.9. Do you have any release window in mind ?

Thanks
Antoine

EDIT: Auto-answer: Shipped in 0.9.0 on 2013-04-25. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment