Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2813: Ignore self dependency #2817

Merged
merged 2 commits into from Jan 10, 2014

Conversation

Projects
None yet
4 participants
@eagletmt
Copy link
Contributor

eagletmt commented Jan 10, 2014

Now it ignores self dependency on ready-to-install check.

cc: @schneems

@schneems

This comment has been minimized.

Copy link
Contributor

schneems commented Jan 10, 2014

Thank you and @gnufied for looking at this with me. I now understand much better why my case didn't work, there were parts of the code that I was mis-interpreting.

This PR works for my test case, though I have some minor comments to make this code more maintainable and approachable in the future. This code wass somewhat confusing to me:

def ready_to_install?(spec, remains)
  spec.dependencies.none? do |dep|
    remains[dep.name] && dep.type != :development && dep.name != spec.name
  end
end

We check all dependencies and only install if none of them are a circular dependency and a runtime dependency and also in the remains list. I recommend a change to the ready_to_install? method as well as a comment explaining it's behavior

# We only want to install if a gem spec if all its dependencies are met.
# If the dependency is no longer in the `remains` hash then it has been met.
# If a dependency is only development or is self referential it can be ignored.
def ready_to_install?(spec, remains)
  spec.dependencies.none? do |dep|
    next if dep.type == :development || dep.name == spec.name
    remains[dep.name]
  end
end

This using == and || is easier to understand to me than != and &&. The code in this case i feel better shows the intention of what we are trying to accomplish.

Since we're using this code to be called multiple times, it should only be written once to avoid changes only going into one and not the other (I didn't even see it there inside of the until loop). You could put it into a method, but I think a cleaner solution is to use another lambda so you don't have to pass reference to remains, enqueued, name2spec, and worker_pool:

# Keys in the remains hash represent uninstalled gems specs.
# We enqueue all gem specs that do not have any dependencies.
# Later we call this lambda again to install specs that depended on
# previously installed specifications. We continue until all specs
# are installed.
enqueue_remaining_specs = lambda {
  remains.keys.each do |name|
    next if enqueued[name]
    spec = name2spec[name]
    if ready_to_install?(spec, remains)
      worker_pool.enq name
      enqueued[name] = true
    end
  end
}
enqueue_remaining_specs.call

until remains.empty?
  message = worker_pool.deq
  remains.delete message[:name]
  if message[:post_install]
    Installer.post_install_messages[message[:name]] = message[:post_install]
  end
  enqueue_remaining_specs.call
end

What do you think?

@eagletmt

This comment has been minimized.

Copy link
Contributor Author

eagletmt commented Jan 10, 2014

@schneems Agreed. I've updated as you commented. Thank you.

@schneems

This comment has been minimized.

Copy link
Contributor

schneems commented Jan 10, 2014

This looks good to me ❤️ 👍

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Jan 10, 2014

Looks good here as well. @schneems & @eagletmt thank you for reporting and fixing this. :-)

@indirect

This comment has been minimized.

Copy link
Member

indirect commented Jan 10, 2014

thanks guys!

indirect added a commit that referenced this pull request Jan 10, 2014

@indirect indirect merged commit 27205e9 into bundler:master Jan 10, 2014

1 check failed

default The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.