Sort requirements in Gem::Requirement to succeed comparison with different order

[toy]

Description:

This PR ensures that comparison of requirements works properly when order or requirements is different.
Before this PR following two dependencies are considered different:

<Gem::Dependency type=:runtime name="fspath" requirements="< 4, >= 2.1">
<Gem::Dependency type=:runtime name="fspath" requirements=">= 2.1, < 4">

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

When debugging an issue toy/image_optim_pack#20 I've found that in certain conditions platform-specific gem is not selected and an invalid warning is displayed due to comparison of equal dependencies failing. I was able to reproduce it by running following commands:

gem uninstall image_optim_pack --all

gem install image_optim_pack --platform ruby

cat <<GEMFILE > Gemfile
source 'https://rubygems.org'
gem 'image_optim'
GEMFILE
bundle

cat <<GEMFILE > Gemfile
source 'https://rubygems.org'
gem 'image_optim'
gem 'image_optim_pack'
GEMFILE
bundle

What is your fix for the problem, implemented in this PR?

Debugging the issue I found that inside Bundler::LazySpecification#__materialize__ the comparison of dependencies failed due to different order of requirements which led to the warning suggesting that ruby and platform specific gems have different dependencies and selected ruby version of the gem. Suggested change fixes the behaviour.

---

Tasks:

• Describe the problem / feature
• Write tests
• Write code to solve the problem
• Get code review from coworkers / friends

I will abide by the code of conduct.

[deivid-rodriguez]

@toy I couldn't repro the problem on my linux box with recent versions of bundler & rubygems. Maybe this is fixed in recent versions? We started sorting requirements here, but then reverted it here. Reason for the revert was that it wasn't necessary for any of the problems we wanted to fix, and that it was pointed out that requirement ordering inside gemspecs might be there for a reason.

[toy]

@deivid-rodriguez
Reproducing using docker:

docker run --rm -i ruby:2.7 bash <<BASH
gem install image_optim_pack --platform ruby

cat <<GEMFILE > Gemfile
source 'https://rubygems.org'
gem 'image_optim'
GEMFILE
bundle

cat <<GEMFILE > Gemfile
source 'https://rubygems.org'
gem 'image_optim'
gem 'image_optim_pack'
GEMFILE
bundle
BASH

For me output is (note the Unable to use the platform-specific …):

Successfully installed fspath-3.1.2
Successfully installed image_size-2.1.0
Successfully installed exifr-1.3.6
Successfully installed progress-3.5.2
Successfully installed in_threads-1.5.4
Rails image assets optimization is extracted into image_optim_rails gem
You can safely remove `config.assets.image_optim = false` if you are not going to use that gem
Successfully installed image_optim-0.27.0
Successfully installed image_optim_pack-0.6.0.20200215
7 gems installed
Fetching gem metadata from https://rubygems.org/.....
Resolving dependencies...
Using bundler 2.1.4
Using exifr 1.3.6
Using fspath 3.1.2
Using image_size 2.1.0
Using in_threads 1.5.4
Using progress 3.5.2
Using image_optim 0.27.0
Bundle complete! 1 Gemfile dependency, 7 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
Fetching gem metadata from https://rubygems.org/.....
Resolving dependencies...
Unable to use the platform-specific (x86_64-linux) version of image_optim_pack (0.6.0.20200215) because it has different dependencies from the ruby version. To use the platform-specific version of the gem, run `bundle config set specific_platform true` and install again.
Using bundler 2.1.4
Using exifr 1.3.6
Using fspath 3.1.2
Using image_size 2.1.0
Using in_threads 1.5.4
Using progress 3.5.2
Using image_optim 0.27.0
Using image_optim_pack 0.6.0.20200215
Bundle complete! 2 Gemfile dependencies, 8 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

I understand #2627, the order of requirements should not randomly change in the output, but the issue is still there and as far as I know the order of requirements is not important for comparison. I'll try to change my PR to fix the issue while persisting the order of requirements in the output.

[deivid-rodriguez]

@toy Thanks for the docker repro, issue confirmed on my side 👍. Your suggestion also looks perfectly fine, as long as the original order is preserved for other things, it sounds perfectly reasonable to ignore it for requirement comparison 👍.

[toy]

@deivid-rodriguez Restored changes in test/rubygems/test_gem_specification.rb prove that order is preserved.
Not certain if it is ok like this or it is better to find a way to not reevaluate _sorted_requirements and _tilde_requirements multiple time during same comparison.

[deivid-rodriguez]

I'm not sure either. Does memoizing do the trick?

Something else that would be nice is to have a test in bundler with the steps provided in the docker image in addition to the unit test. No need to backport the fix itself to bundler, restricting the spec to rubygems >= 3.2.0.rc.2 only should be enough and would also document that the scenario needs the latest rubygems to work.

[toy]

I'm not sure either. Does memoizing do the trick?

It should, I had an impression that class is mutable, but reading through it I didn't find mutating methods.
So I'll add memoization.

Something else that would be nice is to have a test in bundler with the steps provided in the docker image in addition to the unit test. No need to backport the fix itself to bundler, restricting the spec to rubygems >= 3.2.0.rc.2 only should be enough and would also document that the scenario needs the latest rubygems to work.

I'll try to create a test, bundler/spec/install/gemfile/platform_spec.rb looks like the right place for it.

[toy]

The test succeeds if I install rubygems from the branch (bumping version so condition is met) and fails with rubygems 3.1.23.1.4 (by running the test directly).

[deivid-rodriguez]

Nice @toy, thanks!

I said there was no need to backport the fix itself to bundler, but after a second thought it'd be nice to get the fixed in bundler independently of the underlying rubygems. Could you try something like this in lib/bundler/rubygems_ext.rb to see if it works fine against all supported rubies & rubygems?

diff --git a/bundler/lib/bundler/rubygems_ext.rb b/bundler/lib/bundler/rubygems_ext.rb
index 1d3bdc5565..76b779b5bb 100644
--- a/bundler/lib/bundler/rubygems_ext.rb
+++ b/bundler/lib/bundler/rubygems_ext.rb
@@ -129,6 +129,27 @@ def to_lock
     end
   end
 
+  unless Requirement.instance_methods(false).include?(:_sorted_requirements)
+    module OrderIndependentComparison
+      def ==(other)
+        rubygems_comparison = super
+        return rubygems_comparison unless rubygems_comparison == false
+
+        _sorted_requirements == other._sorted_requirements
+      end
+
+      protected
+
+      def _sorted_requirements
+        @_sorted_requirements ||= requirements.sort_by(&:to_s)
+      end
+    end
+
+    class Requirement
+      prepend OrderIndependentComparison
+    end
+  end
+
   class Platform
     JAVA  = Gem::Platform.new("java") unless defined?(JAVA)
     MSWIN = Gem::Platform.new("mswin32") unless defined?(MSWIN)

[toy]

@deivid-rodriguez I was not sure about calling super before and tilde requirements were not handled specially (Gem::Requirement.new('~> 1.3') == Gem::Requirement.new('~> 1.3.0') would be true).
So I'm patching the class only if it does not handle comparison already and then compare new instances with sorted requirements if requirements are not sorted.

[toy]

Force-pushed to keep history clean: found a bug and memoized

[deivid-rodriguez]

Smart! Thanks for fixing my buggy code 😅!

[deivid-rodriguez]

👍

[deivid-rodriguez]

Thanks @toy!! 💜