Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Always try to keep original GEM_PATH #1920

Merged
merged 1 commit into from
Jun 7, 2012
Merged

Conversation

drogus
Copy link
Contributor

@drogus drogus commented May 16, 2012

This fixes a problem which occures when you want to run bundle command
inside code that already loads Gemfile. When you start a command with
bundle exec it sets RUBYOPT to
-I$PATH_TO_BUNDLER -r"bundler/setup". Because of that, when you run
the actual command it already requires bundler, which is fine, but since
GEM_PATH was cleared on the first run ORIGINAL_ENV will include
empty GEM_PATH. Now when you run Bundler.with_clean_env, you will
not have any bundler specific env variables, but you will also not have
original gem path, which will make it impossible to run any gem.

Alternative fix would be to assign ENV["GEM_PATH"] = ORIGINAL_ENV["GEM_PATH"]
before doing Kernel.exec in bundle exec.

@travisbot
Copy link

This pull request passes (merged 8c138efc into 2b4df23).

@drogus
Copy link
Contributor Author

drogus commented May 16, 2012

Update on alternative fix: it will not work correctly as bundle install is called inside rails new inside rails tests, which are run in bundle exec (I hope it's not deeper than that ;)), so we may loose GEM_PATH along the way.

@travisbot
Copy link

This pull request passes (merged 35e0144c into 2b4df23).

@indirect
Copy link
Member

This seems like a more general problem than just GEM_HOME. Perhaps you could serialize the original ENV, so it can be passed down through any number of nested bundle exec invocations? Something like:

ENV['BUNDLER_ORIG_ENV'] ||= YAML.dump(ENV)

def with_clean_env
  with_env(YAML.load(ENV['BUNDLER_ORIG_ENV']) do
    yield
  end
end

What do you think?

@drogus
Copy link
Contributor Author

drogus commented May 17, 2012

@indirect I think that using it just for GEM_PATH is fine. I would rather not want to try to guess which ENV user wants to use (original one or the current one) - if someone makes a change in ENV, they will expect the values to be there. GEM_PATH is different because bundler empties it here. Since we don't touch other environment things, I guess there is no need to try to preserve the original ones.

@indirect
Copy link
Member

Hmm... I guess I'm thinking of this more as a way to replace the entire ENV system that we have right now with something that preserves 100% of the original environment. I'm not sure if you've taken a look at it, but the way with_original_env works right now is kind of hacky (gsub, ick), and has had various bugs in the past with nested bundle exec. I'll try to look at this patch and my idea and see what works out as soon as I have some free time. :)

@drogus
Copy link
Contributor Author

drogus commented May 17, 2012

@indirect oh, I see. I misunderstood you a bit and now when I see that it replaces entire ENV anyway, it may be worth it. It just needs to work in such situation:

# we're in a script run with bundle exec

Bundle.with_clean_env do
  ENV['FOO'] = 'bar'
  `bundle exec ruby  -e "puts ENV['FOO']"` #=> 'bar'
end

Which is probably not very hard to achieve.

@indirect
Copy link
Member

Exactly. As long as with_clean_env swaps back in the original ENV before it calls yield, your example should work without any issues.

This fixes a problem which occures when you want to run `bundle` command
inside code that already loads `Gemfile`. When you start a command with
`bundle exec` it sets `RUBYOPT` to
`-I$PATH_TO_BUNDLER -r"bundler/setup"`. Because of that, when you run
the actual command it already requires bundler, which is fine, but since
`GEM_PATH` was cleared on the first run `ORIGINAL_ENV` will include
empty `GEM_PATH`. Now when you run `Bundler.with_clean_env`, you will
not have any bundler specific env variables, but you will also not have
original gem path, which will make it impossible to run any gem.

To fix this I try to always keep a reference to original gem path, so
it's not emptied along the way.
@drogus
Copy link
Contributor Author

drogus commented Jun 7, 2012

@indirect @hone @wycats since there is no progress on this one and the last fix needed to be reverted, maybe we could merge implementation from this pull request, which is basically the simplest thing that could possibly work? It fixes only the given issue, so the chances of any regressions are relatively small and then, having the test in place, you can refactor it later if someone comes up with better solution. What do you think about that?

@travisbot
Copy link

This pull request passes (merged f4dc6c5 into c666754).

@indirect
Copy link
Member

indirect commented Jun 7, 2012

@drogus yeah, seems reasonable to me.

indirect added a commit that referenced this pull request Jun 7, 2012
Always try to keep original GEM_PATH
@indirect indirect merged commit 9791f47 into rubygems:master Jun 7, 2012
@drogus
Copy link
Contributor Author

drogus commented Jun 25, 2012

@indirect thanks for merging. Is this ok to backport to 1-1-stable?

@indirect
Copy link
Member

Maybe... I'm worried that there are existing scripts in the wild that depend on this bug that might break if we changed it in a point release. :| Is it a big problem for you if this fix is only present in 1.2 and up?

@drogus
Copy link
Contributor Author

drogus commented Jun 25, 2012

@indirect no, it's not critical, the thing that I need it for in rails will be released in 4.0, so it can wait.

@derekatkins
Copy link

Any chance you can push a new release (1.2.2) with this change?

@drogus
Copy link
Contributor Author

drogus commented Oct 23, 2012

It should be released by now, I fixed a bug in rails that needed this a while ago and it seems to work.

@derekatkins
Copy link

I've got bundler 1.2.1 installed (the most recent release) and when I run:
bundle exec printenv
I have an empty GEM_PATH

@derekatkins
Copy link

Actually, I just verified that my install does have this patch, but it doesn't seem to work:

$ echo $GEM_PATH
/usr/local/rvm/gems/ruby-1.8.7-p370@sdm:/usr/local/rvm/gems/ruby-1.8.7-p370@global
$ bundle exec 'echo $GEM_PATH'

$

Am I missing something?

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

Successfully merging this pull request may close these issues.

6 participants