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
Only add extra resolver spec group for Ruby platform when needed #5698
Conversation
1f33f59
to
84694a1
Compare
9563c7c
to
dd5cc4c
Compare
0fcc194
to
cedd5d6
Compare
We just call it once for resolution, so we can simplify things.
cedd5d6
to
3d5993a
Compare
Wow, nice investigation work. 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the custom matcher, it's much nicer. 👍🏻 Looks good to me generally, and I like the way this simplified a lot of manually generating comparison criteria also.
@@ -224,7 +224,7 @@ def requested_dependencies | |||
|
|||
def current_dependencies | |||
dependencies.select do |d| | |||
d.should_include? && !d.gem_platforms(@platforms).empty? | |||
d.should_include? && !d.gem_platforms([generic_local_platform]).empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the second conditional is now true 100% of the time, right? because generic local platform will always exist, so empty will always be false, so the condition will always be true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the current generic local platform is "ruby" and the dependency is for jruby (like gem "activerecord-jdbcsqlite3-adapter", :platform => :java
), then d.gem_platforms([generic_local_platform])
will be empty (and the dependency will be filtered out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although on the other hand, I think that case is already filtered out by d.should_include?
. Let me run the specs removing that second condition and see what fails (if anything).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When removing the second condition, only one spec fails (although maybe more will fail on Windows/JRuby CI, since I only run specs locally)
rubygems/bundler/spec/runtime/platform_spec.rb
Lines 364 to 379 in dd23fa1
it "allows specifying only-ruby-platform on windows with dependency platforms" do | |
simulate_windows do | |
install_gemfile <<-G | |
source "#{file_uri_for(gem_repo1)}" | |
gem "nokogiri", :platforms => [:mingw, :mswin, :x64_mingw, :jruby] | |
gem "platform_specific" | |
G | |
bundle "config set force_ruby_platform true" | |
bundle "install" | |
expect(the_bundle).to include_gems "platform_specific 1.0 RUBY" | |
expect(the_bundle).to not_include_gems "nokogiri" | |
end | |
end |
However, I'm not sure if that may be due to another issue, or whether the expectation may be wrong.
If that's ok, I'll reticket this to not block this PR on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, totally ok. No need to remove the condition, since my reading on it was wrong anyway!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I'm merging this now then!
Adding duplicated spec groups for both platform specific and ruby specific groups makes the number of candidates duplicate all gems that end up considering both. Our criteria for choosing the next dependency to process is based, among other things, on the number of candidates for a particular dependency, so artificially doubling it causes issues where the wrong dependencies are considered before others. To implement this, we start comparing heterogeneous spec types, and that break a particular case of yanked gems errors, so add a spec to cover that.
3d5993a
to
acdcfb9
Compare
Only add extra resolver spec group for Ruby platform when needed (cherry picked from commit 629e08d)
since rubygems/rubygems#5698 Bundler no longer has a DepProxy class, so we can remove the tweaks for it.
What was the end-user or developer problem that led to this PR?
Our platform resolution logic rely on considering platform specific gems and "RUBY" gems different resolution candidates, because they may have different dependencies.
This works, but duplicates the number of candidates considered for every version, if the version has platform specific gems. This may seem harmless but may completely change resolution logic, because the number of candidates for a given dependency is actually used in order to decide which dependency the resolver should consider next. See
rubygems/bundler/lib/bundler/resolver.rb
Lines 188 to 202 in 342c7f8
and
rubygems/bundler/lib/bundler/resolver.rb
Lines 237 to 259 in 342c7f8
This has caused some cases that worked fine before we started considering platforms no longer resolve, for example, https://github.com/duckinator/rubygems-4539-testcase.
What is your fix for the problem, implemented in this PR?
My fix is to only consider extra resolver groups when the dependencies for the platform specific variants of a particular gem differ from the dependencies of the RUBY variant.
This PR includes some other refactoring that led to figuring out this solution, and was extracted from #5960 to make things more easily reviewable.
Fixes #4539.
Make sure the following tasks are checked