[WIP] Run tests with `--disable-gems` #4440

voxik · 2021-03-08T19:13:59Z

This PR is related to discussion in #4429. It makes the test suite execute with --disable-gems option. This should help to reduce the initial footprint of loaded libraries and reduce the risk of inadvertently testing system RubyGems instead of upstream version.

voxik · 2021-03-08T19:24:29Z

This is one way to change the test suite. The other way would be to execute change the content of the default Rake task. That way, the test would be executed similarly for all platforms. Will try that ....

deivid-rodriguez · 2021-03-08T19:42:55Z

I'd rather not change the way rubygems runs tests in general. Most users of rubygems don't have RUBYOPT=--disable-gems, so I'd like to keeping testing that case.

Instead, I would add an extra matrix entry with RUBYOPT=--disable-gems and make sure it also passes.

voxik · 2021-03-08T19:47:05Z

This is the option I was talking about:

diff --git a/Rakefile b/Rakefile
index 8b1164559..9ca37b98f 100644
--- a/Rakefile
+++ b/Rakefile
@@ -27,11 +27,16 @@ task :git_hooks do
 end
 
 Rake::TestTask.new do |t|
+  ENV['RUBYOPT'] = '--disable-gems'
+
+  t.loader = :direct
+
   t.ruby_opts = %w[-w]
   t.ruby_opts << '-rdevkit' if RbConfig::CONFIG['host_os'].include?('mingw')
 
   t.libs << "test"
   t.libs << "bundler/lib"
+  t.libs << "."
 
   t.test_files = FileList['test/**/test_*.rb']
 end

And I'll explore how to extend the matrix as you suggested

deivid-rodriguez · 2021-03-08T20:08:47Z

Don't mind moving to the "direct" loader, but I'd rather massage the test file list so that it plays nice with adding "test" to the $LOAD_PATH rather than adding "." to the $LOAD_PATH as well.

So something like

 
+require "pathname"
+
 Rake::TestTask.new do |t|
+  t.loader = :direct
+
   t.ruby_opts = %w[-w]
   t.ruby_opts << '-rdevkit' if RbConfig::CONFIG['host_os'].include?('mingw')
 
   t.libs << "test"
   t.libs << "bundler/lib"
 
-  t.test_files = FileList['test/**/test_*.rb']
+  t.test_files = Dir.glob('test/**/test_*.rb').map {|p| Pathname.new(p).relative_path_from("test") }
 end

or maybe passing absolute paths directly?

hsbt · 2021-03-09T00:33:56Z

Objection. --disable-gems is only debugging feature for ruby-core, not distribution vendor and ruby users. so It should not be use on primary test of CI.

deivid-rodriguez · 2021-03-09T09:57:59Z

The idea is to not use it for primary testing of CI as per #4440 (comment). But I don't mind having green tests with --disable-gems and adding an extra CI matrix entry to test it, if that helps packagers with upgrading.

This makes the test suite execute with `--disable-gems` option. This should help to reduce the initial footprint of loaded libraries and reduce the risk of inadvertently testing system RubyGems instead of upstream version.

voxik · 2021-03-10T12:43:04Z

Here is another version of the proposal. I was trying to extend the test matrix, but I think this way it will be simpler. Unfortunately, the test on Ruby 3.0 are broken more then on the older Rubies, presumably by ruby/rake#357. I can see several options here:

Go with the :direct loader
Don't execute the test suite via Rake
Fix Rake to work with --disable-gems as it use to do
Drop --disable-gems from Ruby, because it starts to not be supported on many places
Forget about this issue

I completely disagree with @hsbt on --disable-gems being just debugging feature, so I won't pursue the 4th option.

hsbt · 2021-03-10T12:46:57Z

I completely disagree with @hsbt on --disable-gems being just debugging feature

It's not only my opinion, it's opinion of ruby-core team.

deivid-rodriguez · 2021-03-10T13:01:24Z

Either 1 or 5 work for me, I don't have a strong opinion on either. I think @hsbt and @simi feel more strongly against this that I feel in favour. If this is the case, please go ahead and close this PR.

deivid-rodriguez · 2021-03-10T13:23:22Z

I guess issues on ruby 3 should be fixed by ruby/rake#379.

deivid-rodriguez · 2021-03-10T14:14:10Z

By the way, even if we decide to move this forward, I don't see how this part of the commit message is relevant here:

This should help to reduce the initial footprint of loaded libraries and reduce the risk of inadvertently testing system RubyGems instead of upstream version.

Our tests always use -Ilib as a ruby option, so both statements seem false to me. Local rubygems will always be loaded and the footprint of our tests will be the same regardless of --disable-gems.

voxik · 2021-03-10T14:48:39Z

By the way, even if we decide to move this forward, I don't see how this part of the commit message is relevant here:

Have you ever tried to do something like $ mv lib/rubygems.rb{,.bak}? I am quite sure the test suite will mostly work, because it is going to use the system version of rubygems.rb file. Similarly, if you have RUBYOPT="-Isome/path -rrubygems" I don't think the test suite is doing much to at least try to detect is it going to use some unexpected version of library.

I am not saying that --disable-gems is going to magically solve the issues, but I think that the test suite should be much more precise about the load paths.

voxik · 2021-03-10T14:51:17Z

I completely disagree with @hsbt on --disable-gems being just debugging feature

It's not only my opinion, it's opinion of ruby-core team.

Thank you for opening the https://bugs.ruby-lang.org/issues/17684

deivid-rodriguez · 2021-03-10T15:25:03Z

Have you ever tried to do something like $ mv lib/rubygems.rb{,.bak}? I am quite sure the test suite will mostly work, because it is going to use the system version of rubygems.rb file. Similarly, if you have RUBYOPT="-Isome/path -rrubygems" I don't think the test suite is doing much to at least try to detect is it going to use some unexpected version of library.

This seems too hypothetical to me. It doesn't seem like a classic programming mistake to delete or rename the most important file in your library 🤣. In any case, I tried it out of curiosity and it fails even if I install to the system the exact same copy of rubygems I intend to test. If other versions of rubygems are installed in the system, it will fail even more badly of course. So no, it doesn't mostly work.

If we end up introducing this, its sole purpose would be to help you. And the decision won't be influenced by whether you sell the change as reducing footprint or the chances that we shoot ourselves in the foot or not.

voxik · 2021-03-10T16:20:21Z

I have already removed the use of --disable-gems in the Fedora package. I'm here mostly because I have promised to deliver test in Three tests fail in Fedora packaging environment #4429 and I think it is good for the purpose I have stated in the commit message. If the result of this exercise was that --disable-gems is deprecated, I might disagree but at least it will be documented.
There is at least one more interesting failure on Fedora, which is why I said earlier that "I think that the test suite should be much more precise about the load paths." I cannot just drop everything into one issue, because it would be even bigger mess then it is now :/

deivid-rodriguez · 2021-03-10T18:06:57Z

@hsbt If you're still against adding a secondary matrix entry to test --disable-gems, please go ahead and close. Otherwise @voxik can push the actual fix and get this green.

deivid-rodriguez · 2021-04-06T12:34:51Z

@voxik Since there was no feedback from @hsbt feel free to move this forward.

hsbt · 2021-04-06T13:08:46Z

I will handle this tomorrow.

hsbt · 2021-04-07T10:29:05Z

I decided to close this. We should reduce the initial footprint or load time of rubygems at first. The --disable-gems way is the final solution for us.

bundlerbot added CI CI related issues RubyGems labels Mar 8, 2021

voxik force-pushed the disable-gems branch from 7891796 to f42ce72 Compare March 8, 2021 19:15

Run tests with --disable-gems.

182b3e2

This makes the test suite execute with `--disable-gems` option. This should help to reduce the initial footprint of loaded libraries and reduce the risk of inadvertently testing system RubyGems instead of upstream version.

voxik force-pushed the disable-gems branch from f42ce72 to 182b3e2 Compare March 10, 2021 11:49

deivid-rodriguez added the status: feedback required label Mar 18, 2021

hsbt closed this Apr 7, 2021

deivid-rodriguez mentioned this pull request Apr 7, 2021

Three tests fail in Fedora packaging environment #4429

Closed

deivid-rodriguez mentioned this pull request May 1, 2021

Test suite fails when the host RubyGems are configured via operating_system.rb #4446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Run tests with `--disable-gems` #4440

[WIP] Run tests with `--disable-gems` #4440

voxik commented Mar 8, 2021

voxik commented Mar 8, 2021

deivid-rodriguez commented Mar 8, 2021

voxik commented Mar 8, 2021

deivid-rodriguez commented Mar 8, 2021

hsbt commented Mar 9, 2021

deivid-rodriguez commented Mar 9, 2021

voxik commented Mar 10, 2021

hsbt commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

voxik commented Mar 10, 2021

voxik commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

voxik commented Mar 10, 2021 •

edited

deivid-rodriguez commented Mar 10, 2021

deivid-rodriguez commented Apr 6, 2021

hsbt commented Apr 6, 2021

hsbt commented Apr 7, 2021

[WIP] Run tests with --disable-gems #4440

[WIP] Run tests with --disable-gems #4440

Conversation

voxik commented Mar 8, 2021

voxik commented Mar 8, 2021

deivid-rodriguez commented Mar 8, 2021

voxik commented Mar 8, 2021

deivid-rodriguez commented Mar 8, 2021

hsbt commented Mar 9, 2021

deivid-rodriguez commented Mar 9, 2021

voxik commented Mar 10, 2021

hsbt commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

voxik commented Mar 10, 2021

voxik commented Mar 10, 2021

deivid-rodriguez commented Mar 10, 2021

voxik commented Mar 10, 2021 • edited

deivid-rodriguez commented Mar 10, 2021

deivid-rodriguez commented Apr 6, 2021

hsbt commented Apr 6, 2021

hsbt commented Apr 7, 2021

[WIP] Run tests with `--disable-gems` #4440

[WIP] Run tests with `--disable-gems` #4440

voxik commented Mar 10, 2021 •

edited