Skip to content

Changed argument handling in EM.system introduces a breaking change #413

Closed
ohookins opened this Issue Mar 8, 2013 · 7 comments

2 participants

@ohookins
ohookins commented Mar 8, 2013

#322 which was released as part of 1.0.2 today seems to introduce a breaking change. I don't have more information right now as I'm trying to rescue a bunch of production machines which are failing.

Will update later with more information if needed.

cc @tmm1

@tmm1
tmm1 commented Mar 8, 2013

Sorry :cry:

Let me know what the issue is.

@ohookins
ohookins commented Mar 8, 2013

So it appears that previously, you could supply a string as a command, e.g. EM.system("which foo") and it would just work. Ideally the command would be supplied separately from arguments but due to the command and arguments being merged together in lib/em/processes.rb anyway, this did not make any difference to the invocation of Shellwords::shellwords( cmd ).

Now that the command and arguments are explicitly split, the case statement here will never see a String anyway, and the first element of the array is assumed to be the command name. Unfortunately, if a string was supplied, the entire string is taken to be the command name e.g. which foo, which is not a command, thus the EM.system call fails.

@tmm1
tmm1 commented Mar 8, 2013

Ah, that makes sense. Does this fix the issue?

diff --git a/lib/em/processes.rb b/lib/em/processes.rb
index db95982..4bbc14f 100644
--- a/lib/em/processes.rb
+++ b/lib/em/processes.rb
@@ -114,7 +114,7 @@ module EventMachine
     init = args.pop if args.last.is_a? Proc

     # merge remaining arguments into the command
-    cmd = [cmd, *args]
+    cmd = [cmd, *args] if args.any?

     EM.get_subprocess_pid(EM.popen(cmd, SystemCmd, cb) do |c|
       init[c] if init
@ohookins
ohookins commented Mar 8, 2013

Yes, that fixes the problem. Worth adding a unit test? ;)

@tmm1 tmm1 added a commit that closed this issue Mar 8, 2013
@tmm1 tmm1 fix EM.system (cc #322) (closes #413) 16f6d94
@tmm1 tmm1 closed this in 16f6d94 Mar 8, 2013
@ohookins
ohookins commented Mar 8, 2013

Do you think you'll make a new release for this regression fix?
Or rather, when do you think you'll release it?

@tmm1
tmm1 commented Mar 8, 2013

Released v1.0.3

@ohookins
ohookins commented Mar 8, 2013

Awesome! Thanks for the quick turnaround :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.