Fix only_update_to_newer_versions regression by theflow · Pull Request #6708 · rubygems/bundler

theflow · 2018-09-25T16:49:41Z

This is my attempt to fix #6529

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

Running bundle update with BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS: "true" resulted in a gem getting downgraded to a really old version in a certain edge case. Ironically it wouldn't get downgraded when BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS was set to false.

What was your diagnosis of the problem?

My diagnosis was that 47256d2 tried to solve the problem of still allowing manual downgrades in the Gemfile while only_update_to_newer_versions is true. But introduced a regression that prevented the additional_base_requirements_for_resolve method to work as intended:

This is the relevant change from that commit that tries to avoid adding the >= requirement if the requirement in the Gemfile is different than the requirement in the lockfile (as far as I understand it):

next requirements if @locked_deps[name] != dependencies_by_name[name]

I identified two problems

dependencies_by_name[name] returns an array of Bundler::Dependency, where as
@locked_deps[name] just returns a single Bundler::Dependency. Comparing the two will always be false.
@locked_deps is always empty in case of bundle update. See: https://github.com/bundler/bundler/blob/3d9e6167a7df9ca89a030dfe95c7cdff293e74a9/lib/bundler/definition.rb#L95

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

My fixes:

Make sure dependencies_by_name is a hash with Bundler::Dependency as values
Fetch the @locked_gems.dependencies again instead of using @locked_deps
The existing test worked for me with and without the only_update_to_newer_versions set to true, I replaced it with a reproduction of the edge case I was investigating (this is as minimal as I could make it)
I've added a test for the manual downgrading case.

Why did you choose this fix out of the possible options?

This is the only way I could make these cases work. It's possible there are other edge cases I don't understand.

ghost · 2018-09-25T16:50:22Z

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler 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 #bundler channel on Slack.

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

greysteil

This looks good to me - nice catch! And the test looks solid.

greysteil · 2018-09-25T17:10:57Z

lib/bundler/definition.rb

      return [] unless @locked_gems && Bundler.feature_flag.only_update_to_newer_versions?
-      dependencies_by_name = dependencies.group_by(&:name)
+      dependencies_by_name = dependencies.each_with_object({}) {|dep, memo| memo[dep.name] = dep }
+      all_locked_deps = @locked_gems.dependencies


I'd rather lose this assignment and use the @locked_gems.dependencies directly in the loop

greysteil · 2018-09-25T17:32:02Z

lib/bundler/definition.rb

    def additional_base_requirements_for_resolve
      return [] unless @locked_gems && Bundler.feature_flag.only_update_to_newer_versions?
-      dependencies_by_name = dependencies.group_by(&:name)
+      dependencies_by_name = dependencies.each_with_object({}) {|dep, memo| memo[dep.name] = dep }


We don't have each_with_object in Ruby 1.8.7, so this will need to use reduce

theflow · 2018-09-26T08:52:53Z

I've incorporated your comments

greysteil · 2018-09-26T09:01:34Z

@bundlerbot r+

6708: Fix only_update_to_newer_versions regression r=greysteil a=theflow This is my attempt to fix #6529 ### What was the end-user problem that led to this PR? Running `bundle update` with `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS: "true"` resulted in a gem getting downgraded to a really old version in a certain edge case. Ironically it wouldn't get downgraded when `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS` was set to false. ### What was your diagnosis of the problem? My diagnosis was that 47256d2 tried to solve the problem of still allowing manual downgrades in the Gemfile while `only_update_to_newer_versions` is true. But introduced a regression that prevented the `additional_base_requirements_for_resolve` method to work as intended: This is the relevant change from that commit that tries to avoid adding the `>=` requirement if the requirement in the Gemfile is different than the requirement in the lockfile (as far as I understand it): ```ruby next requirements if @locked_deps[name] != dependencies_by_name[name] ``` I identified two problems 1. `dependencies_by_name[name]` returns an array of `Bundler::Dependency`, where as `@locked_deps[name]` just returns a single `Bundler::Dependency`. Comparing the two will always be false. 1. `@locked_deps` is always empty in case of `bundle update`. See: https://github.com/bundler/bundler/blob/3d9e6167a7df9ca89a030dfe95c7cdff293e74a9/lib/bundler/definition.rb#L95 ### What is your fix for the problem, implemented in this PR? My fixes: 1. Make sure `dependencies_by_name` is a hash with `Bundler::Dependency` as values 1. Fetch the `@locked_gems.dependencies` again instead of using `@locked_deps` 1. The existing test worked for me with and without the `only_update_to_newer_versions` set to true, I replaced it with a reproduction of the edge case I was investigating (this is as minimal as I could make it) 1. I've added a test for the manual downgrading case. ### Why did you choose this fix out of the possible options? This is the only way I could make these cases work. It's possible there are other edge cases I don't understand. Co-authored-by: Florian Munz <surf@theflow.de>

ghost · 2018-09-26T09:28:04Z

Build succeeded

