Fix plugin installation from gemfile#6957
Conversation
|
I've confirmed this also fixes #6589 |
ccutrer
force-pushed
the
plugin-install
branch
4 times, most recently
from
024ff9c to
7122f47
Compare
|
@pboling anything else you wanted to see on this PR? |
|
Moving the ball forward on plugins is amazing. There is probably still some gap before I can do what I was hoping with plugins, but this is a great step toward making them more of a first-class citizen within bundler. |
|
This is indeed pretty cool! @ccutrer can you rebase this? |
|
There are several conflicts. I'll try to get them resolved later today. |
ccutrer
force-pushed
the
plugin-install
branch
2 times, most recently
from
31d3050 to
c014b7f
Compare
|
Rebased and conflicts resolved; I didn't run rubocop or specs locally -- I'll let GitHub Actions handle that |
|
@ccutrer I assume test failures here mean this still needs some work? Happy to have a look if needed! |
|
Yes, this still needs attention. I'm sorry I haven't had time for it - have had other priorities at work. |
|
No problem at all! |
ccutrer
force-pushed
the
plugin-install
branch
4 times, most recently
from
e8f0d0a to
5acea42
Compare
|
@deivid-rodriguez : this is ready for review now. I had to do significant reworking for Bundler 3, and I left the fixes as separate commits. I'd be happy to squash down as you want. |
|
Great, thanks! I'll pick this up as soon as I can 👍. |
|
@deivid-rodriguez : any idea when you'll have time to look at this? |
|
This week! |
deivid-rodriguez
left a comment
deivid-rodriguez
left a comment
There was a problem hiding this comment.
I started reviewing and had the idea of locking plugins in the lockfile normally. Essentially, treating them just like the rest of the gems. I think that could mean not having to extend Bundler::Definition at all? What are your thoughts on this idea?
In general I think the current approach is way better than what we have, but it feels we're basically duplicating the current logic to deal with dependencies for plugins, and I wondered if it's really necessary.
One concern that I have is backwards compatibility. If we start including a bunch of extra gems in the lockfile when gemfile includes plugins, will older versions of Bundler understand that? I think it would not be a problem a older Bundler would just remove the gems they don't consider part of the bundle, but we'd need to try.
|
I had considered that, and in many ways it seems far more direct and simpler. For some reason I was thinking new sections of the lockfile can only be introduced in major version updates the Bundler, but maybe I'm wrong? The big thing that turned me away from it though is that plugins can be installed outside the context of a Gemfile (and its lockfile). Or they could be installed when there is a Gemfile and lockfile, but unrelated to it - like if a developer wants to have a bundler plugin in their working directory, but not have others use it. My other idea is to replace the plugin index (in So... let me know if you want me to pursue a different direction, and which. |
|
Minor fix required for Bundler 3. 🤞 we're ready to go now! I'll be out next week though, so won't be able to address anything else right away. |
|
Ping @deivid-rodriguez |
|
Rebased to resolve trivial merge conflict |
ccutrer
force-pushed
the
plugin-install
branch
3 times, most recently
from
12da3f2 to
702f10e
Compare
|
Sorry @ccutrer, I'll get to this as soon as possible! |
deivid-rodriguez
left a comment
deivid-rodriguez
left a comment
There was a problem hiding this comment.
I gave this one more look. It looks almost ready to me, although I mostly have the same concerns as before.
Happy to ignore the one about :type sources and how they force us to do two install phases, but would like to clarify plugin validation since it still feels a bit off to me.
| def validate_runtime! | ||
| validate_ruby! | ||
| validate_platforms! | ||
| validate_plugins! |
There was a problem hiding this comment.
I still don't understand what's special about plugins here. If I change the spec that you added to use gem instead of plugin then I get the same error. That's why I think if we want to improve this situation, we should do it in general, not just for "plugin gems".
Also, I'm not convinced the new error is better. path gems don't actually get installed, they're just a folder in your system which gets added to the $LOAD_PATH, nothing more. So changing the current error that highlights that the path pointed to by a path sourced gem no longer exists, to instead read that the gem is not installed seems not an improvement to me?
|
Yup, I plan to get to it this week. I finally have some slack from other higher priority projects at work. Thanks for keeping it in mind! |
Several things are fixed: * Don't re-install a plugin referenced from the gemfile with every call to `bundle install` * If the version of a plugin referenced in the gemfile conflicts with what's in the plugin index, _do_ re-install it * If plugins aren't installed yet, don't throw cryptic errors from commands that don't implicitly install gems, such as `bundle check` and `bundle info`. This also applies if the plugin index references a system gem, and that gem is removed. This is all accomplished by actuallying including plugins as regular dependencies in the Gemfile, so that they end up in the lockfile, and then just using the regular lockfile with the plugins-only pass of the gemfile in the Plugin infrastructure. This also means that non-specific version constraints can be used for plugins, and you can update them with `bundle update <plugin>` just like any other gem. Co-authored-by: Diogo Fernandes <diogofernandesop@gmail.com>
Since ruby#8480, we can't use the undocumented "type" option, but the add_dependency helper was added that lets us easily validate first, then add our custom options, then directly add the now custom dependency. This conveniently simplifies the Plugin version of the DSL, because `plugin` no longer flows through `gem`, so it doesn't need to special case it.
Since ruby#8486, hax to make any dependency type work inside bundler have been removed, so we mark plugins as type: :plugin. Instead, follow that PR's lead and just make it a dedicated option to Bundler::Dependency.
it's not core to this PR, and can be debated separately
|
@deivid-rodriguez: I've rebased, resolved two trivial conflicts, then I had to fix two things that failed because of logic changes in the meantime. Then I removed validate_plugins! - it's not core to this PR, and can be debated separately. I've left the changes as separate commits so you can see what actually changed since the last time you reviewed. After you've done that, I can squash back down to a single commit. |
|
@deivid-rodriguez are you waiting for anything else from me? Or just been busy with other priorities? |
|
No, just busy, sorry about it, I'll try find some time for this PR. |
|
Ping @deivid-rodriguez |
|
Sorry again for the delay, I have this PR in mind but I'm not finding time for it. I'll get back to it once I make the releases that I'm working on now. |
) Every `bundle install` reprints four "Installing X" lines for the :vault plugin's transitive dependencies (command_kit, sqlite3, gemvault, bundler-source-vault) even when nothing has changed. Root cause: Bundler::Plugin::Installer#install_definition unconditionally calls spec.source.install(spec) for every resolved spec — there is no skip-if-already-installed check. This is a Bundler-side issue, not a gemvault issue. It affects every bundler plugin that declares runtime dependencies. There's no clean fix from the plugin side: our plugins.rb loads during Bundler.definition — after Plugin.gemfile_install has already reinstalled everything — so a monkey-patch from there arrives too late. An upstream Bundler PR (ruby/rubygems#6957) addresses this and will resolve the noise once it ships in a Bundler release. - README section links to the upstream bug and fix PR, and documents `bundle config set --local plugins false` as a user-opt-in workaround with its trade-offs (disables plugin auto-install for the project; caller must not commit .bundle/config with the setting, since it would affect teammates who rely on plugin auto-install elsewhere). - Integration spec `stops reinstalling plugin deps once BUNDLE_PLUGINS=false is set` reproduces the bug end-to-end (uninstall system gemvault to force the plugin gems into Plugin.root only, then run `bundle install` twice) and verifies that setting BUNDLE_PLUGINS=false neutralises the reinstall. That same spec will keep passing after the upstream PR lands, serving as a regression check for our side of the contract.
Every `bundle install` reprints four "Installing X" lines for the :vault plugin's transitive dependencies (command_kit, sqlite3, gemvault, bundler-source-vault) even when nothing has changed. Root cause: Bundler::Plugin::Installer#install_definition unconditionally calls spec.source.install(spec) for every resolved plugin spec — there is no skip-if-already-installed check. Upstream bug tracked at ruby/rubygems#6630, fix proposed in ruby/rubygems#6957. Ship a drop-in wrapper instead of auto-editing the user's config: gemvault bundle install # instead of `bundle install` gemvault bundle update --conservative rake gemvault bundle lock `gemvault bundle` execs `bundle` with BUNDLE_PLUGINS=false set only in the child process environment — no .bundle/config write, no persistent project setting, no impact on teammates or other bundler plugins the project uses. Plain `bundle install` still works exactly as before (including plugin auto-install) for the cases where you actually need plugin install to run. - lib/gemvault/cli/commands/bundle.rb — the command, auto-registered via command_kit AutoLoad. - test/cli_bundle_command_test.rb — minitest covering the exec behaviour: sets BUNDLE_PLUGINS=false, forwards arguments, works with no args. - spec/integration/bundle_install_spec.rb — new integration spec that uninstalls the system-gem copy of bundler-source-vault so the reinstall bug manifests, runs a plain `bundle install` twice (second run reprints "Installing bundler-source-vault" — the bug), then runs `gemvault bundle install` (no reinstall — the fix). - README section documents the wrapper and the underlying upstream bug.
#4) Bundler's `Plugin.gemfile_install` calls `Installer.new.install_definition` with the full dependency list on every `bundle install` and has no check for "all declared plugins are already registered". `install_from_specs` then blindly reinstalls each spec, so the :vault plugin's four transitive gems (command_kit, sqlite3, gemvault, bundler-source-vault) reprint "Installing X" on every single `bundle install`. It's bundler bug ruby/rubygems#6630 from 2023; a structural fix is proposed in ruby/rubygems#6957 but hasn't shipped in a release. Nothing a third-party plugin loads runs before `Plugin.gemfile_install`: the shim's `plugins.rb` only loads during `Bundler.definition`, which happens after the bug fires. So the fix has to live inside Bundler's own file. Ship a CLI that applies it directly: gemvault patch-bundler # one-time, per bundler version on disk gemvault unpatch-bundler # reversible The patch inserts one early return into `Bundler::Plugin.gemfile_install`: plugins = definition.dependencies.map(&:name) # gemvault-bundler-patch: skip-reinstalled-plugins return if definition.dependencies.map(&:name).all? { |n| index.installed?(n) } installed_specs = Installer.new.install_definition(definition) If every declared plugin is already in the index, gemfile_install returns before `install_definition` can do anything. Plain `bundle install` and every other bundler subcommand go back to behaving normally. The patch carries a marker comment so it's idempotent and unpatch can find it to reverse it cleanly. Discovery scans system gem paths, Ruby's stdlib dir (for the bundled-default bundler that ships with Ruby 4.x), and any `vendor/ruby/*/gems/bundler-*` under the current directory — so a user who runs `gemvault patch-bundler` from within a project catches both the system bundler and their vendored one. - lib/gemvault/bundler_patch.rb — the patcher, pure string surgery over Bundler's source file with a constant marker for idempotency. - lib/gemvault/cli/commands/{patch_bundler,unpatch_bundler}.rb — command_kit commands exposed as `gemvault patch-bundler` and `gemvault unpatch-bundler`. - test/bundler_patch_test.rb — unit tests over the patch/revert logic: rewrites pristine source, idempotent, refuses unknown bundler shapes, round-trips to the exact pristine byte stream. - spec/integration/bundle_install_spec.rb — integration spec that reproduces the bug end-to-end (uninstall the system-gem copy of bundler-source-vault so the plugin is only in Plugin.root, then run two plain `bundle install`s — the second reprints "Installing bundler-source-vault": RED), runs `gemvault patch-bundler`, then runs a third `bundle install` and asserts the reinstall line is gone: GREEN. - README section documents the command, the scope of its scan, the upstream bug/PR, and the reason for patching bundler on disk rather than monkey-patching from our plugin. Removes the earlier `gemvault bundle` wrapper approach, which was a BUNDLE_PLUGINS=false env-var workaround dressed up as a CLI.
#4) Bundler's `Plugin.gemfile_install` calls `Installer.new.install_definition` with the full dependency list on every `bundle install` and has no check for "all declared plugins are already registered". `install_from_specs` then blindly reinstalls each spec, so the :vault plugin's four transitive gems (command_kit, sqlite3, gemvault, bundler-source-vault) reprint "Installing X" on every single `bundle install`. It's bundler bug ruby/rubygems#6630 from 2023; a structural fix is proposed in ruby/rubygems#6957 but hasn't shipped in a release. Nothing a third-party plugin loads runs before `Plugin.gemfile_install`: the shim's `plugins.rb` only loads during `Bundler.definition`, which happens after the bug fires. So the fix has to live inside Bundler's own file. Ship a CLI that applies it directly: gemvault patch-bundler # one-time, per bundler version on disk gemvault unpatch-bundler # reversible The patch itself is a standard unified diff (`lib/gemvault/bundler_patch.diff`) that inserts one early return above `install_definition`: + # gemvault-bundler-patch: skip-reinstalled-plugins + return if definition.dependencies.map(&:name).all? { |n| index.installed?(n) } installed_specs = Installer.new.install_definition(definition) Applied via the canonical `patch(1)` tool — hunk matching, fuzz, and reversal come for free. Ruby handles idempotency with a marker-comment check (pure-insertion diffs can't be distinguished by `patch` from a pristine target). Two entities, constructor-injected: - `Gemvault::BundlerInstallation` — a bundler `plugin.rb` file on disk. `.discover(root:)` class method finds installations across system gems, Ruby stdlib (for bundled-default bundler), and vendored copies under the project. - `Gemvault::BundlerPatch` — the fix, parameterised by its diff path and a process runner (defaults to `Open3.method(:capture2e)` for easy stubbing). `#apply_to(installation)` and `#revert_from(installation)` return `:applied`/`:already_applied`/`:reverted`/`:not_applied`. CLI commands `PatchBundler` / `UnpatchBundler` wire the two together and print per-installation status. Tests: - test/bundler_patch_test.rb — covers `BundlerPatch#apply_to`, `#revert_from`, idempotency, and round-trip-to-pristine against a realistic `plugin.rb` fixture, plus `BundlerInstallation` value- object semantics (==, hash, Pathname wrapping). - spec/integration/bundle_install_spec.rb — end-to-end podman spec that uninstalls bundler-source-vault so the reinstall bug manifests, runs two plain `bundle install`s (second reprints `Installing bundler-source-vault`: RED), runs `gemvault patch-bundler`, then runs a third `bundle install` and asserts the reinstall line is gone: GREEN. - Dockerfile.test installs `patch` so the test image has the tool available.
5 participants
What was the end-user or developer problem that led to this PR?
This fixes #6630, #6589, and several related issues. Related to #6643, but a very different approach. Specs were copied from that PR though.
What is your fix for the problem, implemented in this PR?
Instead of blindly calling the installer with the plugins from the gemfile, include plugins as regular dependencies in the main Gemfile, and use its lockfile. This fixes several issues:
bundle installis calledFixes #6630.
Fixes #6589.
Closes #3319.
Make sure the following tasks are checked