rubygems / bundler

Manage your Ruby application's gem dependencies
https://bundler.io
MIT License
4.88k stars 2k forks source link

Gemfile order should be honored #107

Closed indirect closed 14 years ago

indirect commented 14 years ago

A lot of gems hook into other gems using mechanisms like this:

do_stuff if defined?(ActiveRecord)

Since the gems in question have optional ActiveRecord support, it cannot depend on the AR gem, but must be loaded after the AR gem is loaded. An easy way to make this work would be to load the gems in the order they are listed in the Gemfile.

Note that this needs to work without changing the lockfile load order as fixed in #78.

ghost commented 14 years ago

+1 For this feature!!!!

nesquena commented 14 years ago

+1 For this

yortz commented 14 years ago

2 thumbs up! :)

KieranP commented 14 years ago

+1 Order should also be used when installing. See #15

DAddYE commented 14 years ago

+1 obviously

achiurizo commented 14 years ago

+1

namxam commented 14 years ago

+1 "ThinkingSphinx + delta" are an example where this is preventing us from switching to bundler

rubiii commented 14 years ago

+1

btab commented 14 years ago

+1 (this is absolutely killing us with datamapper)

btab commented 14 years ago

Quick patch to Bundler::Runtime#autorequires_for_groups. Replace...

ordered_deps = []
specs_for(*groups).each do |g|
  dep = @definition.dependencies.find{|d| d.name == g.name }        
  ordered_deps << dep if dep && !ordered_deps.include?(dep)
end

...with its inverse...

ordered_deps = []
spec_names = specs_for(*groups).map { |s| s.name }
@definition.dependencies.each do |dep|
  ordered_deps << dep if spec_names.include?(dep.name) && !ordered_deps.include?(dep)
end
indirect commented 14 years ago

Add a test to show that gemfile order is being honored, and I'll pull it.

DAddYE commented 14 years ago

@btab force is with you!

btab commented 14 years ago

I should note that the above works as a monkeypatch to 0.9.10. However, it relies on unlocked projects as, the moment you lock one, you suffer the random, hash-based, recall from the lock file. Thus our workflow (for now) is going to be to...

[specify all static versions in our Gemfile including bundler 0.9.10]

add the above mokeypatch to our web apps bundle install bundle package bundle unlock git commit

(and obviously just a bundle install on our servers)

indirect commented 14 years ago

You can just bundle cache instead of bundle package; bundle unlock, just so you know.

We would obviously like to fix this for both the locked and unlocked cases, so thanks for the input and I'll keep you guys posted.

pdlug commented 14 years ago

Just wondering if there's an update on the fix btab proposed, I was having the same issue with ordering when running a locked bundle (especially with DataMapper). The patch works for me, if the only thing preventing it from inclusion is some specs I'm happy to help with that.

patchspace commented 14 years ago

+1 here too. Just ran into this with DataMapper where dm-timestamps is getting loaded before dm-core.

dkubb commented 14 years ago

DataMapper is going to fix this problem by making sure that all dm-more plugins require dm-core explicitly. The fact that this isn't working with DataMapper gems is our fault, and should not be blamed on bundler. We will be releasing a fix to this shortly.

snusnu commented 14 years ago

In fact, dm-rails users shouldn't see that issue anymore because it's already patched in my AS compatible dm-forks.

DAddYE commented 14 years ago

Sorry but this issue is not dm related and only some user say about dm. It's a lot of days that this issue is open and we appreciate so much your fastest fixes but ... it's not only a dm suff.

yortz commented 14 years ago

I also think this kind of issue goes way beyond DM: I wish somebody could prove me that I'm wrong:)

btab commented 14 years ago

OK I'll bite. Once dkubb has the DM dependency chains sorted in the manner he describes, bundler will be able to perform the correct topological sort in determining load order for DM. This is excellent news for all DM users (so a big thank you for this).

That said, the reason the Gemfile-order-respecting load process is so important is that there are libraries out there with competitive naming strategies. Only 3 months ago I was working on a project that needed two different gems, both of which defined noticeably different Mash implementations. Depending on the load order, you got one behaviour or the other. While we still have prominent Ruby gems that squat in our apps' global namespaces, we simply can't do without deterministic control of load order.