continuous-integration/travis-ci/push

greysteil · 2018-09-26T09:32:08Z

Thanks for this @theflow! 🎉

segiddins · 2018-10-03T03:20:43Z

Thanks so much for this!

6708: Fix only_update_to_newer_versions regression r=greysteil a=theflow This is my attempt to fix #6529 ### What was the end-user problem that led to this PR? Running `bundle update` with `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS: "true"` resulted in a gem getting downgraded to a really old version in a certain edge case. Ironically it wouldn't get downgraded when `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS` was set to false. ### What was your diagnosis of the problem? My diagnosis was that 47256d2 tried to solve the problem of still allowing manual downgrades in the Gemfile while `only_update_to_newer_versions` is true. But introduced a regression that prevented the `additional_base_requirements_for_resolve` method to work as intended: This is the relevant change from that commit that tries to avoid adding the `>=` requirement if the requirement in the Gemfile is different than the requirement in the lockfile (as far as I understand it): ```ruby next requirements if @locked_deps[name] != dependencies_by_name[name] ``` I identified two problems 1. `dependencies_by_name[name]` returns an array of `Bundler::Dependency`, where as `@locked_deps[name]` just returns a single `Bundler::Dependency`. Comparing the two will always be false. 1. `@locked_deps` is always empty in case of `bundle update`. See: https://github.com/bundler/bundler/blob/3d9e6167a7df9ca89a030dfe95c7cdff293e74a9/lib/bundler/definition.rb#L95 ### What is your fix for the problem, implemented in this PR? My fixes: 1. Make sure `dependencies_by_name` is a hash with `Bundler::Dependency` as values 1. Fetch the `@locked_gems.dependencies` again instead of using `@locked_deps` 1. The existing test worked for me with and without the `only_update_to_newer_versions` set to true, I replaced it with a reproduction of the edge case I was investigating (this is as minimal as I could make it) 1. I've added a test for the manual downgrading case. ### Why did you choose this fix out of the possible options? This is the only way I could make these cases work. It's possible there are other edge cases I don't understand. Co-authored-by: Florian Munz <surf@theflow.de> (cherry picked from commit 8501b1e)

* 1-16-stable: Version 1.16.6 with changelog fix uninitialized @use_gvp instance var warning no longer test Ruby 1.9.3 against rubygems master Merge #6708 Auto merge of #6697 - walf443:added_changelog_section, r=hsbt Merge #6687 Merge #6686 Auto merge of #6670 - bundler:colby/invite-stephanie-morillo, r=segiddins Auto merge of #6627 - agrim123:agr-fix-add-groups, r=deivid-rodriguez Auto merge of #6612 - hdf1986:readme-bundle-add, r=segiddins Auto merge of #6495 - bundler:segiddins/6491-extra-gem-platform-in-lockfile, r=segiddins Auto merge of #6493 - agrim123:agr-update-bundle-update-docs, r=colby-swandale Auto merge of #6310 - utilum:rescue_unspecified_exception, r=segiddins Auto merge of #6184 - arbonap:pa-check-in-gemfile-docs, r=indirect fix typo

theflow force-pushed the unwanted-downgrades branch from f2c8d3b to 71f0a0c Compare September 25, 2018 16:58

greysteil approved these changes Sep 25, 2018

View reviewed changes

greysteil reviewed Sep 25, 2018

View reviewed changes

make only_update_to_newer_versions work correctly with bundle update

21a12c5

theflow force-pushed the unwanted-downgrades branch from 71f0a0c to 21a12c5 Compare September 25, 2018 18:32

ghost merged commit 21a12c5 into rubygems:master Sep 26, 2018

colby-swandale added this to the 1.16.6 milestone Oct 2, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix only_update_to_newer_versions regression#6708

Fix only_update_to_newer_versions regression#6708
1 commit merged intorubygems:masterfrom
theflow:unwanted-downgrades

theflow commented Sep 25, 2018

Uh oh!

ghost commented Sep 25, 2018

Uh oh!

greysteil left a comment

Uh oh!

greysteil Sep 25, 2018

Uh oh!

greysteil Sep 25, 2018

Uh oh!

theflow commented Sep 26, 2018

Uh oh!

greysteil commented Sep 26, 2018

Uh oh!

ghost commented Sep 26, 2018

Uh oh!

greysteil commented Sep 26, 2018

Uh oh!

segiddins commented Oct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

theflow commented Sep 25, 2018

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

What was your diagnosis of the problem?

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

Why did you choose this fix out of the possible options?

Uh oh!

ghost commented Sep 25, 2018

Uh oh!

greysteil left a comment

Choose a reason for hiding this comment

Uh oh!

greysteil Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

greysteil Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

theflow commented Sep 26, 2018

Uh oh!

greysteil commented Sep 26, 2018

Uh oh!

ghost commented Sep 26, 2018

Build succeeded

Uh oh!

greysteil commented Sep 26, 2018

Uh oh!

segiddins commented Oct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants