Autoswitch to exact bundler version if present by deivid-rodriguez · Pull Request #2583 · ruby/rubygems

deivid-rodriguez · 2019-01-07T11:03:00Z

Description:

Currently we filter out bundler versions not matching the major version of the bundler version in the BUNDLED WITH section of the lock file. But we do not choose the exact bundler version matching the one in the lock file if present. This PR implements that feature.

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 · 2019-01-07T11:05:30Z

test/rubygems/test_gem_bundler_version_finder.rb

    end
    bvf.stub(:bundler_version, v("2.a")) do
-      assert_equal %w[2 2.a 2.0 2.1.1], util_filter_specs(specs).map(&:version).map(&:to_s)
+      assert_equal %w[2.a 2 2.0 2.1.1], util_filter_specs(specs).map(&:version).map(&:to_s)


I had to change this existing expectation. The new expectation makes more sense to me. If we have a prerelease in the BUNDLED WITH section, we still want that activated even if it is a prerelease... right? 🤔

hmmmm good question! I'm not sure how we want to handle autoswitching if the locked version is a prerelease. probably using the exact version is the safest, so I think this is a good place to start.

indirect

Using the exact version if it's present seems like a good step forward! Let's start strategizing how to automatically install the exact version next. 😅

colby-swandale · 2019-01-08T02:33:44Z

I'm really worried about this change, i'm not sure what problem this is solving exactly. It's also starting to make the Bundler Version Finder logic quite complex, which i would prefer to avoid. IMO either we just lock the version down or keep with the current behavior.

colby-swandale · 2019-01-08T02:38:28Z

If there is one thing that i have leaned in the past week, it's to heavily focus on user expectations and to keep surprises to a minimum.

MSP-Greg · 2019-01-08T03:20:33Z

quite complex

Maybe add a comment like "move exact match to the front of array, otherwise return as is"?

colby-swandale · 2019-01-08T03:33:56Z

I'm not sure what you're talking about, I wasn't suggesting a specific change to the PR.

deivid-rodriguez · 2019-01-08T07:12:06Z

I'm really worried about this change, i'm not sure what problem this is solving exactly.

The ability to lock the version of bundler itself, just like we do with other dependencies, has been discussed in length and it's been a mid-term goal for the bundler team for a while. The idea is that by ensuring that the person running bundler under a lock file uses the same version as the person who created the lock file, we ensure that they get the same results. This helps with reproducibility of any potential issues and reduces "it works on my machine" kind of surprises.

It's also starting to make the Bundler Version Finder logic quite complex, which i would prefer to avoid.

Every feature has a cost in terms of complexity and maintaining new code, there's not much that can be done about it... This particular improvement, I don't think it's particularly complex?

IMO either we just lock the version down or keep with the current behavior.

The final goal is full lock, but we want to do it in a smooth manner. That's why we supported #2515, because we were not ready yet for full locking. This changes aims to start this transition in a smooth way.

Let's start strategizing how to automatically install the exact version next. 😅

Yeah, that would be the next (harder) step! 😅

indirect · 2019-01-08T07:19:03Z

@colby-swandale if I'm understanding this PR correctly (which I might not be 😬), this PR is trying to do a specific thing: use the version of Bundler from BUNDLED WITH, if that version is installed.

Right now, we simply use BUNDLED WITH to choose between 1.x and 2.x. For example, if you have installed Bundler 1.17.3, 2.0.0, and 2.0.1, then any 2.x BUNDLED WITH will activate Bundler 2.0.1.

After this change, a BUNDLED WITH of 2.0.0 would activate Bundler 2.0.0, and a BUNDLED WITH of 2.0.1 would activate Bundler 2.0.1. If they are both installed. If the exact version from BUNDLED WITH isn't installed, it should fall back to looking for any 2.x version, and if none are installed it should raise an error just like it does now.

That seems like a strict improvement to me, where you can know that you will get the BUNDLED WITH version if it is installed on the machine you are using. What do you think?

