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

Deprecate --default option from install command #7588

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion Manifest.txt
Expand Up @@ -427,7 +427,6 @@ lib/rubygems/gemcutter_utilities/webauthn_listener.rb
lib/rubygems/gemcutter_utilities/webauthn_listener/response.rb
lib/rubygems/gemcutter_utilities/webauthn_poller.rb
lib/rubygems/gemspec_helpers.rb
lib/rubygems/install_default_message.rb
lib/rubygems/install_message.rb
lib/rubygems/install_update_options.rb
lib/rubygems/installer.rb
Expand Down
13 changes: 12 additions & 1 deletion bundler/spec/support/helpers.rb
Expand Up @@ -321,9 +321,20 @@ def install_gem(path, install_dir, default = false)
raise "OMG `#{path}` does not exist!" unless File.exist?(path)

args = "--no-document --ignore-dependencies --verbose --local --install-dir #{install_dir}"
args += " --default" if default

gem_command "install #{args} '#{path}'"

if default
gem = Pathname.new(path).basename.to_s.match(/(.*)\.gem/)[1]

# Revert Gem::Installer#write_spec and apply Gem::Installer#write_default_spec
FileUtils.mkdir_p File.join(install_dir, "specifications", "default")
File.rename File.join(install_dir, "specifications", gem + ".gemspec"),
File.join(install_dir, "specifications", "default", gem + ".gemspec")

# Revert Gem::Installer#write_cache_file
File.delete File.join(install_dir, "cache", gem + ".gem")
end
end

def with_built_bundler(version = nil, &block)
Expand Down
6 changes: 1 addition & 5 deletions lib/rubygems/commands/install_command.rb
Expand Up @@ -243,11 +243,7 @@ def install_gems # :nodoc:
# Loads post-install hooks

def load_hooks # :nodoc:
if options[:install_as_default]
require_relative "../install_default_message"
else
require_relative "../install_message"
end
require_relative "../install_message"
require_relative "../rdoc"
end

Expand Down
8 changes: 5 additions & 3 deletions lib/rubygems/commands/setup_command.rb
Expand Up @@ -400,16 +400,18 @@ def install_default_bundler_gem(bin_dir)
Dir.chdir("bundler") do
built_gem = Gem::Package.build(new_bundler_spec)
begin
Gem::Installer.at(
installer = Gem::Installer.at(
built_gem,
env_shebang: options[:env_shebang],
format_executable: options[:format_executable],
force: options[:force],
install_as_default: true,
bin_dir: bin_dir,
install_dir: default_dir,
wrappers: true
).install
)
installer.install
File.delete installer.spec_file
installer.write_default_spec
Copy link
Member

Choose a reason for hiding this comment

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

I think this will install a full copy of bundler in the standard $GEM_HOME. I don't think it's a problem per se, but it makes Bundler inconsistent with other default gems with executables provided with Ruby (for example, irb, which includes a placeholder gem directory with just the executable).

We could try to iterate on this but perhaps for now it's better to not use Gem::Installer at all here and create the files necessary (executables, and gemspec) manually, to keep the exact same folder structure gem update --system creates now.

ensure
FileUtils.rm_f built_gem
end
Expand Down
3 changes: 0 additions & 3 deletions lib/rubygems/dependency_installer.rb
Expand Up @@ -28,7 +28,6 @@ class Gem::DependencyInstaller
wrappers: true,
build_args: nil,
build_docs_in_background: false,
install_as_default: false,
}.freeze

##
Expand Down Expand Up @@ -87,7 +86,6 @@ def initialize(options = {})
@wrappers = options[:wrappers]
@build_args = options[:build_args]
@build_docs_in_background = options[:build_docs_in_background]
@install_as_default = options[:install_as_default]
@dir_mode = options[:dir_mode]
@data_mode = options[:data_mode]
@prog_mode = options[:prog_mode]
Expand Down Expand Up @@ -240,7 +238,6 @@ def install(dep_or_name, version = Gem::Requirement.default)
user_install: @user_install,
wrappers: @wrappers,
build_root: @build_root,
install_as_default: @install_as_default,
dir_mode: @dir_mode,
data_mode: @data_mode,
prog_mode: @prog_mode,
Expand Down
13 changes: 0 additions & 13 deletions lib/rubygems/install_default_message.rb

