Downgrade bundler mismatches to a warning by deivid-rodriguez · Pull Request #2621 · ruby/rubygems

deivid-rodriguez · 2019-02-05T10:00:05Z

Description:

closes #2592.

The behavior described in that issue is expected, we really prefer the BUNDLED_WITH version in the lock file to be used when running a bundled application.

However, at the current time, it is too much to fail because of mismatches here, so I propose to downgrade this to a warning for the time being.

The strategy I followed is to, instead of filtering out bundler versions not matching the major version of the "BUNDLED WITH" bundler, we we put a version matching it (if it exists) in front of the matching specs, so that is the one to be picked up. Otherwise a warning is raised. By not removing any versions, we make sure we no longer fail because of not finding an appropriate bundler version.

Commit per commit review recommended, since I needed a few refactorings before implementing the fix.

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.

indirect · 2019-02-22T06:34:25Z

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 1 1.0 1.0.1.1 2.a 2.0 2.1.1 3 3.a 3.0 3.1.1], util_prioritize_specs(specs)


Shouldn't 2.a try 2, but then try 2.a before falling back to 1?

Oh, I see... it is that 2 is available, so it's moved to the front. And if 2 was not available, 2.a would be moved to the front instead. Is that right?

That's exactly right.

The new strategy would be to find the first compatible version and move it to the front, but leave the array untouched if no compatible version is found. The difference with the old strategy is that no bundler versions are ever filtered out, so the array will never be left empty, and thus we'll never cause an error.

We could do further prioritization here as you suggested in the first comment, but only the first spec in this array gets actually used, so it seemed unnecessary.

indirect · 2019-02-24T09:28:08Z

Got it. Sounds good to me!

…

On Feb 22, 2019, 1:31 AM -0800, David Rodríguez ***@***.***>, wrote: @deivid-rodriguez commented on this pull request. In 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 1 1.0 1.0.1.1 2.a 2.0 2.1.1 3 3.a 3.0 3.1.1], util_prioritize_specs(specs) That's exactly right. The new strategy would be to find the first compatible version and move it to the front, but leave the array untouched if no compatible version is found. The difference with the old strategy is that no bundler versions are ever filtered out, so the array will never be left empty, and thus we'll never cause an error. We could do further prioritization here as you suggested in the first comment, but only the first spec in this array gets actually used, so it seemed unnecessary. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

hsbt · 2019-03-23T08:49:00Z

I also agreed with this strategy.

hsbt · 2019-03-23T08:49:04Z

@bundlerbot r+

2621: Downgrade bundler mismatches to a warning r=hsbt a=deivid-rodriguez # Description: closes #2592. The behavior described in that issue is expected, we really prefer the BUNDLED_WITH version in the lock file to be used when running a bundled application. However, at the current time, it is too much to fail because of mismatches here, so I propose to downgrade this to a warning for the time being. The strategy I followed is to, instead of filtering out bundler versions not matching the major version of the "BUNDLED WITH" bundler, we we put a version matching it (if it exists) in front of the matching specs, so that is the one to be picked up. Otherwise a warning is raised. By not removing any versions, we make sure we no longer fail because of not finding an appropriate bundler version. Commit per commit review recommended, since I needed a few refactorings before implementing the fix. # 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-03-23T09:20:26Z

Build succeeded

continuous-integration/travis-ci/push

This reverts commit dad8d25, reversing changes made to b4063c6.

2694: Revert downgrading bundler mismatches to a warning r=deivid-rodriguez a=deivid-rodriguez This reverts commit dad8d25, reversing changes made to b4063c6. # Description: #2621 broke bundler's build. I'm going to investigate why but in the meantime I don't want to have a red master in bundler so I'm reverting this. I wonder if we should test rubygems against a vendored bundler that contains the code in bundler's master. Probably too much overhead 🤷‍♂️. # Tasks: - [x] Describe the problem / feature - [ ] 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>

2695: Add a job to test bundler's master r=bronzdoc a=deivid-rodriguez # Description: #2621 broke our bundler's master build without us noticing in advance. I'd like to add one job that would've allowed us to catch this. # Tasks: - [x] Describe the problem / feature - [ ] 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>

hsbt · 2019-03-25T02:49:59Z

@deivid-rodriguez Can you restore this pull-request for the future plan?

deivid-rodriguez · 2019-03-25T10:40:18Z

@hsbt Sure! Even after restoring the branch, I wouldn't be given the option to reopen this pull request, so I opened a separate one to follow up: #2696.

6996: Downgrade version mismatches to warnings r=indirect a=deivid-rodriguez Closes #6993. Complements ruby/rubygems#2621. ### What was the end-user problem that led to this PR? The problem was that bundler 2 has become too unfriendly because it errors out when the running version does not match the version in the BUNDLED WITH section in the lockfile. ### What was your diagnosis of the problem? My diagnosis is that we still believe that making sure the exact same bundler version is always run for a specific application is a good thing, because it ensures resolution is always the same because it's resolved by the exact same code that generated the lockfile. BUT, we need to ensure this in a more user friendly way, for example by auto-switching and auto-installing the right bundler version without any user intervention. ### What is your fix for the problem, implemented in this PR? My fix is to downgrade this hard error to a warning that does not prevent bundler from running. ### Why did you choose this fix out of the possible options? I chose this fix because it still shows the mismatch but allows bundler to run, thus being a bit friendlier. Hopefully this won't be needed once bundler is smart enough to autoswitch and autoinstall. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

bronzdoc added the status: triage label Feb 5, 2019

deivid-rodriguez added 5 commits February 18, 2019 16:50

Refactor filter! method to use compatible?

9699d2a

Small logic simplification in to_spec

6668222

Extract an spec_for_exe utility method

762a7b0

Extract some common logic in filter! specs

4aaec06

Downgrade bundler version mismatches to a warning

6b5662b

deivid-rodriguez force-pushed the downgrade_bundler_mismatches_to_a_warning branch from e3ef29e to 6b5662b Compare February 18, 2019 16:06

bronzdoc requested review from indirect and segiddins February 21, 2019 18:08

indirect reviewed Feb 22, 2019

View reviewed changes

deivid-rodriguez mentioned this pull request Feb 27, 2019

Downgrade version mismatches to warnings rubygems/bundler#6996

Merged

ghost merged commit 6b5662b into ruby:master Mar 23, 2019

bronzdoc removed the status: triage label Mar 23, 2019

deivid-rodriguez deleted the downgrade_bundler_mismatches_to_a_warning branch March 23, 2019 09:42

deivid-rodriguez mentioned this pull request Mar 23, 2019

Fix circular require warnings rubygems/bundler#7061

Merged

deivid-rodriguez added a commit that referenced this pull request Mar 23, 2019

Revert "Merge #2621"

412fb8a

This reverts commit dad8d25, reversing changes made to b4063c6.

This was referenced Mar 23, 2019

Revert downgrading bundler mismatches to a warning #2694

Merged

Add a job to test bundler's master #2695

Merged

deivid-rodriguez restored the downgrade_bundler_mismatches_to_a_warning branch March 24, 2019 14:45

deivid-rodriguez deleted the downgrade_bundler_mismatches_to_a_warning branch March 24, 2019 14:50

deivid-rodriguez restored the downgrade_bundler_mismatches_to_a_warning branch March 25, 2019 10:37

deivid-rodriguez mentioned this pull request Mar 25, 2019

Downgrade bundler mismatches to a warning (2nd try) #2696

Closed

4 tasks

deivid-rodriguez deleted the downgrade_bundler_mismatches_to_a_warning branch March 25, 2019 10:40

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Downgrade bundler mismatches to a warning#2621

Downgrade bundler mismatches to a warning#2621
5 commits merged intoruby:masterfrom
deivid-rodriguez:downgrade_bundler_mismatches_to_a_warning

deivid-rodriguez commented Feb 5, 2019 •

edited by bronzdoc

Loading

Uh oh!

indirect Feb 22, 2019

Uh oh!

indirect Feb 22, 2019

Uh oh!

deivid-rodriguez Feb 22, 2019

Uh oh!

indirect commented Feb 24, 2019 via email

Uh oh!

hsbt commented Mar 23, 2019

Uh oh!

hsbt commented Mar 23, 2019

Uh oh!

ghost commented Mar 23, 2019

Uh oh!

hsbt commented Mar 25, 2019

Uh oh!

deivid-rodriguez commented Mar 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

deivid-rodriguez commented Feb 5, 2019 • edited by bronzdoc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Tasks:

Uh oh!

indirect Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

indirect Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

indirect commented Feb 24, 2019 via email

Uh oh!

hsbt commented Mar 23, 2019

Uh oh!

hsbt commented Mar 23, 2019

Uh oh!

ghost commented Mar 23, 2019

Build succeeded

Uh oh!

hsbt commented Mar 25, 2019

Uh oh!

deivid-rodriguez commented Mar 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deivid-rodriguez commented Feb 5, 2019 •

edited by bronzdoc

Loading