Skip to content

Don't wrap non-ruby executables and explicit no-wrap requests. #4069

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

Open
iboB wants to merge 1 commit into
base: master
Choose a base branch
from
+55 −1
Open

Don't wrap non-ruby executables and explicit no-wrap requests. #4069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion lib/rubygems/installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,26 @@ def generate_windows_script(filename, bindir)
end
end

##
# Checks if +bin_path+ should be wrapped if we're generating wrappers
#
# If the first line of the file is a shebang check if it mentions ruby and
# return false if it doesn't (don't wrap non-ruby executable)
#
# If the first line is not a shebang, check if it contains the magic string:
# "rubygems: no-wrap" and return false if it does (don't wrap if no-wrap was
# requested)
#
# Otherwise wrap

def wrappable_executable?(bin_path)
File.open bin_path, 'r' do |f|
line1 = f.gets
break line1['ruby'] if line1.start_with?('#!') # if the line is a shebang check for ruby

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

The check for 'ruby' in the shebang line uses a simple substring search, which could produce false positives. For example, a shebang like '#!/usr/bin/groovy' would match because it contains 'ruby' as a substring. Consider using a more robust check such as matching word boundaries or checking for common ruby interpreter patterns like /ruby\d*$/ or /ruby\s/.

Suggested change
break line1['ruby'] if line1.start_with?('#!') # if the line is a shebang check for ruby
break line1[/\bruby\d*\b/] if line1.start_with?('#!') # if the line is a shebang check for ruby

Copilot uses AI. Check for mistakes.
!line1['rubygems: no-wrap'] # check for magic string
Comment on lines +492 to +493

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

The wrappable_executable? method does not handle the case where the file is empty or where f.gets returns nil. If line1 is nil, calling line1.start_with? or line1['ruby'] will raise a NoMethodError. You should add a check to return true (wrappable) if line1 is nil, which would preserve the existing behavior for empty files.

Suggested change
break line1['ruby'] if line1.start_with?('#!') # if the line is a shebang check for ruby
!line1['rubygems: no-wrap'] # check for magic string
break true if line1.nil? # empty file, preserve existing behavior: wrappable
if line1.start_with?('#!') # if the line is a shebang check for ruby
break !!line1['ruby']
end
# if not a shebang, check for magic string "rubygems: no-wrap"
break !line1['rubygems: no-wrap']

Copilot uses AI. Check for mistakes.

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

The logic for checking the magic string is inverted. When line1['rubygems: no-wrap'] finds the substring, it returns 'rubygems: no-wrap' (truthy), and !truthy becomes false, meaning "not wrappable" which is correct. However, when the substring is NOT found, it returns nil, and !nil becomes true, meaning "wrappable" which is also correct. While this works, using !line1.include?('rubygems: no-wrap') would be more readable and explicit about the intent.

Suggested change
!line1['rubygems: no-wrap'] # check for magic string
!line1.include?('rubygems: no-wrap') # check for magic string

Copilot uses AI. Check for mistakes.
Comment on lines +492 to +493

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

Using String#[] with a substring returns the substring if found, or nil if not found. However, the code is using this in a boolean context. When checking for 'ruby' in a shebang, if 'ruby' is NOT found, line1['ruby'] returns nil which is falsy, so it correctly returns false (not wrappable). But the logic seems backwards from the documentation comment. The comment says "return false if it doesn't [contain ruby]" but the code returns the result of line1['ruby'], which returns a truthy value (the substring) when ruby IS found. This actually works correctly but could be clearer. Consider using line1.include?('ruby') for better readability and intent.

Suggested change
break line1['ruby'] if line1.start_with?('#!') # if the line is a shebang check for ruby
!line1['rubygems: no-wrap'] # check for magic string
break line1.include?('ruby') if line1.start_with?('#!') # if the line is a shebang check for ruby
!line1.include?('rubygems: no-wrap') # check for magic string

Copilot uses AI. Check for mistakes.

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

The substring search for 'rubygems: no-wrap' is case-sensitive. If a developer writes 'Rubygems: no-wrap' or 'RUBYGEMS: NO-WRAP', it won't be recognized. Consider using a case-insensitive search or documenting the exact required format for the magic string.

Suggested change
!line1['rubygems: no-wrap'] # check for magic string
!line1.downcase['rubygems: no-wrap'] # check for magic string (case-insensitive)

Copilot uses AI. Check for mistakes.
end

@knu knu Jan 15, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using String#[] is not the best way to check if a string contains a substring, as it's not a boolean operation.
Also, the method should be aware that IO#gets may return nil if the file is empty.

You could use a regexp here.

# If the first line is a non-ruby shebang or contains a magic string, return false.
/\A#!(?!.*ruby)|rubygems: no-wrap/ !~ File.open(bin_path, &:gets)

end

def generate_bin # :nodoc:
return if spec.executables.nil? or spec.executables.empty?

Expand All @@ -499,7 +519,7 @@ def generate_bin # :nodoc:

