Fix #2813: Ignore self dependency

[eagletmt]

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

cc: @schneems

[schneems]

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]

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

[schneems]

This looks good to me ❤️ 👍

[gnufied]

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

[indirect]

thanks guys!