This file was deleted.

3 changes: 1 addition & 2 deletions lib/rubygems/install_update_options.rb
Expand Up @@ -158,10 +158,9 @@ def add_install_update_options
options[:without_groups].concat v.map(&:intern)
end

add_option(:"Install/Update", "--default",
add_option(:Deprecated, "--default",
"Add the gem's full specification to",
"specifications/default and extract only its bin") do |v,_o|
options[:install_as_default] = v
end
Copy link
Member

Choose a reason for hiding this comment

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

Should we use deprecate_option here, to also give a runtime deprecation message? The way it is right now, I think it only appears as deprecated in documentation.


add_option(:"Install/Update", "--explain",
Expand Down
39 changes: 15 additions & 24 deletions lib/rubygems/installer.rb
Expand Up @@ -308,11 +308,7 @@ def install
run_pre_install_hooks

# Set loaded_from to ensure extension_dir is correct
if @options[:install_as_default]
spec.loaded_from = default_spec_file
else
spec.loaded_from = spec_file
end
spec.loaded_from = spec_file

# Completely remove any previous gem files
FileUtils.rm_rf gem_dir
Expand All @@ -321,24 +317,17 @@ def install
dir_mode = options[:dir_mode]
FileUtils.mkdir_p gem_dir, mode: dir_mode && 0o755

if @options[:install_as_default]
extract_bin
write_default_spec
else
extract_files
extract_files

build_extensions
write_build_info_file
run_post_build_hooks
end
build_extensions
write_build_info_file
run_post_build_hooks

generate_bin
generate_plugins

unless @options[:install_as_default]
write_spec
write_cache_file
end
write_spec
write_cache_file

File.chmod(dir_mode, gem_dir) if dir_mode

Expand Down Expand Up @@ -443,12 +432,18 @@ def spec_file
File.join gem_home, "specifications", "#{spec.full_name}.gemspec"
end

def default_spec_dir
dir = File.join(gem_home, "specifications", "default")
FileUtils.mkdir_p dir
dir
end

##
# The location of the default spec file for default gems.
#

def default_spec_file
File.join gem_home, "specifications", "default", "#{spec.full_name}.gemspec"
File.join default_spec_dir, "#{spec.full_name}.gemspec"
end

##
Expand Down Expand Up @@ -916,11 +911,7 @@ def pre_install_checks

ensure_loadable_spec

if options[:install_as_default]
Gem.ensure_default_gem_subdirectories gem_home
else
Gem.ensure_gem_subdirectories gem_home
end
Gem.ensure_gem_subdirectories gem_home

return true if @force

Expand Down
42 changes: 40 additions & 2 deletions test/rubygems/helper.rb
Expand Up @@ -63,6 +63,44 @@ def self.specific_extra_args_hash=(value)
end
end

class Gem::Installer
# Copy from Gem::Installer#install with install_as_default option from old version
def install_default_gem
pre_install_checks

run_pre_install_hooks

spec.loaded_from = default_spec_file

FileUtils.rm_rf gem_dir
FileUtils.rm_rf spec.extension_dir

dir_mode = options[:dir_mode]
FileUtils.mkdir_p gem_dir, mode: dir_mode && 0o755

extract_bin
write_default_spec

generate_bin
generate_plugins

File.chmod(dir_mode, gem_dir) if dir_mode

say spec.post_install_message if options[:post_install_message] && !spec.post_install_message.nil?

Gem::Specification.add_spec(spec)

load_plugin

run_post_install_hooks

spec
rescue Errno::EACCES => e
# Permission denied - /path/to/foo
raise Gem::FilePermissionError, e.message.split(" - ").last
end
end

##
# RubyGemTestCase provides a variety of methods for testing rubygems and
# gem-related behavior in a sandbox. Through RubyGemTestCase you can install
Expand Down Expand Up @@ -799,8 +837,8 @@ def install_specs(*specs)

def install_default_gems(*specs)
specs.each do |spec|
installer = Gem::Installer.for_spec(spec, install_as_default: true)
installer.install
installer = Gem::Installer.for_spec(spec)
installer.install_default_gem
Gem.register_default_spec(spec)
end
end
Expand Down
113 changes: 0 additions & 113 deletions test/rubygems/test_gem_installer.rb
Expand Up @@ -2413,119 +2413,6 @@ def test_dir
assert_match %r{/gemhome/gems/a-2$}, installer.dir
end

def test_default_gem_loaded_from
spec = util_spec "a"
installer = Gem::Installer.for_spec spec, install_as_default: true
installer.install
assert_predicate spec, :default_gem?
end

def test_default_gem_without_wrappers
installer = setup_base_installer

FileUtils.rm_rf File.join(Gem.default_dir, "specifications")

installer.wrappers = false
installer.options[:install_as_default] = true
installer.gem_dir = @spec.gem_dir

use_ui @ui do
installer.install
end

assert_directory_exists File.join(@spec.gem_dir, "bin")
installed_exec = File.join @spec.gem_dir, "bin", "executable"
assert_path_exist installed_exec

assert_directory_exists File.join(Gem.default_dir, "specifications")
assert_directory_exists File.join(Gem.default_dir, "specifications", "default")

default_spec = eval File.read File.join(Gem.default_dir, "specifications", "default", "a-2.gemspec")
assert_equal Gem::Version.new("2"), default_spec.version
assert_equal ["bin/executable"], default_spec.files

assert_directory_exists util_inst_bindir

installed_exec = File.join util_inst_bindir, "executable"
assert_path_exist installed_exec

wrapper = File.read installed_exec

if symlink_supported?
refute_match(/generated by RubyGems/, wrapper)
else # when symlink not supported, it warns and fallbacks back to installing wrapper
assert_match(/Unable to use symlinks, installing wrapper/, @ui.error)
assert_match(/generated by RubyGems/, wrapper)
end
end

def test_default_gem_with_wrappers
installer = setup_base_installer

installer.wrappers = true
installer.options[:install_as_default] = true
installer.gem_dir = @spec.gem_dir

use_ui @ui do
installer.install
end

assert_directory_exists util_inst_bindir

installed_exec = File.join util_inst_bindir, "executable"
assert_path_exist installed_exec

wrapper = File.read installed_exec
assert_match(/generated by RubyGems/, wrapper)
end

def test_default_gem_with_exe_as_bindir
@spec = quick_gem "c" do |spec|
util_make_exec spec, "#!/usr/bin/ruby", "exe"
end

util_build_gem @spec

@spec.cache_file

installer = util_installer @spec, @gemhome

installer.options[:install_as_default] = true
installer.gem_dir = @spec.gem_dir

use_ui @ui do
installer.install
end

assert_directory_exists File.join(@spec.gem_dir, "exe")
installed_exec = File.join @spec.gem_dir, "exe", "executable"
assert_path_exist installed_exec

assert_directory_exists File.join(Gem.default_dir, "specifications")
assert_directory_exists File.join(Gem.default_dir, "specifications", "default")

default_spec = eval File.read File.join(Gem.default_dir, "specifications", "default", "c-2.gemspec")
assert_equal Gem::Version.new("2"), default_spec.version
assert_equal ["exe/executable"], default_spec.files
end

def test_default_gem_to_specific_install_dir
@gem = setup_base_gem
installer = util_installer @spec, "#{@gemhome}2"
installer.options[:install_as_default] = true

use_ui @ui do
installer.install
end

assert_directory_exists File.join("#{@gemhome}2", "specifications")
assert_directory_exists File.join("#{@gemhome}2", "specifications", "default")

default_spec = eval File.read File.join("#{@gemhome}2", "specifications", "default", "a-2.gemspec")
assert_equal Gem::Version.new("2"), default_spec.version
assert_equal ["bin/executable"], default_spec.files
end

def test_package_attribute
gem = quick_gem "c" do |spec|
util_make_exec spec, "#!/usr/bin/ruby", "exe"
Expand Down