Do not modify global Specification.dirs during installation. #442

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@voxik
Contributor
voxik commented Feb 6, 2013

While gems are installed into --install-dir just fine even without
modifications of Specification.dirs, change in it makes inaccessible
gems already present on the system.

@voxik voxik Do not modify global Specification.dirs during installation.
While gems are installed into --install-dir just fine even without
modifications of Specification.dirs, change in it makes inaccessible
gems already present on the system.
95ee154
@drbrain drbrain was assigned Feb 8, 2013
@luislavena
Member

@drbrain can you review?

@drbrain
Member
drbrain commented Feb 8, 2013

I am reviewing it now.

@drbrain
Member
drbrain commented Feb 8, 2013

The current behavior of gem install -i ~/tmp/gems foo is to install all dependencies of foo into ~/tmp/gems. It is different from GEM_HOME=~/tmp/gems gem install foo which considerers Gem.path when resolving dependencies.

Since this behavior is intentional I must reject this pull request.

While the modified block of code has the useless @gem_home variable, setting the specification directories must remain to maintain the behavior of -i.

Additionally, a test is lacking for this behavior of -i, I will add one.

Perhaps Gem::Specification.dirs = options[:install_dir] can be moved to the install command in a future version. It is too late to make such a change for rubygems 2.0.0/ruby 2.0.0, though.

@drbrain drbrain added a commit that referenced this pull request Feb 8, 2013
@drbrain drbrain Clean up --install-dir handling
While reviewing pull request #442 an unused instance variable @gem_home
was discovered.

The unused instance variable was removed and a slight refactoring was
performed.
7d9de49
@drbrain drbrain added a commit that closed this pull request Feb 8, 2013
@drbrain drbrain Properly test --install-dir behavior
The --install-dir option for gem install is supposed to create a
stand-alone installation of the listed gems.  It is supposed to ignore
gems already installed in the gem home or gem path.

The test for this feature did not test installing dependencies into the
chosen install directory.

These tests have been added.

Closes #442
86826aa
@drbrain drbrain closed this in 86826aa Feb 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment