Fix canonical_segments/equality to match <=> and pre-2.7.0 behaviour

[jhawthorn]

Description:

This fixes #2595 by not removing 0's which precede a prerelease segment. I've split this into 4 commits, each of which should pass tests to "show my work", but they could all be squashed.

The final commit re-adds canonical_segments == other.canonical_segments, which probably isn't necessary since canonical_segments now match the later logic in this method, but I left it in to make the change as small as possible.

I will abide by the code of conduct.

[welcome]

Thanks for opening a pull request and helping make RubyGems better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

[segiddins]

Can you be a bit more explicit what this is trying to fix? I dont think this is correct, 1.pre should probably == 1.0.pre, holding the invariant that a == b -> a + x == b + x

[jhawthorn]

(oops. I gave this a bad title. Fixed)

#2595 has the detailed description of the issue, the short version is:

a = 0.0.beta.1
b = 0.beta.1 
c = 0.0.beta

Previously, a == b, a > c, but b < c. So the comparison isn't transitive (meaning ex. Array#sort won't work correctly in some cases)

This restores the transitive property and reverts comparisons to the pre-2.7.0 behaviour.

I dont think this is correct, 1.pre should probably == 1.0.pre, holding the invariant that a == b -> a + x == b + x

(I've interpreted + as concatenation, sorry if that's wrong)

IMO 1.pre should not == 1.0.pre. that invariant doesn't hold for ex. adding .1 instead of .pre, 1 == 1.0 -> 1.1 == 1.0.1. I could see a special case being made for prereleases, but it unfortunately doesn't work well with comparisons.

Oh! I came up with another example under the previous behaviour:

> def v(s); Gem::Version.new(s); end
> v("1.a") < v("1.b")
=> true
> v("1.0.a") < v("1.b")
=> false

[segiddins]

I think we should fix the b < c behavior instead of reverting the a == b behavior

[jhawthorn]

I think we should fix the b < c behavior instead of reverting the a == b behavior

That would make this internally consistent, but I don't think >/< should be changed, since it's always worked that way (unlike == which has changed, I believe unintentionally). Changing how gem versions compare is weird, because gem developers can't exactly cut a new release to fix them, like they might with any other API change.

Changing >/< in this way would also break a common pattern of using ex. ~> 5.x to allow prereleases of a gem when using semver. Currently 5.0.0.beta satisfies ~> 5.x, but changing > to match == would mean it wouldn't. Off hand I know older versions of rails-controller-testing uses this pattern (these were cut when rails 5 was in beta, so the usage of ~> 5.x was very intentional). I think this pattern is pretty common (read: I have used it 😬).

On the other hand I don't think changing the behaviour of == will cause any major problems. If it helps, I could check all gem versions against all gem requirements from the rubygems.org dump to see if there are inconsistencies 🤷‍♂️.

[deivid-rodriguez]

@jhawthorn I had a look at this. What about this patch?

diff --git a/lib/rubygems/version.rb b/lib/rubygems/version.rb
index f2f10569..7d7c15f7 100644
--- a/lib/rubygems/version.rb
+++ b/lib/rubygems/version.rb
@@ -343,8 +343,8 @@ class Gem::Version
     return unless Gem::Version === other
     return 0 if @version == other._version || canonical_segments == other.canonical_segments
 
-    lhsegments = _segments
-    rhsegments = other._segments
+    lhsegments = canonical_segments
+    rhsegments = other.canonical_segments
 
     lhsize = lhsegments.size
     rhsize = rhsegments.size
diff --git a/test/rubygems/test_gem_version.rb b/test/rubygems/test_gem_version.rb
index 939360c7..ece8001c 100644
--- a/test/rubygems/test_gem_version.rb
+++ b/test/rubygems/test_gem_version.rb
@@ -157,6 +157,10 @@ class TestGemVersion < Gem::TestCase
     assert_equal( 1, v("1.8.2.a10") <=> v("1.8.2.a9"))
     assert_equal( 0, v("")          <=> v("0"))
 
+    assert_equal( 0, v("0.beta.1")  <=> v("0.0.beta.1"))
+    assert_equal(-1, v("0.0.beta")  <=> v("0.0.beta.1"))
+    assert_equal(-1, v("0.0.beta")  <=> v("0.beta.1"))
+
     assert_nil v("1.0") <=> "whatever"
   end

It matches the behavior I personally expect for the tests you added.

[deivid-rodriguez]

Changing >/< in this way would also break a common pattern of using ex. ~> 5.x to allow prereleases of a gem when using semver. Currently 5.0.0.beta satisfies ~> 5.x, but changing > to match == would mean it wouldn't. Off hand I know older versions of rails-controller-testing uses this pattern (these were cut when rails 5 was in beta, so the usage of ~> 5.x was very intentional). I think this pattern is pretty common (read: I have used it grimacing).

I don't think we want to break this pattern. Since the patch I just posted passes all tests for me, we should also add a new test covering this pattern in case my patch is actually breaking it.

[jhawthorn]

I don't think we want to break this pattern. Since the patch I just posted passes all tests for me, we should also add a new test covering this pattern in case my patch is actually breaking it.

@deivid-rodriguez thanks for taking a look! Your patch does break this pattern and I've now added additional tests for that 🙂 (both to the version test and requirement test, where I think it's more clear).

  1) Failure:
