Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix supervisor to pass RUBYOPT to worker process #1066

Merged
merged 2 commits into from
Jun 28, 2016
Merged

Fix supervisor to pass RUBYOPT to worker process #1066

merged 2 commits into from
Jun 28, 2016

Conversation

naritta
Copy link
Member

@naritta naritta commented Jun 23, 2016 •
edited
Loading

Fix #1042

I did't use shellescape in unix for setting various values directly.

@naritta
Copy link
Member Author

naritta commented Jun 23, 2016 •
edited
Loading

MEMO:
In unix, we can set RUBYOPT in either way like:
RUBYOPT="-r bundler/setup" ./bin/fluentd -c fluent.conf
ruby -r bundler/setup ./bin/fluentd -c fluent.conf

In windows, we can't specify RUBYOPT directly in command line, then we have to set like:
ruby -r bundler/setup ./bin/fluentd -c fluent.conf

@naritta naritta mentioned this pull request Jun 23, 2016
Closed
@repeatedly repeatedly mentioned this pull request Jun 26, 2016
Merged
repeatedly
repeatedly reviewed Jun 26, 2016
View reviewed changes
lib/fluent/supervisor.rb
if Fluent.windows?
fluentd_spawn_cmd = ServerEngine.ruby_bin_path + " -Eascii-8bit:ascii-8bit "
fluentd_spawn_cmd << ' "' + rubyopt.gsub('"', '""') + '" ' if rubyopt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nurse Is this gsub safe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not safe. Maybe it should use Shellwords.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naritta Could you fix?

Copy link
Member Author

@naritta naritta Jun 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shellwords didn't work on windows, then I found this
http://stackoverflow.com/questions/5636601/is-there-an-equivalent-to-the-ruby-shellwords-module-for-the-windows-shell

If I'm misunderstanding, please tell me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sure. It sounds good.

Copy link
Member

@tagomoris tagomoris Jun 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot understand the reason why this code is like that.
Please add code comment to explain it.

@repeatedly
Copy link
Member

If there is no concern, I will merge this patch after added the comment.

@repeatedly repeatedly merged commit 9a7fd33 into master Jun 28, 2016
@repeatedly
Copy link
Member

Thansk!

@naritta
Copy link
Member Author

naritta commented Jun 28, 2016

Thank you very much for reviewing.

@nurse nurse deleted the pass-rubyopt branch June 28, 2016 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@naritta @repeatedly @nurse @tagomoris