Ignore default_path test cases for Ruby with custom configuration

[voxik]

RubyGems provides means to override their defaults, such as Gem.default_path method. With such configuration, the following errors might be observed:

    1) Failure:
  TestGem#test_default_path_vendor_dir [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:612]:
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/gemhome",
   "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/vendor/gems/3.3.0+0"]> expected but was
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/gemhome",
   "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-qs43ch/vendor/gems/3.3.0+0",
   "/usr/local/lib/ruby/bundled_gems/3.3.0+0",
   "/usr/local/lib/ruby/default_gems/3.3.0+0"]>.
  make: *** [uncommon.mk:943: yes-test-all] Error 4

    2) Failure:
  TestGem#test_default_path_missing_vendor [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:592]:
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-c41yb7/gemhome"]> expected but was
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-c41yb7/gemhome",
   "/usr/local/lib/ruby/bundled_gems/3.3.0+0",
   "/usr/local/lib/ruby/default_gems/3.3.0+0"]>.

    3) Failure:
  TestGem#test_default_path_user_home [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:600]:
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/userhome/.local/share/gem/ruby/3.3.0+0",
   "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/gemhome"]> expected but was
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/userhome/.local/share/gem/ruby/3.3.0+0",
   "/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-5mm6uc/gemhome",
   "/usr/local/lib/ruby/bundled_gems/3.3.0+0",
   "/usr/local/lib/ruby/default_gems/3.3.0+0"]>.

    4) Failure:
  TestGem#test_default_path [/home/runner/work/ruby/ruby/src/test/rubygems/test_gem.rb:582]:
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-kan9o5/gemhome"]> expected but was
  <["/home/runner/work/ruby/ruby/build/tmp/test_rubygems_20231110-59661-kan9o5/gemhome",
   "/usr/local/lib/ruby/bundled_gems/3.3.0+0",
   "/usr/local/lib/ruby/default_gems/3.3.0+0"]>.

Of course the Gem.default_path could be stubbed, but testing stub should not be the point.

Therefore, just detect if Gem.default_path was customized and ignore the test cases.

This is going to address the last 4 test failures referred in #7119 / ruby/ruby#8761 and I can't see other way around 🫤

[voxik]

Just FTR I have included the same patch in ruby/ruby#8761 where it can be seen that it helps to make the test suite green.

However, I have realized that should the ruby/ruby#8761 be accepted, the modified configuration will eventually become default and will be reused for RubyGems test suite resulting in these test cases being always skipped ...

[martinemde]

It seems like the solution to this paradox of permanently skipped tests is to make bundled_gems a built-in feature of rubygems instead of an override.

As an aside, the name "bundled gems" seems confusing since it implies connection with bundler, when that is not the intention. "Packaged gems"? 🤷‍♂️ I'll reply about that to the ruby feature. This name ship sailed a long time ago, but maybe we can avoid naming a directory with this name.

Edit: to elaborate on the test situation, I think the test isn't so much testing a default as it is testing how the path is composed from other configuration. If we could standardize how the path is composed no matter what the path was, we could test it just fine with or without a modified path.

I think what irks me about the current approach to supplying alternate gem paths is that it's perpetuating and formalizing a monkey patching system. It would be more maintainable to offer an API for this important function with the intention of formalizing all patches into APIs over time. You should be able to push something onto the gem path without overriding anything.

[voxik]

I agree just partially.

The thing is that the lib/rubygems/defaults.rb is IMHO just configuration file which happens to be written in Ruby (but of course we can discuss how much logic should there be).

Also contrary to "for this important function", I'd say this is quite unimportant function. And the test coverage apparently tests something which might not be used in practice (it certainly is not used on Fedora and the proposed Ruby patch also modifies the functionality in a way that the test is broken).

What is important to me is the concept of GEM_PATH. Is there user directory or path to default gems? That is not important and is fine if it is configurable. And TBH, the current implementation of Gem.default_path is not quite compelling to me. E.g. adding some directories to the path based on their existence on the filesystem, that is pretty unfortunate. That just makes valuable information about versatility of RubyGems more obscure. IOW I cannot create such directory and benefit from it unless I know I can create such directory (I was not aware of vendor_dir for ~10 years 😞 and I would consider my RubyGems knowledge above avarage).

I am still thinking about proposing something like:

$ git diff
diff --git a/lib/rubygems/defaults.rb b/lib/rubygems/defaults.rb
index 1fe6f36f3..ae55a6b7f 100644
--- a/lib/rubygems/defaults.rb
+++ b/lib/rubygems/defaults.rb
@@ -174,9 +174,9 @@ def self.path_separator
 
   def self.default_path
     path = []
-    path << user_dir if user_home && File.exist?(user_home)
+    path << user_dir if user_home
     path << default_dir
-    path << vendor_dir if vendor_dir && File.directory?(vendor_dir)
+    path << vendor_dir if vendor_dir
     path
   end

or even

$ git diff
diff --git a/lib/rubygems/defaults.rb b/lib/rubygems/defaults.rb
index 1fe6f36f3..5647b13fe 100644
--- a/lib/rubygems/defaults.rb
+++ b/lib/rubygems/defaults.rb
@@ -174,9 +174,9 @@ def self.path_separator
 
   def self.default_path
     path = []
-    path << user_dir if user_home && File.exist?(user_home)
+    path << user_dir
     path << default_dir
-    path << vendor_dir if vendor_dir && File.directory?(vendor_dir)
+    path << vendor_dir
     path
   end
 

[martinemde]

Is there a concern of including paths that may not exist?

For example, we don't allow gems to be written to world writable directories, for good reason. Do we need to check that none of the paths are world writeable before including them since that could pose a security threat?

Beyond that I don't see any reason not to include them. Should rubygems trim off non existent path at runtime and save the result so that each invocation uses only the paths that existed at the time?

I otherwise support the idea of adding all dirs by default if it solves the problem and creates no new problems.