Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Inconsistency with bundle update (major vs minor/patch) #4934

Closed
marcandre opened this issue Aug 30, 2016 · 5 comments
Closed

Inconsistency with bundle update (major vs minor/patch) #4934

marcandre opened this issue Aug 30, 2016 · 5 comments

Comments

@marcandre
Copy link

Here's a test case aiming to illustrate the inconsistency with bundle update foo with different --major/minor/patch. The only available gem versions are patches releases, so all three options should all behave the same but they don't for the minor and patch options.

I feel this inconsistency has to be bug. For sure, it makes it impossible to understand what is supposed to be going on.

Moved from chrismo/bundler-cases#6

@marcandre
Copy link
Author

marcandre commented Aug 30, 2016

FWIW, I strongly disagree with bundler update foo changing needlessly bar's version (see #2016), so I'm kind of shooting myself in the foot here by pointing this out 😢

My christmas wish is that bundle update does change whatever you want, but that bundler makes an incompatible change for bundle update a_particular_gem and mimicks the new behavior of --minor/patch instead of bringing those inline with the current behavior.

One of the main reason against this was that it would "hard to do" or "hard to get right". With the new logic in place with --minor/patch, it's already done!

If I can't get my christmas wish, I am still hoping for a way to do bundler update foo --whatever-options-you-d-like-me-to-type that won't change bar (no matter what / unless necessary, pick your favorite).

@indirect
Copy link
Member

@marcandre that is my understanding of our actual plan for Bundler 2.0: make the logic of --minor and --patch the default. We're not sure if we'll manage to ship it in time for it to be your christmas present, though. 😆

@marcandre
Copy link
Author

OMG! Awesome!

@chrismo
Copy link
Contributor

chrismo commented Sep 9, 2016

I think in conjunction with the desire for a --conservative flag** (great name, hate typing it - too long?) ... we can have a little 1.x cake and eat it too here.

(** https://github.com/chrismo/bundler-cases/blob/master/cases/conservative/update/conservative_like_install.rb)

Also added PR with your case => chrismo/bundler-cases#12

chrismo added a commit to chrismo/bundler-cases that referenced this issue Sep 9, 2016
This case was failing to illustrate the desired effect, but that sort of
goes against the grain here. Anyway - more comments with thoughts and
how this could help play nice with the bug fix for
rubygems/bundler#4934
@chrismo
Copy link
Contributor

chrismo commented Sep 13, 2016

ha - this may be all that's needed to fix this:

--- a/lib/bundler/definition.rb
+++ b/lib/bundler/definition.rb
@@ -103,8 +103,6 @@ module Bundler
       end
       @unlocking ||= @unlock[:ruby] ||= (!@locked_ruby_version ^ !@ruby_version)

-      @gem_version_promoter = create_gem_version_promoter
-
       current_platform = Bundler.rubygems.platforms.map {|p| generic(p) }.compact.last
       add_platform(current_platform)

@@ -112,6 +110,8 @@ module Bundler
       eager_unlock = expand_dependencies(@unlock[:gems])
       @unlock[:gems] = @locked_specs.for(eager_unlock).map(&:name)

+      @gem_version_promoter = create_gem_version_promoter
+
       @source_changes = converge_sources
       @dependency_changes = converge_dependencies
       @local_changes = converge_locals

which makes perfect sense - create_gem_version_promoter passes through @unlock[:gems] to the GemVersionPromoter instance. It should really do that after the Definition is done setting it up.

chrismo added a commit to chrismo/bundler-cases that referenced this issue Sep 13, 2016
Case will fail against 1.13.0, bug fix in
rubygems/bundler#4977.
chrismo added a commit to chrismo/bundler-cases that referenced this issue Sep 13, 2016
Case will fail against 1.13.0, bug fix in
rubygems/bundler#4977.
homu added a commit that referenced this issue Sep 13, 2016
Fix #4934. Make GVP _after_ eager unlock.

When Definition creates the GemVersionPromoter, it needs to do so
_after_ it's performed the eager_unlock so the GVP gets the correct list
of unlocked gems.

Prior to this fix, it had a stricter list of gems being updated with the
new `--patch` or `--minor` options used with `bundle update` and in some
cases would have inconsistent results when used without a conservative
switch or the `--major` option. See #4934 for details.
homu added a commit that referenced this issue Sep 13, 2016
Fix #4934. Make GVP _after_ eager unlock.

When Definition creates the GemVersionPromoter, it needs to do so
_after_ it's performed the eager_unlock so the GVP gets the correct list
of unlocked gems.

Prior to this fix, it had a stricter list of gems being updated with the
new `--patch` or `--minor` options used with `bundle update` and in some
cases would have inconsistent results when used without a conservative
switch or the `--major` option. See #4934 for details.
@homu homu closed this as completed in 5323646 Sep 13, 2016
indirect pushed a commit that referenced this issue Oct 11, 2016
Fix #4934. Make GVP _after_ eager unlock.

When Definition creates the GemVersionPromoter, it needs to do so
_after_ it's performed the eager_unlock so the GVP gets the correct list
of unlocked gems.

Prior to this fix, it had a stricter list of gems being updated with the
new `--patch` or `--minor` options used with `bundle update` and in some
cases would have inconsistent results when used without a conservative
switch or the `--major` option. See #4934 for details.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants