Support for pre flag in bundle update/bundle lock

[siegfault]

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

More thoroughly described in #5207, but essentially allowing bumps to prerelease versions of gems without specifying a version in the lockfile.

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

Passing this flag allows bumping to the current version, even if that
version is prerelease. This is a superset of the major flag since it
behaves the same but also considers prerelease versions.

The main alternative I see would be having pre work in concert with the currently existing flags. This would mean passing --pre --major would work as in this PR, but --pre --minor would prefer not to bump major versions.

Assuming we want the current behavior, are there any other commands I should update, or any other places I should add specs?

Closes #5207.

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

[welcome]

Thanks for opening a pull request and helping make RubyGems and Bundler 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 GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions 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 or #bundler channel on Slack.

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

[deivid-rodriguez]

Thanks so much for working on this @siegfault!

The main alternative I see would be having pre work in concert with the currently existing flags. This would mean passing --pre --major would work as in this PR, but --pre --minor would prefer not to bump major versions.

I like this idea, also allowing --pre without any existing flags which would work like already proposed by this PR.

[deivid-rodriguez]

There are still some conflicts here.

[siegfault]

Definitely, thanks for being on top of this! I'm switching computers and wanted to push my progress, but will hopefully have something more ready to view next week

[siegfault]

As an update since this has been sitting awhile without movement (:sweat_smile:):

I'm pushing up a change that feels fairly close to what we want so I wanted to open up some questions.

Questions

1. Maybe it's just me, but the effect of the sort within GemVersionPromoter#sort_dep_specs was really hard for me to wrap my head around. I ended up not needing to modify it and there is some documentation for it within the class (maybe elsewhere too?), but would you like me to (a) expand the docs or (b) make the code more obvious, perhaps by using sort_by as in

   rubygems/lib/rubygems/resolver.rb

   Lines 281 to 292 in 23c3622

   	def sort_dependencies(dependencies, activated, conflicts)
   	dependencies.sort_by.with_index do |dependency, i|
   	name = name_for(dependency)
   	[
   	activated.vertex_named(name).payload ? 0 : 1,
   	amount_constrained(dependency),
   	conflicts[name] ? 0 : 1,
   	activated.vertex_named(name).payload ? 0 : search_for(dependency).count,
   	i, # for stable sort
   	]
   	end
   	end

   ?
2. I've added some tests in bundler/spec/commands/update_spec.rb and there should probably be additions in bundler/spec/bundler/gem_version_promoter_spec.rb, but are there any other places that come to mind that I should add tests?
3. Would you rather I revert and/or break out any smaller changes into their own PRs (map {}.uniq == [true] -> all? {}, adding the ? to interogative methods, etc.)?

GemVersionPromoter Sort

The sort in GemVersionPromoter was difficult for me to tease out, but I believe it's essentially:

1. Prerelease versions (unless the current version is already a prerelease)
2. Downgrades
3. Upgrades, organized by number of major(/minor?) upgrades descending and version ascending
4. Current version (if the current version is locked)

To clarify (3), given:

• --minor flag
• current version 4.0
• versions [..., 4.0, 5.0, 5.1, 6.0, 6.1]

, we want to group upgrades by how many major bumps there are ([6.X, 5.X, 4.X]). This gives us [6.0, 6.1, 5.0, 5.1, 4.1].

[deivid-rodriguez]

Thanks @siegfault, it's looking very good!

Regarding improvements to gem version promoter, both of your suggestions look nice, happy to take any improvements there. Also I made some changes to this class while introducing PubGrub, and I think it can use some more renaming now since for example, sort_spec_deps no longer makes sense as a name since the class now deals with resolution packages, not dependencies.

[siegfault]

Thanks for the feedback! I've moved the smaller changes to their own commits, taking your early return suggestion, reverted the changelog bit, and all bundler specs pass for me locally (outside 6 that fail for me on master as well).

With sort_versions being so simple and straightforward now (compared to when I first dug into this), it could make sense to let the specs for GemVersionPromoter test the public interface. I've pushed up a change with that, but I'm happy to revert it if we want to keep things as is. The diff is a little noisy since the args are expected in reverse order.

Either way, I'll work on adding some tests for --pre in gem_version_promoter_spec, likely tomorrow

[deivid-rodriguez]

Thanks @siegfault!

Looking forward to those specs, reviewed the other stuff and feels ready to me!

Thanks so much for the changes to version promoter specs, their current shape was a pain to work with when refactoring this class.

[siegfault]

Re: the GemVersionPromoter specs, I've made a few more changes. I think there are five considerations for these tests (level, pre, strict, locked, and whether the current version is a prerelease version, though the last one overlaps with pre and didn't seem to behave how I expected under test). Rather than write out the 16 (/32 if the last consideration is real) combinations, I wrote most with independent tests for each value. The existing combined tests were maintained (level vs strict) because these seem the most interrelated. Let me know if you'd prefer a different resolution on any of those or if you'd prefer more of the build up (hidden within sorted_versions to be within the tests. Thanks so much!

[deivid-rodriguez]

@siegfault So sorry I dropped the ball here, I will make sure to include this PR in the final 2.4.0 release before the end of the year. Could you fix the lint failures?

[siegfault]

@deivid-rodriguez no worries at all! I know end of year can be pretty busy. My fault for not rerunning rubocop after making the spec changes. Just pushed a change that should fix the hash rockets

[deivid-rodriguez]

@siegfault Thanks so much for the fine work on this, and for your patience. This patch looks great to me ❤️.

However, the specs added in the last commit raise some questions, since the current behavior feels quite surprising and I suspect we may want to tweak the implementation instead of codifying the current behavior in specs. I also have some minor comments about wording.

Since I don't want to block this whole PR on that, how about temporarily removing the last commit and merging the rest of the PR as is, and then discussing further the desired behavior of the gem promoter on a separate PR.

[siegfault]

@deivid-rodriguez absolutely! Just dropped the last commit. I'll open up a new PR with that once this is merged. Thanks!

[deivid-rodriguez]

Sorry I'm always seeing some little things right before merging 😅.

[deivid-rodriguez]

Thanks so much @siegfault ❤️

[siegfault]

Of course! Thanks so much for helping get it merged! I'll open up that follow up spec PR at some point soon

[deivid-rodriguez]

Hei @siegfault, just a note since I stumbled on this today. I think there's a bug in the different criteria considered by the gem version promoter, not sure if it's the same behavior that surprised you when writing specs:

Prereleases are NOT considered even when the dependency specification includes a prerelease segment, if the dependency is not locked. To me that doesn't make sense, because I could create a Gemfile with say gem "rails", ">= 7.0.0.beta1" and I'd expect prereleases considered there even if there's no lock file yet.

I think a patch like this would fix that:

diff --git a/bundler/lib/bundler/gem_version_promoter.rb b/bundler/lib/bundler/gem_version_promoter.rb
index f08fceb7e2..d281f46eeb 100644
--- a/bundler/lib/bundler/gem_version_promoter.rb
+++ b/bundler/lib/bundler/gem_version_promoter.rb
@@ -93,7 +93,7 @@ def sort_dep_specs(specs, package)
       locked_version = package.locked_version
 
       result = specs.sort do |a, b|
-        unless locked_version && (package.prerelease_specified? || pre?)
+        unless package.prerelease_specified? || pre?
           a_pre = a.prerelease?
           b_pre = b.prerelease?
 

I may propose this at some point but if you get to proposing these specs first and you agree, feel free to propose it yourself too together with the specs!