Conservative updates #4676

chrismo · 2016-06-16T22:36:47Z

A port of bundler-patch to bundler proper.

Core team questions:

bundler-patch also works in vulnerable gem updates based on ruby-advisory-db content. I presume the core team would consider this out of scope for Bundler proper? I probably would, and there's no reason bundler-patch couldn't continue to exist as a plugin with this behavior.
"resolves foo only to latest patch - changing dependency case" - more comments on that spec. It's a weird edge case. Hopefully the specs and comments make it clear.
Name of new class: GemVersionPromoter. It's gone through a few renames already and every option I think of I hate now :)
The --minimal flag is also something bundler-patch has that has debatable value. Would love to hear opinions on it and could be easily swayed to remove it or keep it. If I'm on 1.0.1, and both 1.0.2 and 1.0.15 exist, --minimal will resolve to 1.0.2 instead of the latest 1.0.15. Dunno if there's value in that.
even without --strict mode, this code will remove older versions from the options handed back to Molinillo (in --patch and --minor cases). This is a weird case, but I have seen bundle update proper regress a gem version backwards if its parent gem changed to an older dependency (I guess in a bug fix case). I'm ok with this, but it could be considered an inconsistency with the default --major case.
if we roll this out first as an undocumented feature, do we require a config option to toggle it on? We wouldn't have to have one, none of the new behavior will kick in until a new flag (--patch or --minor) is passed.

Notes for issues to create that could be handled separately after this PR, esp. if we release undocumented/unsupported to begin with.

Updating outdated to use the new flags and resolution
adding some warnings when dependent updates go past --patch or --minor level (with an instruction to use --strict if they don’t want that)
adding a warning when an unlocked gem doesn't move (possibly w/ a reason why?). the lack of feedback otherwise can be disconcerting, like "did I not do it right?"
possibly adding a dry-run flag - though after looking at that today, that would make the most sense inside Installer, and make that flag an option for both install and update (drilling through all of the calls to push that option in looks like it would be annoying).
man page documentation

segiddins · 2016-06-17T00:36:12Z

lib/bundler/cli/update.rb

@@ -37,6 +37,9 @@ def run
        Bundler.definition(:gems => gems, :sources => sources, :ruby => options[:ruby])
      end

+      patch_level = [:major, :minor, :patch].detect { |v| options.keys.include?(v.to_s) }


What if multiple are specified?

should favor left to right (maj, min, pat) ... specifying multiple wouldn't make sense internally. dunno if it should favor the last one specified by the user? e.g. --minor --patch => --patch vs. --patch --minor => --minor

chrismo · 2016-06-17T21:49:29Z

After rubocop, seeing good test results. 1 failing that's intentional on my part, 2 that seem unrelated.

chrismo · 2016-06-17T22:05:28Z

spec/resolver/basic_spec.rb

+      should_consv_resolve_and_include :patch, ["foo"], %w(foo-1.3.8 bar-2.0.3)
+    end
+
+    it "resolves foo only to latest patch - changing dependency case" do


just thinking through this - here's the potential design flaw. In all other cases, strict means: ensure no unlocking gem gets past the patch/minor threshold. And accordingly, not specifying strict means for unlocking gems, if one of them has to go past the patch/minor threshold to satisfy the dependency tree, then ... ok.

But in THIS case, the 'not specifying strict' case means for unlocking gems and their dependencies ... and thus having to use strict to get the expected behavior is a debatable flaw.

Compare this case to the prior spec: "resolves foo only to latest patch - same dependency case" - that should highlight the inconsistency.

I'm not sure there's an easy code fix for this case, though.

chrismo · 2016-06-17T22:07:08Z

Another discussion point - bundler-patch also works in vulnerable gem updates based on ruby-advisory-db content. I presume the core team would consider this out of scope for Bundler proper? I probably would, and there's no reason bundler-patch couldn't continue to exist as a plugin with this behavior.

chrismo · 2016-06-17T22:08:50Z

spec/resolver/basic_spec.rb

+      # versions and I'd rather go from 1.0.2 to 1.0.3 instead of 1.0.45. But, we could consider killing the
+      # minimal option altogether. If that's what you need, use the Gemfile dependency.
+      should_consv_resolve_and_include [:minor, :minimal], [], %w(foo-1.4.4 bar-2.0.4)
+    end


The --minimal flag is also something bundler-patch has that has debatable value. Would love to hear opinions on it and could be easily swayed to remove it or keep it.

