Replace Molinillo with PubGrub for dependency resolution#9402
Replace Molinillo with PubGrub for dependency resolution#9402mlarraz wants to merge 26 commits intoruby:masterfrom
Conversation
e8b7fb1 to
32a966d
Compare
|
I appreciate the effort here, but I should flag that I was already actively working on this. I opened the issue, I'm assigned to it, and I've had a WIP implementation going for several weeks now. That said, I don't want to be a gatekeeper, but I'm not exactly sure what the path forward here is. |
Makes sense. Not sure what your version looks like but feel free to use any ideas from mine if it helps. I would just like the change to get done. |
|
@colby-swandale Could you open a pull request with your current WIP branch? Even if it's still in progress, having it visible will help us coordinate. Once your PR is up, we can compare both implementations and incorporate any useful differences from this PR into yours. The goal is to combine the best of both approaches into the final PubGrub integration. |
Molinillo (a backtracking resolver) is replaced by PubGrub (a CDCL SAT-based solver) which provides better conflict explanations, smarter backjumping, and provable completeness. PubGrub is already used by Bundler; this vendors the same library re-namespaced under Gem::PubGrub. Key changes: - Vendor PubGrub from bundler/lib/bundler/vendor/pub_grub/, re-namespaced Bundler::PubGrub -> Gem::PubGrub - Rewrite Gem::Resolver to implement PubGrub's source interface (all_versions_for, versions_for, incompatibilities_for, etc.) - Add Gem::Resolver::PubGrubFailure for error reporting via PubGrub's superior conflict explanation format - Add Source::Local#find_all_gems to expose all local gem versions (PubGrub needs complete version information upfront) - Prefer installed specs in version ordering to avoid unnecessary upgrades - Delete Molinillo (21 files, ~2400 lines) and resolver/stats.rb Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Preserve the original dependency requirement from root deps when building ActivationRequests, so lockfiles correctly record constraints like "a (>= 1)" instead of bare "a". Update the orphaned dependencies test: PubGrub correctly backtracks from b-2 (missing c-2) to b-1 (has c-1), finding a valid solution that Molinillo's simpler backtracking missed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create pub_grub-rubygems.patch for custom Logger and Strategy changes - Remove molinillo from vendor_gems.rb and its lockfile - Remove molinillo-master.patch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
666a38a to
38b0aa6
Compare
…sions_for PubGrub requires both methods to agree on the version universe.
|
Lots of progress over the weekend. Performance is noticeably much much improved, which is encouraging. That said, there's still a fair amount of resolver behaviour to verify and work through before this is ready. Will block out some more time this week before RubyKaigi to get this ready for review. |
There was a problem hiding this comment.
Pull request overview
This PR migrates RubyGems’ dependency resolution from Molinillo to PubGrub to align with Bundler’s solver and support the ongoing unification work discussed in #9365.
Changes:
- Vendors PubGrub under
Gem::PubGrub, wiresGem::Resolverto PubGrub’s source interface, and removes vendored Molinillo. - Updates local gem sourcing to support returning all matching local gems (
find_all_gems) so PubGrub can see the full version universe. - Adjusts RubyGems error handling/tests to reflect PubGrub’s failure explanations (
SolveFailure→DependencyResolutionError).
Reviewed changes
Copilot reviewed 27 out of 67 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tool/bundler/vendor_gems.rb.lock | Removes Molinillo from the vendoring lockfile. |
| tool/bundler/vendor_gems.rb | Removes Molinillo from Bundler’s vendoring Gemfile. |
| tool/automatiek/vendor.rb | Switches automatiek vendoring config from Molinillo to PubGrub. |
| tool/automatiek/pub_grub-rubygems.patch | Applies RubyGems-specific PubGrub tweaks (logger + strategy nil-index handling). |
| tool/automatiek/molinillo-master.patch | Removes now-obsolete Molinillo patch. |
| test/rubygems/test_gem_source_local.rb | Adds tests for Source::Local#find_all_gems and prerelease behavior. |
| test/rubygems/test_gem_resolver_strategy.rb | Adds focused tests for strategy selection + caching behavior. |
| test/rubygems/test_gem_resolver_conflict.rb | Removes Molinillo-era conflict explanation tests. |
| test/rubygems/test_gem_resolver.rb | Updates resolver tests for new error shapes/messages; adds PubGrub-related scenarios. |
| test/rubygems/test_gem_impossible_dependencies_error.rb | Removes tests for removed ImpossibleDependenciesError. |
| test/rubygems/test_gem_dependency_resolution_error.rb | Updates DependencyResolutionError expectations for PubGrub explanation-based errors. |
| test/rubygems/test_gem_dependency_installer.rb | Updates installer error expectations and adds --force/unsatisfiable dep test. |
| test/rubygems/test_gem_commands_install_command.rb | Loosens error matching to account for new error text. |
| test/rubygems/test_gem_commands_exec_command.rb | Adds exec-command test for dependency resolution failures. |
| test/rubygems/test_gem.rb | Updates activation test to reflect PubGrub backtracking behavior. |
| lib/rubygems/vendored_pub_grub.rb | Adds a new vendored entrypoint for PubGrub. |
| lib/rubygems/vendored_molinillo.rb | Removes Molinillo vendored entrypoint. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub.rb | Adds vendored PubGrub entrypoint (namespaced as Gem::PubGrub). |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/assignment.rb | Vendored PubGrub solver support code. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/basic_package_source.rb | Vendored PubGrub package source base implementation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/failure_writer.rb | Vendored PubGrub failure explanation writer. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/incompatibility.rb | Vendored PubGrub incompatibility representation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/package.rb | Vendored PubGrub package abstraction. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/partial_solution.rb | Vendored PubGrub partial solution state. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/rubygems.rb | RubyGems requirement → PubGrub range/constraint conversion helpers. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/solve_failure.rb | Vendored PubGrub SolveFailure error type. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/static_package_source.rb | Vendored PubGrub static source (mostly for tests/examples). |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/strategy.rb | Vendored PubGrub default strategy implementation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/term.rb | Vendored PubGrub term implementation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/version.rb | Vendored PubGrub version constant. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/version_constraint.rb | Vendored PubGrub version constraint implementation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/version_range.rb | Vendored PubGrub version range implementation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/version_solver.rb | Vendored PubGrub version solver implementation. |
| lib/rubygems/vendor/pub_grub/lib/pub_grub/version_union.rb | Vendored PubGrub union-of-ranges implementation. |
| lib/rubygems/vendor/pub_grub/LICENSE.txt | Adds PubGrub license. |
| lib/rubygems/source/local.rb | Adds find_all_gems; rewires find_gem to use it. |
| lib/rubygems/resolver/strategy.rb | Adds a RubyGems-specific PubGrub strategy with caching. |
| lib/rubygems/resolver/stats.rb | Removes Molinillo-era resolver stats. |
| lib/rubygems/resolver/installer_set.rb | Switches local lookup to find_all_gems so all versions are visible. |
| lib/rubygems/resolver/incompatibility.rb | Adds RubyGems wrapper for PubGrub incompatibilities with extended explanation. |
| lib/rubygems/resolver/conflict.rb | Removes Molinillo-era conflict type. |
| lib/rubygems/resolver.rb | Replaces Molinillo resolver with PubGrub integration and error translation. |
| lib/rubygems/exceptions.rb | Reworks DependencyResolutionError; removes ImpossibleDependenciesError. |
| lib/rubygems/commands/install_command.rb | Adds handling for DependencyResolutionError exit code behavior. |
| lib/rubygems/commands/exec_command.rb | Adds handling for DependencyResolutionError exit code behavior. |
| Manifest.txt | Updates shipped file list: removes Molinillo, adds PubGrub + new strategy/incompatibility files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I ran the top 1000 gems through both Molinillo & PubGrub. 996 of 1000 resolved identically. The 3 edge casesBoth resolvers produce resolutions that satisfy the declared constraints; they just pick different valid solutions in their graphs.
The one that errors
Performance
PubGrub is at parity at the median and ~30% slower at the tail and in aggregate. |
What was the end-user or developer problem that led to this PR?
My attempt at fixing #9365.
What is your fix for the problem, implemented in this PR?
Gem::PubGrubGem::Resolverto implement PubGrub's source interfaceWe should probably to de-duplicate the copies at some point but I figured that can wait.
Gem::Resolvernow implements PubGrub's source interface (all_versions_for,versions_for,incompatibilities_for,no_versions_incompatibility_for) instead of Molinillo's interfaces.Some notes:
all_versions_forreturns highest-first, with already-installed versions preferred to avoid unnecessary upgrades. Theskip_gemsmechanism continues to work for conservative updates.= 2.a).spec_forprefersInstalledSpecificationwhen available, then selects byGem::Platform.platform_specificity_matchUnsatisfiableDependencyErrorfor root deps with zero matches. Everything else flows through PubGrub'sSolveFailure->DependencyResolutionErrorwith its full explanation chain.InstallerSet: PubGrub needs all available versions upfront, butSource::Local#find_gemonly returns the highest matching version. AddedSource::Local#find_all_gems(returns all matches) and updatedInstallerSet#find_allto use it.Make sure the following tasks are checked