check_executable_overwrite filename

if @wrappers
if @wrappers && wrappable_executable?(bin_path)
generate_bin_script filename, @bin_dir
else
generate_bin_symlink filename, @bin_dir
Expand Down
34 changes: 34 additions & 0 deletions test/rubygems/test_gem_installer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,40 @@ def test_generate_bin_script_wrappers
'real executable overwritten'
end

def test_no_wrap_non_ruby_executables
skip "Symlinks not supported or not enabled" unless symlink_supported?

installer = setup_base_installer

installer.wrappers = true
util_make_exec @spec, "#!/usr/bin/env bash"
installer.gem_dir = @spec.gem_dir

installer.generate_bin
assert_directory_exists util_inst_bindir
installed_exec = File.join util_inst_bindir, 'executable'
assert_equal true, File.symlink?(installed_exec)
assert_equal(File.join(@spec.gem_dir, 'bin', 'executable'),
File.readlink(installed_exec))
end
Comment on lines +527 to +542

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

The test does not verify that the test actually tests the intended behavior. The test creates a bash executable but doesn't verify that it would have been wrapped without this change. Consider adding an assertion that checks the original executable file still exists in the gem directory and hasn't been wrapped, or add a comment explaining why checking for a symlink is sufficient evidence that wrapping was skipped.

Copilot uses AI. Check for mistakes.

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

The test only covers bash executables. Consider adding test coverage for other common non-ruby shebangs such as '#!/usr/bin/env python', '#!/bin/sh', '#!/usr/bin/perl', etc. to ensure the feature works correctly for various script types.

Suggested change
def test_no_wrap_non_ruby_executables_env_python
skip "Symlinks not supported or not enabled" unless symlink_supported?
installer = setup_base_installer
installer.wrappers = true
util_make_exec @spec, "#!/usr/bin/env python"
installer.gem_dir = @spec.gem_dir
installer.generate_bin
assert_directory_exists util_inst_bindir
installed_exec = File.join util_inst_bindir, 'executable'
assert_equal true, File.symlink?(installed_exec)
assert_equal(File.join(@spec.gem_dir, 'bin', 'executable'),
File.readlink(installed_exec))
end
def test_no_wrap_non_ruby_executables_bin_sh
skip "Symlinks not supported or not enabled" unless symlink_supported?
installer = setup_base_installer
installer.wrappers = true
util_make_exec @spec, "#!/bin/sh"
installer.gem_dir = @spec.gem_dir
installer.generate_bin
assert_directory_exists util_inst_bindir
installed_exec = File.join util_inst_bindir, 'executable'
assert_equal true, File.symlink?(installed_exec)
assert_equal(File.join(@spec.gem_dir, 'bin', 'executable'),
File.readlink(installed_exec))
end
def test_no_wrap_non_ruby_executables_perl
skip "Symlinks not supported or not enabled" unless symlink_supported?
installer = setup_base_installer
installer.wrappers = true
util_make_exec @spec, "#!/usr/bin/perl"
installer.gem_dir = @spec.gem_dir
installer.generate_bin
assert_directory_exists util_inst_bindir
installed_exec = File.join util_inst_bindir, 'executable'
assert_equal true, File.symlink?(installed_exec)
assert_equal(File.join(@spec.gem_dir, 'bin', 'executable'),
File.readlink(installed_exec))
end

Copilot uses AI. Check for mistakes.
def test_no_wrap_explicit_rubygems_no_wrap
skip "Symlinks not supported or not enabled" unless symlink_supported?

installer = setup_base_installer

installer.wrappers = true
util_make_exec @spec, ":: rubygems: no-wrap" # a batch file would likely look like this
installer.gem_dir = @spec.gem_dir

installer.generate_bin
assert_directory_exists util_inst_bindir
installed_exec = File.join util_inst_bindir, 'executable'
assert_equal true, File.symlink?(installed_exec)
assert_equal(File.join(@spec.gem_dir, 'bin', 'executable'),
File.readlink(installed_exec))
end

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

There is no test coverage for the edge case where an executable file is completely empty. With the current implementation of wrappable_executable?, an empty file would cause line1 to be nil, which would cause a NoMethodError when calling start_with? on it. Consider adding a test for this edge case.

Suggested change
def test_no_wrap_empty_executable
skip "Symlinks not supported or not enabled" unless symlink_supported?
installer = setup_base_installer
installer.wrappers = true
util_make_exec @spec, ""
installer.gem_dir = @spec.gem_dir
installer.generate_bin
assert_directory_exists util_inst_bindir
installed_exec = File.join util_inst_bindir, 'executable'
assert_equal true, File.symlink?(installed_exec)
assert_equal(File.join(@spec.gem_dir, 'bin', 'executable'),
File.readlink(installed_exec))
end

Copilot uses AI. Check for mistakes.
def test_generate_bin_symlink
skip "Symlinks not supported or not enabled" unless symlink_supported?

Expand Down
Loading