thought-driven / bummr

Update your gems in separate commits. Identify any that fail your build.
MIT License
234 stars 23 forks source link

Consider upgrading only gems listed in Gemfile #7

Closed monfresh closed 8 years ago

monfresh commented 8 years ago

Thanks for this very useful tool.

When I update gems, I usually only update the ones listed in the Gemfile, which is what Gemnasium uses to report outdated gems. I noticed that bummr uses Bundler.load.specs, which lists a whole lot more than what's in the Gemfile, and in some cases gems that aren't even in the Gemfile.lock. Is there a particular reason for that decision?

Would it be possible to provide some sort of configuration to allow the user to choose to update only the gems listed in the Gemfile?

Updating only the files listed in the Gemfile should automatically update the dependencies of those gems, and should make bummr run faster since the list of gems it iterates through will be smaller.

Thanks!

monfresh commented 8 years ago

I did some more debugging and came to the conclusion that all_gem_specs should only return Bundler.load.dependencies. I'm not sure why you are also including Bundler.load.specs. What was the reasoning behind that?

One of the reasons why you don't want to include the gems in the Gemfile.lock is because they are likely to change while you are updating the gems. For example, here is what resulted in the error I reported here:

I had flamegraph version 0.1.0, which lists fast_stack as its runtime dependency. fast_stack in turn was at version 0.1.0, which has rack-compiler as a runtime dependency. Because bummr includes all files in the Gemfile.lock, it assumes that both fast-stack and rack-compiler need to be updated.

When Bummr updated flamegraph, it was upgraded to version 0.9.5. This latest version no longer has a dependency on fast_stack, which means that both fast_stack and rake-compiler have now been removed from the Gemfile.lock.

So, when Bummr tries to get the updated_version from bundle list for rake-compiler, it crashes because that gem is not there anymore.

This means that the only gems Bummr should try to update are the ones listed in the Gemfile.

lpender commented 8 years ago

Hey,

Thanks for the report. That part of the code was essentially pulled from Bundler's source code. I'm going to take a look and see what I can do.

monfresh commented 8 years ago

I have a fix. I'll submit a PR in a bit.

lpender commented 8 years ago

It looks like what we're looking for may be bundle outdated --strict https://github.com/bundler/bundler/issues/2575

lpender commented 8 years ago

@monfresh I couldn't help myself, been working on my own fix here: https://github.com/lpender/bummr/pull/11

It removes all of the code that access Bundler internally, instead relying on regexing the output of bundler outdated --strict. I think this will be more stable and easier to maintain if Bundler has internal code churn.

Unfortunately bundler outdated --strict is not documented It doesn't explicitly mention in the documentation whether bundler outdated --strict includes dependencies from the Gemfile.lock, but analyzing the statement: "outdated gems that match the dependency requirements", you can infer that any gem that is able to have dependency requirements would be in the Gemfile.

Based on my perfunctory tests and https://github.com/bundler/bundler/issues/2575, it appears to be updating only gems in the Gemfile that are not locked to a specific version.

lpender commented 8 years ago

I'm going to go ahead and merge my pull request, since I'm concerned that other users of the app may be having a poor experience. Please let me know if you come up with an alternate method that you feel is better.

lpender commented 8 years ago

Did some more testing -- looks like it is still trying to update gems that are not in the Gemfile :/

lpender commented 8 years ago

https://github.com/lpender/bummr/pull/12

lpender commented 8 years ago

Upgrading to 0.1.1 should now work for you.

The list of gems to update went from ~90 to 32 on my test fixture so you should also gain quite a bit of speed.

monfresh commented 8 years ago

Thanks, I'll try it out. For your test fixture, I recommend you set it up so that you can test what happens when a gem removes a dependency after being updated, like what happened in my case with flamegraph.

lpender commented 8 years ago

I fixed the regex and added rspec and some basic unit tests in v0.1.2.

Unfortunately, integration tests are going to be an issue until this issue is resolved.

Take a look at https://github.com/lpender/bummr/compare/lp-bdd if you'd like to take a crack at the integration tests yourself. Maybe I'm doing something wrong.

lpender commented 8 years ago

@monfresh I'm closing this again for now. Let me know if you have any more issues.

monfresh commented 8 years ago

Thanks! The latest version looks great, but I found another issue with the commit message. I'll open a new issue now.