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

Remove satisfied dependecies from next source lookup #3200

Closed
wants to merge 2 commits into from
Closed

Remove satisfied dependecies from next source lookup #3200

wants to merge 2 commits into from

Conversation

dubek
Copy link
Contributor

@dubek dubek commented Oct 6, 2014

When iterating sources and checking for met dependencies, remove the
actual specs that were satisified by the source from the lookup of
following sources.

This prevents lookup of local gems (from gemspec or path or git
sources) in rubygems source.

fixes #2909

When iterating sources and checking for met dependencies, remove the
actual specs that were satisified by the source from the lookup of
following sources.

This prevents lookup of local gems (from `gemspec` or `path` or `git`
sources) in rubygems source.

fixes #2909
@grosser
Copy link
Contributor

grosser commented Oct 6, 2014

👍 Looks about right :)

@@ -203,6 +203,7 @@ def index
sources.all_sources.each do |s|
s.dependency_names = dependency_names
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should dup dependency_names, or else each source will be aliasing the instance of the list that gets changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll add it too to that fix.

Note that we "suffer" from the same issue today (on master). Can you think of a test case to reveal this issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a second source contains the same gem, I guess? So like two git sources that both contain the gem "foo". Bundler should warn about that, I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be totally reasonable for two Rubygems sources to contain the same gem.

I'm still not sure whether this whole change could potentially break something there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a gem can appear in both Rubygems sources and we should consider both of them, then the original bug report #2909 in which Bundler requests for a private gem from a public Rubysgems source is not a bug (though I still see an "private information leak" problem in that case; maybe we need a specific :private=>true mark for some gems, which will make sure we don't look for them in other Rubygems sources). Maybe we should move the discussion back to the original issue #2909 and decide what's the appropriate solution (might require new Gemfile syntax?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting the gem should (theoretically) be fixed by using a source block. Then the gems in that source block should be requested from that source, and gems outside that source block should not be requested from that source. However, dependencies of that gem will still be requested from other sources.

On Oct 13, 2014, at 7:50 AM, Dov Murik [email protected] wrote:

In lib/bundler/definition.rb:

@@ -203,6 +203,7 @@ def index
sources.all_sources.each do |s|
s.dependency_names = dependency_names
If a gem can appear in both Rubygems sources and we should consider both of them, then the original bug report #2909 in which Bundler requests for a private gem from a public Rubysgems source is not a bug (though I still see an "private information leak" problem in that case; maybe we need a specific :private=>true mark for some gems, which will make sure we don't look for them in other Rubygems sources). Maybe we should move the discussion back to the original issue #2909 and decide what's the appropriate solution (might require new Gemfile syntax?).


Reply to this email directly or view it on GitHub.

@dubek
Copy link
Contributor Author

dubek commented Oct 7, 2014

I added a fix for the missing dup as per @TimMoore advice.

@indirect - I don't have an idea for a Gemfile that will cause that missing dup to cause trouble (note that we have it already before my original fix for issue #2909). Right now we might have several sources point to the same (mutuable) dependency_names array, but they don't really use it except when called for #specs on the first time.

@indirect
Copy link
Member

Backported to 1.7.4 as 1eb62f2 and 2f17e3e.

@indirect indirect closed this Nov 29, 2014
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 this pull request may close these issues.

do not request gems declared in gemspec from rubygems
4 participants