deivid-rodriguez · 2019-01-08T07:34:05Z

@indirect Yes, that's exactly how I expect this improvement to behave. Thanks for explaining it better than me! 😄

@colby-swandale Do you think it is a reasonable improvement?

MSP-Greg · 2019-01-08T14:45:21Z

Thinking about seeing this code months from now, two minor things:

Bang methods normally operate on the calling object and are instance methods. I believe Ruby Core has only one bang class method, part of ENV, which is a edge case. I suppose it could be considered an indication that something's different...
I can only find (quick look) one place where this method is called, Gem::Dependency#matching_specs. It does not operate on the return value. Maybe change

return specs unless exact_match_index

to

return unless exact_match_index

to make it clearer that the return value is ignored? Additionally, maybe add a comment similar to 'return value is ignored' or 'return value is ignored, parameter is changed in-place'?

If the above, code could also change to (long 1st line):

    if exact_match_index = specs.find_index { |spec| spec.version == bundler_version }
      specs.unshift(specs.delete_at(exact_match_index))
    end

deivid-rodriguez · 2019-01-10T16:11:59Z

@MSP-Greg Thanks, I didn't notice that the return value was ignored. I applied your suggestion to clarify that!

Regarding the other stylistic suggestion, I don't have a strong opinion so I'll let other potential reviewers / mergers decide!

MSP-Greg · 2019-01-10T16:25:10Z

@deivid-rodriguez

Thanks for the consideration and update. Before your PR, the method:

passed a parameter
modified it in-place
returned it
also returned nil (note not an empty collection/array)

To me, those four things add up to "what is being processed after this method is called?"...

deivid-rodriguez · 2019-01-10T19:11:17Z

@MSP-Greg Are you suggesting any particular changes to this PR? 🤔

I suggest that you use the review feature in the future, so we can see your comments next to the relevant part of the diff you intend to comment. It should make it easier to understand what you mean, I think.

MSP-Greg · 2019-01-10T19:57:57Z

LGTM

colby-swandale · 2019-01-11T15:20:10Z

@indirect I understand the problem that this is trying to solve and the how it's going about doing it. But my biggest worry is about the user experience here and the surprise factor that this is going to create. Talking with users about how the auto switcher works in Bundler 2, a lot of people find that it's not obvious how this feature works and how it surprised them when first using Bundler 2. I fear that this change is going to be adding more confusion for users. (this is what i mean when i said adding complexity)

Consider the scenario where 1 person has the exact version + the latest version of Bundler installed, and another person only just has the latest version of Bundler installed. Both are working on the same Gemfile but can end up having widely different sets of functionality available without any set of explicitness to an explanation of why. This will be especially hard for new users to Ruby who are not familiar with Bundler or RubyGems.

This is why i much prefer to use @segiddins original implementation to lock down the version for everyone and is made a lot more explicit to the user when the exact version is not installed.

colby-swandale · 2019-01-11T15:23:29Z

I agree fully that work needs to be done to improve the feature of locking down the version for Bundler, but i don't agree that this change is the correct answer.

deivid-rodriguez · 2019-01-11T17:48:06Z

Consider the scenario where 1 person has the exact version + the latest version of Bundler installed, and another person only just has the latest version of Bundler installed. Both are working on the same Gemfile but can end up having widely different sets of functionality available without any set of explicitness to an explanation of why. This will be especially hard for new users to Ruby who are not familiar with Bundler or RubyGems.

But this can already happen. Consider the example where one person has the exact version + the latest version while another person only has the exact version. The same kind of mismatch would happen, and would be fixed by this PR. If we're going to prefer one bundler version over another, it's better to prefer a known working version over a version that the application might have never run against.

Imagine the following user story:

GIVEN that a developer of an application is successfully running bundler against the application
AND the bundler team releases a new minor version with an unintentional breaking change that affects the application
AND the developer installs it
THEN the new bundler release will not break the application for the developer because the known working version will still be used.

This is one situation this PR improves.

I agree fully that work needs to be done to improve the feature of locking down the version for Bundler, but i don't agree that this change is the correct answer.

This PR does not intend to be the answer, just an improvement over the current situation. Our direction for this feature is that the "BUNDLED WITH" version is always installed and used "on the fly".

deivid-rodriguez · 2019-08-08T14:40:02Z

Hei, any updates here anybody?

I still think this is a good move, allowing more control and determinism over the bundler version that an application runs, while still not failing hard if the exact version is not found.

indirect · 2019-08-09T00:33:26Z

As long as we print a warning when the version doesn’t match and we are falling back, I think we should merge this. Maybe we can give it 48 hours and merge if there’s no more feedback?

deivid-rodriguez · 2019-08-10T11:55:59Z

That sounds great to me @indirect!

deivid-rodriguez · 2019-08-14T15:15:06Z

Let's see how it goes!

@bundlerbot r+

2583: Autoswitch to exact bundler version if present r=deivid-rodriguez a=deivid-rodriguez # Description: Currently we filter out bundler versions not matching the major version of the bundler version in the `BUNDLED WITH` section of the lock file. But we do not choose the exact bundler version matching the one in the lock file if present. This PR implements that feature. # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

ghost · 2019-08-14T15:50:24Z

Build succeeded

continuous-integration/travis-ci/push

bronzdoc added the status: triage label Jan 7, 2019

deivid-rodriguez commented Jan 7, 2019

View reviewed changes

indirect approved these changes Jan 8, 2019

View reviewed changes

deivid-rodriguez added 2 commits August 8, 2019 15:37

Autoswitch to exact bundler version if present

bb02953

Return nil to clarify return value is ignored

8702f59

deivid-rodriguez force-pushed the better_autoswitch branch from 2d48a22 to 8702f59 Compare August 8, 2019 13:47

ghost merged commit 8702f59 into ruby:master Aug 14, 2019

bronzdoc removed the status: triage label Aug 14, 2019

deivid-rodriguez deleted the better_autoswitch branch August 14, 2019 15:55

hsbt modified the milestone: 3.0.7 Feb 18, 2020

This was referenced Dec 11, 2020

Bundler does not find platform specific gem #4123

Closed

bundle install does not respect lock file #4122

Closed

This pull request was closed.

Uh oh!

Conversation

deivid-rodriguez commented Jan 7, 2019

Description:

Tasks:

Uh oh!

deivid-rodriguez Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

indirect Jan 7, 2019

Choose a reason for hiding this comment

Uh oh!

indirect left a comment

Choose a reason for hiding this comment

Uh oh!

colby-swandale commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colby-swandale commented Jan 8, 2019

Uh oh!

MSP-Greg commented Jan 8, 2019

Uh oh!

colby-swandale commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Jan 8, 2019

Uh oh!

indirect commented Jan 8, 2019

Uh oh!

deivid-rodriguez commented Jan 8, 2019

Uh oh!

MSP-Greg commented Jan 8, 2019

Uh oh!

deivid-rodriguez commented Jan 10, 2019

Uh oh!

MSP-Greg commented Jan 10, 2019

Uh oh!

deivid-rodriguez commented Jan 10, 2019

Uh oh!

MSP-Greg commented Jan 10, 2019

Uh oh!

colby-swandale commented Jan 11, 2019

Uh oh!

colby-swandale commented Jan 11, 2019

Uh oh!

deivid-rodriguez commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Aug 8, 2019

Uh oh!

indirect commented Aug 9, 2019

Uh oh!

deivid-rodriguez commented Aug 10, 2019

Uh oh!

deivid-rodriguez commented Aug 14, 2019

Uh oh!

ghost commented Aug 14, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

colby-swandale commented Jan 8, 2019 •

edited

Loading

colby-swandale commented Jan 8, 2019 •

edited

Loading

deivid-rodriguez commented Jan 11, 2019 •

edited

Loading