TestGemRequirement#test_satisfied_by_eh_good [test/rubygems/test_gem_requirement.rb:270]:
Expected 5.0.0.rc2 to satisfy ~> 5.x

[deivid-rodriguez]

@jhawthorn Thanks!

So, I wasn't accurate on defining my expectations in my last message. The way I would expect rubygems to work is so that 5.0.0.beta does not satisfy ~> 5.x, but it does satisfy ~> 5.a. My patch satisfies that, and also fixes your problem. I would consider it a bug fix, since it matches the rules defined in

https://github.com/rubygems/rubygems/blob/1172320540c8c33c59fc1db5191b021c3b2db487/lib/rubygems/version.rb#L3-L30

I think the only reason 5.0.0.beta was matching ~> 5.x was that non canonical segments were being used (without trailing zeros removed), so 5.0.0.beta was considered higher than 5.x simply because it had more segments.

I do understand people are currently relying on this bug, so it's complicated how to proceed. We could consider deprecating it and keeping it until the next major, it might be worth investigating how hard it is to do that.

[deivid-rodriguez]

Actually, it might be easier to actually look out for for gems with these wrong requirements and spread out PRs changing this? 😃

[jhawthorn]

spread out PRs changing this

gems using this already exist and are on rubygems, sending PRs will do nothing to "fix" existing published gems using this pattern. They're immutable (unless they are all yanked).

I don't think the concept of "how gem versions work" can ever be safely deprecating and it would be best if it never changed. It's unfortunate it was changed by accident in #1659. Fortunately it changed in a mostly inconsequential (due to usage) way. We would not be so lucky with the proposed change to </>.

Personally, I'd rather we keep the current, broken (per #2595) change than to change </> 😬.

5.0.0.beta was considered higher than 5.x simply because it had more segments.

This is inaccurate. 5.0.0.beta was considered higher than 5.x because segment x is less than 0.

(Docs from rubygems/lib/rubygems/version.rb)

I see no inconsistencies between the docs here and the current behaviour (although it's wrong about "a-z", since "A-Z" are also supported). Nothing describes internal .0s being ignored (in fact nothing describes trailing .0s being ignored, which could be improved).

---

It would be nice to hear where the desire to strip internal .0s comes from. Though it may be intuitive to some, I was very surprised to find that 1.a was considered the same version as 1.0.a (and I knew that 1 was the same as 1.0), so I don't think it's universally more expected.

[deivid-rodriguez]

gems using this already exist and are on rubygems, sending PRs will do nothing to "fix" existing published gems using this pattern. They're immutable (unless they are all yanked).

But this will only affect versions of gems depending on unreleased pre's. Once the final version is released, this problem should no longer happen, right? For example, rails-controller-testing depending on rails ~> 5.x should no longer be a problem? So my guess is that this is currently affecting very few gems. For those, the situation can be improved with PRs and new fixed versions, because the dependants that update to them wouldn't be affected. Is this correct?

This is inaccurate. 5.0.0.beta was considered higher than 5.x because segment x is less than 0.

You're right. I misunderstood those docs, and didn't double check with the code.

I see no inconsistencies between the docs here and the current behaviour (although it's wrong about "a-z", since "A-Z" are also supported). Nothing describes internal .0s being ignored (in fact nothing describes trailing .0s being ignored, which could be improved).

You're right again. But the behavior we're currently talking about (5.0.0.beta matching ~> 5.x) is not documented nor tested either, I think? And whereas removing zeros is not documented either, it's currently part of the test suite.

It would be nice to hear where the desire to strip internal .0s comes from. Though it may be intuitive to some, I was very surprised to find that 1.a was considered the same version as 1.0.a (and I knew that 1 was the same as 1.0), so I don't think it's universally more expected.

To me that was a good change, because of being intuitive. I do expect that 1.a is the same as 1.0.a, but maybe it is because I read the docs and the code before actually thinking about it? Would this be intuitive if we followed semver strictly, i.e., would it be intuitive that 1.0-a is the same as 1-a? Maybe starting strict semver support could be another option.

To sum up, I tend to prefer to go with the option 2 you proposed in #2595, preceded by PRs to affected gems. That said, I could live with what you are proposing here too.

I'd like to hear more opinions!

[deivid-rodriguez]

Still it strikes as weird to me that ~> 5.x matches 5.0.0.beta because of "x" being lexicographically less than "0", since "x" is part of the prerelease segment, and "0" is part of the minor release segment, so they should never be compared to each other.

[deivid-rodriguez]

Also, what if we had ~> 5.rc, does it make sense that 5.0.0.beta matches it? It doesn't to me...

Going back in history a bit, I'm now remembering how I started learning about rubygems prereleases. It was because I originally proposed tweaking the dependency recommendation for semver dependencies when using upper bounds to include ".x" after the upper bound. @segiddins told me that would introduce a bug of not excluding ".a" prereleases (and others alphabetically sorted before ".x"). That was actually not happening, but now I realized it was only because of this bug of not comparing canonical versions. See #2266 for the discussion.

[deivid-rodriguez]

I looked for gems with these kind of requirements on my system with fd gemspec ~/.rbenv/versions/ | xargs rg "dependency.*\.x" and found three gems. But changing this wouldn't have any effect on any of them, because they apply to old versions already with final releases.

[deivid-rodriguez]

Superseded by #2651.