homu · 2016-06-19T05:16:53Z

☔ The latest upstream changes (presumably #4618) made this pull request unmergeable. Please resolve the merge conflicts.

segiddins · 2016-06-20T17:36:01Z

lib/bundler/definition.rb

@@ -94,7 +94,7 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti
      end
      @unlocking ||= @unlock[:ruby] || (!@locked_ruby_version ^ !@ruby_version)

-      @gem_version_promoter = GemVersionPromoter.new(@locked_specs, @unlock[:gems])
+      create_gem_version_promoter


for testing purposes, maybe make the assignment here instead of in create_gem_version_promoter ?

Do you mean inline #create_gem_version_promoter? constructor already feels rather fat to me - I wanted to encapsulate that in its own method with the additional logic to re-capture the locked_specs. I tried to find a way to re-use the @locked_specs or locked local var rather than re-executing, but it didn't seem clean to me with the various conditionals.

But if that's not what you mean, I'm not understanding your comment ... ?

no, I mean have @gem_version_promoter = create_gem_version_promoter

oh right - yeah, I'm down with that.

indirect · 2016-06-21T05:31:25Z

ruby-advisory-db: sounds like a great plugin. :)
changing dependency spec: I buy your argument based on the docs that if it's not in the Gemfile (and you don't pass any restriction options), you're implicitly declaring that you don't care about how it updates. This seems okay to me.
GemVersionPromoter: works for me.
--minimal flag: let's drop it and see if the masses demand it. if so we can always add it. :)

segiddins · 2016-06-21T14:17:45Z

lib/bundler/gem_version_promoter.rb

+          sort_dep_specs(dep_specs, locked_spec)
+        end.tap do |specs|
+          if ENV["DEBUG_PATCH_RESOLVER"] # MODO: proper debug flag name, check and proper debug output
+            STDERR.puts before_result


please use the Bundler.ui debug methods here

will do. I need to revisit that flag too, perhaps re-use DEBUG_RESOLVER.

chrismo · 2016-06-21T14:20:25Z

@indirect thx for the feedback - added a couple of more to the checklist

segiddins · 2016-06-21T14:31:23Z

No need for an explicit feature flag, since the new behavior is still opt-in via the CLI options
I think in strict mode, we might want to >= the current version in the locked deps handed to molinillo, if that answers your question?

segiddins · 2016-06-21T16:40:28Z

lib/bundler/cli.rb

@@ -206,6 +206,12 @@ def install
      "Update ruby specified in Gemfile.lock"
    method_option "bundler", :type => :string, :lazy_default => "> 0.a", :banner =>
      "Update the locked version of bundler"
+    method_option "patch", :type => :boolean, :banner =>
+      "Prefer updating only to next patch version"


isn't this only allow updating gems' patch versions ?

there's a --strict flag I hadn't added here yet I just realized ...

If --strict is also used, then the code will actually remove versions fed back to Molinillo, but that can result in no updates happening or VersionConflict cases that report a version can't be found that actually does exist in the index, causing a very weird "looks like a bug" experience. (hmmm ... while I did see VersionConflict cases early on, I haven't seen one in a while ... I may need to try and recreate that in a spec -- maybe that was due to an early bug and shouldn't be possible).

So, not using --strict is the default and it means the code will only sort to the front (not filter out) the patch versions first - which favors getting something done, but it's potentially not as conservative.

One challenge still existing is trying to give better output in these conservative cases, because sometimes no change will happen at all and there's no feedback to the user trying to explain "well, you wanted to go up to 1.8.3 from 1.8.2, but 1.8.3 needs a minor increment and you fed it --strict so it couldn't do anything" - but I've no idea if that's even plausible. It wouldn't be too unlike the Conflict output ... but these aren't cases of a Conflict.

I did think also there could be a way to detect increments beyond what was requested and warn those somehow.

#pascalsapology

So, not using --strict is the default and it means the code will only sort to the front (not filter out) the patch versions first - which favors getting something done, but it's potentially not as conservative.

AHHH, this was the whole piece I was missing, sorry

no worries - option isn't listed there yet.

indirect · 2016-06-21T19:49:45Z

@chrismo: we want to stop allowing update to downgrade gems, so removing downgrades from new flags seems good to me. since this is only new flags, no need for a feature flag either.

chrismo · 2016-06-21T19:59:17Z

