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

[iboB]

This is still a draft. The functionality is in, but docs are not updated.

What was the end-user or developer problem that led to this PR?

This is the result of the discussion in #88

In short: apart from checking @wrappers the installer performs an additional check before deciding whether to wrap executables:

• Check if the first line is a shebang:
  • if it is, check if it contains the string 'ruby',
  • if it doesn't, assume this is not a ruby executable and don't wrap
• If it's not a shebang, check if it contains the magic string 'rubygems: no-wrap'
  • if it does, don't wrap as well. This will be the way to deploy windows batch files
  • if it doesn't contain the magic string, assume it's a shebang-less ruby executable and wrap away

This doesn't break any of the existing tests. I find it highly unlikely that any existing gem will be affected by this

Future gems will be able to provide shell scripts as non-wrapped executables, without requesting the users to explicitly use --no-wrappers when installing

Make sure the following tasks are checked

• Describe the problem / feature
• Write tests for features and bug fixes
• Write code to solve the problem
• Make sure you follow the current code style and write meaningful commit messages without tags

[iboB]

Er... I saw the failing tests on windows, but to be honest I can't see how my changes could've caused the fails

[knu]

Is this PR still a draft?

[knu]

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)

[iboB]

@knu it is still a draft. Waiting on @deivid-rodriguez on whether we should go on with this functionality. If yes, then I'll add some docs changes in the same PR. The conversation is in #88

Thanks for the review!

[Copilot]

Pull request overview

This pull request implements a feature to prevent RubyGems from wrapping non-ruby executables and executables that explicitly request not to be wrapped. The change addresses issue #88 where RubyGems was creating Ruby wrappers around shell scripts, batch files, and other non-Ruby executables, causing them to fail.

Changes:

• Added wrappable_executable? method to check if an executable should be wrapped based on shebang content or magic string
• Modified generate_bin to call wrappable_executable? before deciding whether to wrap or symlink
• Added tests for non-ruby executables and explicit no-wrap requests

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
lib/rubygems/installer.rb	Adds wrappable_executable? method and integrates it into generate_bin logic to conditionally wrap executables
test/rubygems/test_gem_installer.rb	Adds tests for non-ruby executables (bash) and explicit no-wrap magic string behavior

Comments suppressed due to low confidence (1)

lib/rubygems/installer.rb:526

• On Windows, when symlinks are not supported, generate_bin_symlink falls back to generate_bin_script (line 586 in the file). This means that non-ruby executables and files with the 'rubygems: no-wrap' magic string will still get wrapped on Windows, contradicting the feature's intent. Consider documenting this limitation or handling Windows batch files differently, perhaps by copying them directly instead of creating a wrapper.

      if @wrappers && wrappable_executable?(bin_path)
        generate_bin_script filename, @bin_dir
      else
        generate_bin_symlink filename, @bin_dir
      end

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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]

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]

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]

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]

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]

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]

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)