-
-
Notifications
You must be signed in to change notification settings - Fork 925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -109,6 +110,7 @@ def initialize(env_or_cmd, *args) | |||
|
|||
@command = command | |||
@argv = argv | |||
@env_array = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not initialize it, so it remains nil until it's properly set and not empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubinius::Type.check_null_safe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we let the shell do the lookup in $PATH in both cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.