indirect commented 14 years ago

Hey guys. We're already convinced this is a real need, and this is going to be part of 0.10. Patches are welcome, but telling us you really need it won't make it happen any faster. That said, feel free to keep complaining until it ships. :)

DAddYE commented 14 years ago

Andre,

yep Im sure that you are convinced, my reply was only for @dkubb that after 25 days seems to say: we fixed dm! Close this issue!

Andre very very thanks for your work! /DAddYE

snusnu commented 14 years ago

Of course this issue goes way beyond DM. It's a general problem with buggy upstream gems, not necessarily one for bundler to solve though, imho. This is only a problem because Bundler.require exists - or better put - is used by rails/merb by default, and should obviously handle broken upstream gems. With Bundler.require, people don't need to require the code they use explicitly (of course they can always decide to just leave out Bundler.require and actually do it explicitly, or combine explicit require's with Bundler.require).

I can see how having Bundler.require might be convenient for some, but I'd rather require the code I use explicitly, where i use it. It feels weird that calling gem actually require's code. That's not how ruby(gems) normally behaves. One weird side effect of this is that you have to do something like :require => nil to not require some stuff, even though I'd never expect gem to actually require something in the first place.

So while I actually think that keeping the Gemfile is not important, there are basically 2 situations it could be of help. (Note again that keeping the order is in fact not important right now, as long as you don't rely on Bundler.require, or put the relevant requires before Bundler.require)

1) There's a problem in a gem where it doesn't require all that it needs (as was the case with DM, for example) In this case, the problem should be fixed upstream. (Keep in mind that the same code would break without bundler as well, so how could it be a bundler problem?)

2) A gem does some kind of feature detection, something like:

# inside the LIBRARY
require 'dm-validations' if defined?(DataMapper::Validate)

Here, some code (foo) needs some other code (Rails) to be required already, in order to do its thing.

This can always be solved (more elegantly imho) by telling that code about the environment it runs in, basically, configure it (or, inject a dependency if you will, e.g. from an initializer in your rails app, or early on in the boot process of whatever code).

# early on in CLIENT code
Foo.configure { |config| config.use_dm_validations = true }
# inside the LIBRARY
require 'dm-validations' if Foo.config.use_dm_validations

Note how the latter delays resolving the dependency until the time it's actually used, completely removing the need for the library to have any "foreign" constants already loaded (and thus be subject to potential load order issues, require order should never break code)

So I guess for me it's fine to say that I don't have any problem with the way bundler currently works. In fact, I think this issue should be a #wontfix as this would encourage people to write conceptually cleaner and less brittle code.

snusnu commented 14 years ago

The careful reader will note that at some point i obviously wanted to say "keeping the Gemfile order is not important" instead of just "keeping the Gemfile is not important" :P

DAddYE commented 14 years ago

The careful reader will note that at some point i obviously wanted to say "keeping the Gemfile order is not important" instead of just "keeping the Gemfile is not important"

Hahhaha, delightful ... hilarious ... you changed my day ;)

In fact, I think this issue should be a #wontfix as this would encourage people to write conceptually cleaner and less brittle code. I don't need to rely on gem declaration ordering because I don't use Bundler.require (even if I would, I could always add the troublesome require's before Bundler.require).

Yep because all of us are like you and all of us are DRY lover.

So following for example the collective we need to add gems in Gemfile

# Gemfile
gem 'diff-lcs', '=1.1.2'
gem 'dm-validations', '=0.9.5'
gem 'dm-migrations', '=0.9.5'
gem 'dm-timestamps',  '=0.9.5'
gem 'mailfactory', '=1.4.0'
gem 'merb-action-args', '=0.9.5'
gem 'merb-assets', '=0.9.5'
gem 'merb_exceptions', '=0.1.2'
gem 'merb-haml', '=0.9.5'
gem 'merb_has_flash', '=0.9.2'
gem 'merb_helpers', '=0.9.4'
gem 'merb-mailer', '=0.9.5'
gem 'merb_openid', '=0.0.2'
gem 'merb-parts', '=0.9.5'
gem 'openid_dm_store', '=0.1.1'
gem 'ruby-openid', '=2.1.2'
gem 'RedCloth', '3.0.4'
gem 'tlsmail', '0.0.1'
gem 'vikinggem', '0.0.3'
...
# In my/cool/initializer
require 'diff/lcs'
require 'diff/lcs/hunk'
require 'dm-validations'
require 'dm-migrations'
require 'dm-timestamps'
require 'mailfactory'
require 'merb-action-args'
require 'merb-assets'
require 'merb_exceptions'
require 'merb-haml'
require 'merb_has_flash'
require 'merb_helpers'
require 'merb-mailer'
require 'merb_openid'
require 'merb-parts'
require 'openid_dm_store'
require 'openid'
require 'redcloth'
require 'tlsmail'
require 'viking'
...

Sure, this is conceptually cleaner and prevent all kind of mistakes... yesse! We have Bundler.require ... why use them? ... it's bad!!

Better be explicit and repeat some code...

A small note: for me follow things honored is not a big deal, in fact in my projects was fixed few hours after bundler change this... and of course I can bypass any kind of problem without any show me how.

But not all of us know that Bundle.require didn't respect order, so for me and others if we have a Bundler.require order should be honored like will be honored in a fantastic Initializer.

I prefer don't repeat things ... if possible, and I prefer don't force people to configure everything when things can be simpler.

Different points of view.

ashmoran commented 14 years ago

I agree with DAddYE (at least in principle) - by not preserving the order, it forces every Bundler user to know about this issue. Now you might be able to justify beating gem maintainers with a stick if they have bugs in dependency loading :) But it's not fair to impose that on users of those gems, IMHO.

dkubb commented 14 years ago

@DAddYE: I agree that this issue is not only DM related. That's why I stated that this problem in the context of DM can be solved by us requiring the dependencies explicitly in each DM lib.

Many people in this thread referenced DM specifically, so I wanted to provide some feedback that we have a solution for our specific problem. It does appear that there is an underlying issue that could affect other gems and no general consensus on the best way to deal with it.

DAddYE commented 14 years ago

@dkubb, sorry I was misled by:

The fact that this isn't working with DataMapper gems is our fault, and should not be blamed on bundler.

dkubb commented 14 years ago

Ahh yeah, such is the problem with online communication. If I had properly emphasized the "with" part, or added extra detail about this being something we fixed for DM specifically, perhaps there might not have been a misunderstanding.

Before bundler came around, I would've said that with any core + plugin system, it's mostly obvious that the core system should be required before the plugins. Plugins are meant to extend something, and I think this is true whether or not you're talking about dm-core and dm-more, or ActiveRecord and it's hundreds of plugins.

I don't use Bundler at the moment, so I can't speak on specifics about how it works or does not work; but it makes sense to me that if Bundler is requiring the gems (or provides a way to require the gems) that all things being equal, they should be required in the order specified. Obviously dependencies should be required first, but if two gems are given equal weight, then the tie breaker should be the order they were defined in.

I was curious how Bundler resolves dependencies, since I need to solve a similar problem in DM when saving an object graph with parent/child objects dependent on each other, and it looks like it uses a topological sort on a directed acyclic graph (basically a tree with no circular references). It uses tsort from ruby stdlib, which is the same that I will be using in DM 0.10.3. However, in my case I track the insertion order of each node, and when two or more nodes are given equal weight by the tsort algorithm, I sort those nodes by insertion order. This means parents are created before children, but two independent objects will be created in the order added to the "queue". I think a similar technique could be used in Bundler to ensure order is preserved.

I brought this up in #carlhuda, and was told that it isn't possible due to the way the lockfile uses YAML to serialize the Hash of dependencies. With Ruby 1.8, a Hash loses the order things were added in, so that won't work in this case. My response was to use an ordered map in the YAML for the deps (YAML defines something called an omap, which is basically a Hash that tracks the order things were added). I think Carl or someone else said that this will probably be changing in the next major release, but I can't be certain.

indirect commented 14 years ago

Just to clarify Bundler's official position: as the Roadmap states, 0.10 will change the lockfile format. The new lockfile format will store the order of the serialized dependencies, and this issue will be resolved as fixed.

indirect commented 14 years ago

Coded in f17183b53edc96162cd2e20255d6bc315779b5cf, slated to be part of the 0.10 release.

DAddYE commented 14 years ago

Thanks so so so much!