i imagine it's a rare case, but if i need to bugfix foo with a little new code and fall back to an older version of dependent gem bar ... if we always eliminate older gems, there'd be no way to resolve that.

chrismo · 2016-06-21T22:21:05Z

Thinking on this more - I think the best balance is: (a) do NOT filter older versions from the non-strict case, and (b) remove older versions from the --strict case. Otherwise, there's no way to resolve the rare but (IMO) legit case of regressing a dependent gem.

chrismo · 2016-06-21T22:22:54Z

(For a real life case, bundler-patch itself started with slop v. 4, but that conflicted with any bundle including pry, which uses slop v. 3. So I released a newer version of bundler-patch that required an older version of slop).

chrismo · 2016-06-22T04:35:22Z

There's also an existing test that tests a regression: https://github.com/bundler/bundler/blob/master/spec/commands/install_spec.rb#L155-L168 - from activesupport 2.3.5 back to 2.3.2

Getting caught up on missing update_specs, realized `--strict` flagged wasn't being passed through. Fixed.

When I ported over specs from bundler-patch, I had a TODO on a combination that had no specs, so that's taken care of now.

@segiddins

This is the first behavior change from bundler-patch. Used to be older versions would never be an option, but Bundler proper has always supported this (if necessary to resolve the dependency tree) and there can be some legit cases for doing this. The `--strict` flag could be used to override this behavior, but I'm running into a Molinillo behavior that I'm not sure is correct, so the specs involving the strict option are failing right now. I'm going to push so @segiddins and I can discuss.

Also some pending specs cleanup. These may be added as new issues and addressed later.

segiddins and I are agreed there appears to be an issue with Molinillo in these cases, but they are unusual and for now we're looking to do an undocumented release of the new conservative updates, so we can start to get feedback. We'll revisit these cases.

chrismo · 2016-07-09T02:28:36Z

I think I found the problem here. Should have commits soon.

Not a common case, but glitch is triggered by new plugin integration tests when there's no Gemfile or .bundle directory. A GemfileNotFound exception is raised deeper down the call stack trying to access the cache_path when executed in a non-bundler dir. That case apparently is legit for plugins.

chrismo · 2016-07-09T05:20:59Z

@indirect or @segiddins - can you retry the auto-merge?

segiddins · 2016-07-09T11:36:16Z

@homu r+

homu · 2016-07-09T11:36:17Z

📌 Commit 263c187 has been approved by segiddins

homu · 2016-07-09T11:36:30Z

⚡ Test exempted - status

Conservative updates A port of bundler-patch to bundler proper. Core team questions: - [x] bundler-patch also works in vulnerable gem updates based on ruby-advisory-db content. I presume the core team would consider this out of scope for Bundler proper? I probably would, and there's no reason bundler-patch couldn't continue to exist as a plugin with this behavior. - [x] "resolves foo only to latest patch - changing dependency case" - more comments on that spec. It's a weird edge case. Hopefully the specs and comments make it clear. - [x] Name of new class: GemVersionPromoter. It's gone through a few renames already and every option I think of I hate now :) - [x] The `--minimal` flag is also something bundler-patch has that has debatable value. Would love to hear opinions on it and could be easily swayed to remove it or keep it. If I'm on 1.0.1, and both 1.0.2 and 1.0.15 exist, `--minimal` will resolve to `1.0.2` instead of the latest `1.0.15`. Dunno if there's value in that. - [x] even without `--strict` mode, this code will _remove_ older versions from the options handed back to Molinillo (in `--patch` and `--minor` cases). This is a weird case, but I have seen `bundle update` proper regress a gem version backwards if its parent gem changed to an older dependency (I guess in a bug fix case). I'm ok with this, but it could be considered an inconsistency with the default `--major` case. - [x] if we roll this out first as an undocumented feature, do we require a config option to toggle it on? We wouldn't have to have one, none of the new behavior will kick in until a new flag (`--patch` or `--minor`) is passed. --- Notes for issues to create that could be handled separately after this PR, esp. if we release undocumented/unsupported to begin with. - Updating `outdated` to use the new flags and resolution - adding some warnings when dependent updates go past --patch or --minor level (with an instruction to use --strict if they don’t want that) - adding a warning when an unlocked gem doesn't move (possibly w/ a reason why?). the lack of feedback otherwise can be disconcerting, like "did I not do it right?" - possibly adding a dry-run flag - though after looking at that today, that would make the most sense inside Installer, and make that flag an option for both install and update (drilling through all of the calls to push that option in looks like it would be annoying). - man page documentation

chrismo · 2016-07-09T14:27:01Z

w00t - thx :)

segiddins reviewed Jun 17, 2016
View reviewed changes

chrismo mentioned this pull request Jun 17, 2016

Port behavior to bundler 2 livingsocial/bundler-patch#13

Closed

7 tasks

chrismo reviewed Jun 17, 2016
View reviewed changes

chrismo force-pushed the conservative_updates branch 2 times, most recently from b0a245d to 5d87a15 Compare June 20, 2016 13:46

segiddins reviewed Jun 20, 2016
View reviewed changes

chrismo mentioned this pull request Jun 20, 2016

Minimal bundle install #4690

Closed

segiddins reviewed Jun 21, 2016
View reviewed changes

chrismo added 8 commits July 8, 2016 19:35

Mark new options as hidden

f37c76f

Hook-up --strict option, flesh out update specs.

3d1e6e3

Getting caught up on missing update_specs, realized `--strict` flagged wasn't being passed through. Fixed.

Remove old pending spec

57d70be

Add some additional GVP specs per TODO.

257c8b2

When I ported over specs from bundler-patch, I had a TODO on a combination that had no specs, so that's taken care of now.

Add docs to GemVersionPromoter.

3e56e8f

Also some pending specs cleanup. These may be added as new issues and addressed later.

Raise message on multiple options

9022339

chrismo force-pushed the conservative_updates branch from 1b3d14d to 263c187 Compare July 9, 2016 03:45

homu merged commit 263c187 into rubygems:master Jul 9, 2016

This was referenced Jul 9, 2016

[RubygemsIntegration] Add support for reversing hooks #4770

Merged

[WIP]Add pristine command and spec #4729

Closed

chrismo deleted the conservative_updates branch July 9, 2016 14:29

This was referenced Aug 23, 2016

Conservative Bundle Updates rubygems/bundler-features#122

Closed

Update bundle lock with conservative update behavior/options #4912

Closed

coilysiren modified the milestone: Release Archive Sep 22, 2016

chrismo mentioned this pull request Mar 14, 2020

Add warnings on bundle update when dependent updates go past --patch or --minor level rubygems/rubygems#3180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conservative updates #4676

Conservative updates #4676

chrismo commented Jun 16, 2016 •

edited

segiddins Jun 17, 2016

chrismo Jun 17, 2016

chrismo commented Jun 17, 2016

chrismo Jun 17, 2016

chrismo commented Jun 17, 2016

chrismo Jun 17, 2016

homu commented Jun 19, 2016

segiddins Jun 20, 2016

chrismo Jun 20, 2016

segiddins Jun 20, 2016

chrismo Jun 20, 2016

indirect commented Jun 21, 2016

segiddins Jun 21, 2016

chrismo Jun 21, 2016

chrismo commented Jun 21, 2016

segiddins commented Jun 21, 2016

segiddins Jun 21, 2016

chrismo Jun 21, 2016

segiddins Jun 21, 2016

chrismo Jun 21, 2016

indirect commented Jun 21, 2016

chrismo commented Jun 21, 2016

chrismo commented Jun 21, 2016

chrismo commented Jun 21, 2016

chrismo commented Jun 22, 2016

chrismo commented Jul 9, 2016

chrismo commented Jul 9, 2016

segiddins commented Jul 9, 2016

homu commented Jul 9, 2016

homu commented Jul 9, 2016

chrismo commented Jul 9, 2016

Conservative updates #4676

Conservative updates #4676

Conversation

chrismo commented Jun 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismo commented Jun 17, 2016

Choose a reason for hiding this comment

chrismo commented Jun 17, 2016

Choose a reason for hiding this comment

homu commented Jun 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indirect commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismo commented Jun 21, 2016

segiddins commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indirect commented Jun 21, 2016

chrismo commented Jun 21, 2016

chrismo commented Jun 21, 2016

chrismo commented Jun 21, 2016

chrismo commented Jun 22, 2016

chrismo commented Jul 9, 2016

chrismo commented Jul 9, 2016

segiddins commented Jul 9, 2016

homu commented Jul 9, 2016

homu commented Jul 9, 2016

chrismo commented Jul 9, 2016

chrismo commented Jun 16, 2016 •

edited