[Truffle] Improved exec #4386
[Truffle] Improved exec #4386
Conversation
|
@nirvdrum I think there are some spec failures on Mac. Here is my console output: |
|
Hmm . . . I'll have to get access to a Mac to see what's going on. Thanks for the heads up. |
|
@bjfish Oops. When I added my comments, I accidentally deleted a line of legitimate code. Good thing I didn't use |
| pgroup = options[:pgroup] | ||
| if pgroup | ||
| Truffle::POSIX.setpgid(0, pgroup) | ||
| end |
There was a problem hiding this comment.
Style note: I like assignment in a if like if pgroup = options[:pgroup], but do as you prefer.
There was a problem hiding this comment.
I can adapt. I've just been burned too many times by = vs == in other environments. I think Java gets this right.
There was a problem hiding this comment.
Somehow I adapted and don't recall having an unintentional = in a if for ages. I think it's not the worse mistake and relatively easy to catch.
| def spawn_setup | ||
| require 'fcntl' | ||
|
|
||
| env = options&.delete(:unsetenv_others) ? {} : ENV.to_hash |
There was a problem hiding this comment.
I think options should always be a Hash. Unfortunately it seems the constructor doesn't do that, even though its documentation says it does:
# Assigns @environment, @command, @argv, @redirects, @options. All
# elements are guaranteed to be non-nil. When no env or options are
# given, empty hashes are returned.
Could you fix the constructor?
| @@ -109,6 +110,7 @@ def initialize(env_or_cmd, *args) | |||
|
|
|||
| @command = command | |||
| @argv = argv | |||
| @env_array = [] | |||
There was a problem hiding this comment.
I would not initialize it, so it remains nil until it's properly set and not empty.
There was a problem hiding this comment.
Thanks. This was leftover from when I used to bail early if @options wasn't defined. Setting that in the constructor fixes everything.
| if b == 0x00 | ||
| raise ArgumentError.new("string contains null byte") | ||
| end | ||
| end |
There was a problem hiding this comment.
Thanks. I didn't know about that one.
| command, args = '/bin/sh', ['/bin/sh', '-c', command] | ||
| else | ||
| raise Errno::ENOENT.new("No such file or directory - #{command}") | ||
| end |
There was a problem hiding this comment.
Can't we let the shell do the lookup in $PATH in both cases?
There was a problem hiding this comment.
Do you mean by running /usr/bin/env? Otherwise, in one of these cases we're not supposed to run in the shell at all. In the other case, we'd end up with a different error than MRI (and the specs check for that).
There was a problem hiding this comment.
I meant by just giving the command to /bin/sh. But indeed if the error must be different than the shell error it's better this way. We need the search for the other case anyway.
No description provided.