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.

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

Escape bundler environment when shelling out to fix #190 #191

Merged
merged 5 commits into from
Sep 3, 2015
Merged

Escape bundler environment when shelling out to fix #190 #191

merged 5 commits into from
Sep 3, 2015

Conversation

ksubrama
Copy link

No description provided.

# @see Kitchen::ShellOut#run_command
def run_command(cmd, options = {})
merged = {
:use_sudo => config[:use_sudo], :log_subject => name
}.merge(options)
super(cmd, merged)
if Object.const_defined?("Bundler")
Bundler.with_clean_env { super(cmd, merged) }
Copy link
Contributor

Choose a reason for hiding this comment

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

@ksubrama this actually isn't enough 😦

We need to reset RUBYLIB and RUBYOPT as well as GEM_PATH and GEM_HOME and GEM_ROOT. It's pretty sad.

Copy link
Author

Choose a reason for hiding this comment

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

I terribly misunderstood how bundler (and bundle exec works). Bundler.with_clean_env is utterly useless in the subprocess because by the time it has exec-ed the underlying script, the "pristine" environment is long gone. PATH has been mangled and GEM_* has been reset. And ORIGINAL_GEM_PATH is anything but original.

You're right... this is very very sad :(

Copy link
Contributor

Choose a reason for hiding this comment

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

@ksubrama do not feel bad. I have been misunderstood for like a billion years. It's super super sad 😦.

Basically you'll likely need to do something like launch with a non-Ruby process (we used Go for this in Vagrant) which doesn't mangle the env, copy all keys in the current env to a different key, and then iterates and restores them before shelling out.

@ksubrama
Copy link
Author

Fixing this properly with a wrapper seemed too annoying. I've currently changed it to simply blow away as many bad environment variables as possible in the hopes that it'll pick up the correct vagrant launch it with a good enough environment.

@sethvargo
Copy link
Contributor

LGTM

%w[BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH GEM_ROOT RUBY_LIB
RUBY_OPT _ORIGINAL_GEM_PATH].each do |var|
env[var] = nil
end
Copy link
Member

Choose a reason for hiding this comment

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

Might add a test to ensure these get stripped

@tknerr
Copy link
Contributor

tknerr commented Sep 1, 2015

Just stumbled upon (looks like a similar attempt to solve this. just for inspiration :-)):
https://github.com/berkshelf/vagrant-berkshelf/blob/master/lib/vagrant-berkshelf/helpers.rb#L149-L186

@ksubrama
Copy link
Author

ksubrama commented Sep 2, 2015

@tknerr Thanks! In went with a slightly more conservative approach for now just in case. I'll get more militant about the cleanup if this fails for some reason.
@mwrock Thanks for making me write tests. I now handle edge cases where PATH is overriden or is empty.

@mwrock
Copy link
Member

mwrock commented Sep 2, 2015

👍

@@ -0,0 +1,2 @@
--color
-fd
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference, I usually also add --require spec_helper.rb so that you don't have to have that require at the top of every spec file, but that may just be my preference. Don't need to change the PR

tyler-ball added a commit that referenced this pull request Sep 3, 2015
Escape bundler environment when shelling out to fix #190
@tyler-ball tyler-ball merged commit cf89394 into test-kitchen:master Sep 3, 2015
@fnichol
Copy link
Contributor

fnichol commented Sep 3, 2015

Excellent, beat me to the green button. Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants