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

bundle update --source gem updates conservatively #3759

Closed
neoeno opened this issue Jun 18, 2015 · 10 comments · Fixed by #3763
Closed

bundle update --source gem updates conservatively #3759

neoeno opened this issue Jun 18, 2015 · 10 comments · Fixed by #3763

Comments

@neoeno
Copy link
Contributor

neoeno commented Jun 18, 2015

As noted in #2016, using bundle update --source gem appears to do the same as bundle update gem while refusing to unlock any of the gems' dependencies. It's becoming a more popular technique.

Some people consider this useful behaviour, but the docs don't seem to indicate that this is intended behaviour. The docs say:

––source=<name>

The name of a :git or :path source used in the Gemfile(5). For instance, with a :git source of http://github.com/rails/rails.git, you would call bundle update ––source rails

This would appear to be intended to allow git sources to be unlocked. I reason this way because of the comment and code in definition.rb:

gemfile_sources.each do |source|
  # If the source is unlockable and the current command allows an unlock of
  # the source (for example, you are doing a `bundle update <foo>` of a git-pinned
  # gem), unlock it. For git sources, this means to unlock the revision, which
  # will cause the `ref` used to be the most recent for the branch (or master) if
  # an explicit `ref` is not used.
  if source.respond_to?(:unlock!) && @unlock[:sources].include?(source.name)
    source.unlock!
    changes = true
  end
end

Which is fine. The question is — what does this code do when --source is passed a rubygem rather than a git gem. It does nothing — because lib/bundler/source/rubygems.rb doesn't have #unlock!.

So, by that reasoning, bundle update --source rubygem_gem should be a no-op. But it isn't.

So, I found definition.rb line 529 in #converge_locked_specs:

@locked_specs.each do |s|
  s.source = sources.get(s.source)

  # Don't add a spec to the list if its source is expired. For example,
  # if you change a Git gem to Rubygems.
  next if s.source.nil? || @unlock[:sources].include?(s.name)

When we run bundle update --source rubygem_gem and s.name == 'rubygem_gem'

  1. s.source is not nil
  2. @unlock[:sources] is ['rubygem_gem']

Therefore the condition returns true and the result of all this is that #converge_locked_specs returns a SpecSet that omits rubygem_gem. This means that the gem is effectively unlocked, allowing it to be updated on its own without having any of its dependencies unlocked.

All beyond this line is speculation:

I looked for the commit where this was introduced, and found this commit back in 2010.. It's a little hard to work out what's equivalent, but I found this removal in #converge:

@locked_specs.each do |s|
  if source = @sources.find { |source| s.source == source }
    s.source = source
  else
    @unlock << s.name
  end
end

Which would seem to go through locked specs, try to find their source, and if it can't find it then unlock the spec.

Correspondingly, this addition in #converge_locked_specs:

@locked_specs.delete_if do |s|
  s.source.nil? || (@unlock[:sources] || []).include?(s.name)
end

Which would seem to go through locked specs, and delete them if the source is nil, or if @unlock[:sources] contains... the spec name? Seems unexpected. Shouldn't that be the source name? It could be unintended.


So, I've no strong opinion on whether you should be able to 'update conservatively', but if the functionality is there it should probably be documented as such.

Sorry for the long read! Hope it was clear.

@indirect
Copy link
Member

I was surprised to learn that it worked, but I also didn't write that code, so I can't really tell you whether it was intentional or not. Your code archaeology is top-notch, and I really appreciate how well you documented your question. :)

At this point, removing it would break backwards compatibility, so we're not going to take it out until 2.0. That said, I would strongly prefer to have a documented bundle update --only GEM option rather than a "secret" --source option.

@robinboening
Copy link
Contributor

This is indeed a really nice read of an issue @neoeno.

One of my co-workers showed me this trick to update conservatively with --source yesterday. Wether it was built intentional or not, I find this feature is really enriching bundler. It should be used more often because many people do not know about the risk when running bundle update or even bundle update xyz. Often people do not notice that they updated software dependencies they are not aware of. Especially beginners do not understand how to read the Gemfile.lock. Until they do its just magic happening.

Anyway, I totally agree the name of this "conservatively update" feature should change to something like bundle update --only gem.

@indirect are you sure about not extending the documentation for --source now and pointing out that its name might change in the future?

@indirect
Copy link
Member

indirect commented Jul 1, 2015

Sorry, I guess I wasn't clear. I would love to document it. At the same time, we might as well change the option name to '--only' by adding an alias now, and then when we separate the source and only code paths, they will be documented as separate. :)

@neoeno
Copy link
Contributor Author

neoeno commented Jul 1, 2015

Since we're talking thought I'd drop in an update: started working on the resolution @indirect proposed in #3763 — but ran into another strange result when using --source. Looks like it might not always update conservatively as is unofficially expected. Don't trust that yet though — I'll get details over the next couple days.

@neoeno
Copy link
Contributor Author

neoeno commented Jul 12, 2015

So the behaviour of bundle update --source gem is a a little more subtle than detailed above. Here's my updated understanding:

Passing a gem to the source option will update a gem and all of its dependencies except those that are specified elsewhere in the Gemfile or are depended on by other gems.

In practice, in a project above a given level of complexity (e.g. most Rails projects) any given gem is likely to appear somewhere else in the dependency graph — so will remain locked.

I'm finishing up some changes to #3763 that include tests for this behaviour.

@indirect
Copy link
Member

awesome, thanks!

On Sun, Jul 12, 2015 at 1:30 PM, Caden Lovelace [email protected]
wrote:

So the behaviour of bundle update --source gem is a a little more subtle than detailed above. Here's my updated understanding:
Passing a gem to the source option will update a gem and all of its dependencies except those that are specified elsewhere in the Gemfile or depended on by other gems.
In practice, in a project above a given level of complexity (e.g. most Rails projects) any given gem is likely to appear somewhere else in the dependency graph — so will remain locked.

I'm finishing up some changes to #3763 that include tests for this behaviour.

Reply to this email directly or view it on GitHub:
#3759 (comment)

homu added a commit that referenced this issue Aug 24, 2015
Fix `--source` option causing incorrect gem unlocking

Fixes #3759 and #3761.

And also another issue I found where if:

1. You have a Gemfile with a group,
2. That group contains a gem with the same name as a source
3. And you run `bundle update --group group_name`
4. Then all gems associated with the source will be updated.

The 3 cases fixed here are all pretty subtle. The specs should make it clearer, and the github issues above add more context.

Note that this PR removes the ability to use `bundle update --source gem_name` as a hacky way to update a gem 'conservatively'. This is detailed in #3759.
@chrismo
Copy link
Contributor

chrismo commented May 19, 2016

I've written up, with some code samples, more details on the stuff @neoeno dug up, if anyone is curious: https://github.com/chrismo/bundler-source-hack

@indirect
Copy link
Member

@chrismo I'm pretty interested in the possibility of bundle update acting more like bundle patch. We're planning changes for Bundler 2.0 right now, and this seems like a good candidate. Would you be interested in talking more about it? I can invite you to the Bundler Slack.

@chrismo
Copy link
Contributor

chrismo commented May 20, 2016

Sure thing 👍

@timolehto
Copy link

@indirect, regarding your comment about update --only option #3759 (comment), would you be willing/interested to merge/see a PR for such a feature? Or is there already some awesome features on the pipeline for 2.0 regarding this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants