Fix plugin installation from gemfile #6957

ccutrer · 2023-09-13T22:33:14Z

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:

Avoids re-installing a plugin every time bundle install is called
Avoids even reaching out to get gem metadata if the plugin is already satisfied (not solved by [Plugin] Fix "Installing" bug when install plugin from Gemfile #6643, and would be very difficult to solve with the after-resolution approach to skipping unnecessary installs)
If the plugin referenced in the index doesn't satisfy the version constraint, it will be reinstalled ([Plugin] Fix "Installing" bug when install plugin from Gemfile #6643 has a bug here when used with path based plugins, though it would be easy to solve in the same manner that I extract the version in this PR)

Fixes #6630.
Fixes #6589.
Closes #3319.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

ccutrer · 2023-09-13T23:33:45Z

I've confirmed this also fixes #6589

pboling

This is awesome 🎸

bundler/lib/bundler/plugin.rb

slaughter550 · 2023-12-08T08:33:34Z

@pboling anything else you wanted to see on this PR?

pboling · 2023-12-08T14:10:45Z

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.

deivid-rodriguez · 2024-03-29T15:39:55Z

This is indeed pretty cool! @ccutrer can you rebase this?

ccutrer · 2024-03-29T15:41:31Z

There are several conflicts. I'll try to get them resolved later today.

ccutrer · 2024-03-29T19:53:36Z

Rebased and conflicts resolved; I didn't run rubocop or specs locally -- I'll let GitHub Actions handle that

deivid-rodriguez · 2024-04-29T18:18:41Z

@ccutrer I assume test failures here mean this still needs some work? Happy to have a look if needed!

bundler/spec/plugins/install_spec.rb

ccutrer · 2024-05-03T15:11:50Z

Yes, this still needs attention. I'm sorry I haven't had time for it - have had other priorities at work.

deivid-rodriguez · 2024-05-03T15:18:04Z

No problem at all!

ccutrer · 2024-05-24T22:33:39Z

@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.

deivid-rodriguez · 2024-05-28T09:57:47Z

Great, thanks! I'll pick this up as soon as I can 👍.

ccutrer · 2024-06-10T19:15:59Z

@deivid-rodriguez : any idea when you'll have time to look at this?

deivid-rodriguez · 2024-06-11T11:06:19Z

This week!

deivid-rodriguez

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.

bundler/lib/bundler/dsl.rb

bundler/lib/bundler/definition.rb

bundler/lib/bundler/plugin.rb

ccutrer · 2024-06-12T16:29:19Z

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 .bundle/plugins/index - either within the app, or in the user's home directory) with a lockfile. But the index serves a greater purpose than simply keeping track of the gems installed as plugins. It is also a more lookup type structure of "for a given hook type, which plugin(s) are registered to receive callbacks?". So perhaps we could switch the index to a lockfile, just with a custom section? Really that would just simplify the piece in Bundler::Plugin where we construct an in-memory lockfile structure. Unless we start treating plugins are regular dependencies in the Gemfile as well (perhaps put them in an implicit plugins group, and always setting them to require: false?), the bifurcation of dependencies and plugins in Dsl and Definition has to continue, otherwise the processes that clean the cache will wipe out any cached gems that are actually plugins, because they'll be unaware of them. I honestly like the idea of adding plugins to the regular lockfile, because it means as a developer that manages a plugin that everyone on my team must use, I can rely on the lockfile itself for defining exactly which version is in use, instead of only using an exactly pinned version in the plugin line in the Gemfile. But again, it doesn't really get to eliminate much if any code in Plugin and Plugin::Index, because those need to continue to operate without the lockfile.

So... let me know if you want me to pursue a different direction, and which.

deivid-rodriguez

This is great, I made some comments on the implementation. I also tried it with a real plugin and it works fine!

bundler/lib/bundler/definition.rb

deivid-rodriguez · 2024-06-25T16:44:36Z

bundler/lib/bundler/definition.rb

    def validate_runtime!
      validate_ruby!
      validate_platforms!
+      validate_plugins!


This seems like a bundle check enhancement, but it seems to be that it'd be useful for regular gems too? Should we move this enhacement to a separate PR and work on generalizing it there?

I'm not sure what you're hoping for here. The validate_plugins! piece is checking that all plugins referenced in the Gemfile actually exist. It needs to run for any bundle command (even just bundle/setup, from the app) because if the plugins index references a gem (by path!) that has been removed for any reason (referencing a system gem that has been uninstalled; referencing a path gem that the directory was removed, etc.), you get an indecipherable loading error when Bundler tries to load the plugin. This includes even if you're running bundle check.

I'm just curious, since we don't do the same validation for regular specs here, about how Bundler behaves in this situation for regular gems. We have not heard about such errors regarding gems. I think we automatically reinstall them if missing in some situations, and give a useful error when materializing otherwise (no upfront validation).

I mentioned bundle check because it was the only thing that failed in specs when commenting this out.

I could probably add more specs around this. bundle install, or even simply require "bundler/setup" in the app will fail with a near-indecipherable exception about not being able to load plugins.rb without this.

I added an additional check in the spec for this that bundle exec rake -T also fails gracefully, and ensured that without this line, it indeed fails with a cryptic error about a path not existing:

(`/Users/cody/src/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.6.0.dev/lib/bundler/source/path.rb:197:in `load_spec_files': The path `/Users/cody/src/rubygems/bundler/tmp/1/libs/ga-plugin-1.0` does not exist. (Bundler::PathError)

Also, I wonder if #7639 will improve the situation?

#7639 is definitely related! That was the original situation I encountered that I added this for. I'm a little surprised this PR didn't resolve her issue.

is it something specific to your bundler-multilock plugin

Yes and no. Using return in the Gemfile is my solution to this class of problem. But this class of problem would apply to any plugin that extends the Gemfile DSL. bootboot avoids the problem by not actually modifying syntax of the DSL, but just tweaking ENV vars which are still valid Ruby syntax regardless of if the plugin has loaded yet. bundler-override addresses it by having the user add a line to the Gemfile that immediately loads the plugin, even in the plugin-only phase of parsing - and adding a rescue nil (😱) in that line to ignore the error that occurs when the plugin isn't installed yet (though this seems to me that the initial install would always be broken, because then it will fail later on the override call).

Is this something we can "fix" inside Bundler instead, so it's not necessary?

I've thought about this a decent amount. My possible ideas:

First Idea:

Have a plugin hook to say "my plugin extends the DSL",

During the plugin-only evaluation of the Gemfile, if a plugin isn't installed yet, modify plugin method in two ways:

If the plugin isn't installed yet, exit the entire evaluation immediately and install just that plugin, then retry

If the plugin is installed and is registered to extend the DSL, load it immediately at this point

If you're updating a plugin though, we won't know you're out of date during this phase, and you could be using a new feature from the updated plugin that hasn't been installed yet, and it could still fail.

Essentially capture and ignore any exception during Gemfile evaluation for the plugin phase. You'd think it would just be NoMethodError (and possibly LoadError and ArgumentError), but I imagine it would be possible for just about anything to happen inside a plugin's DSL addition. This sounds kind of icky at first, but any "real" errors should crop up again and be reported when we get to the primary evaluation of the Gemfile.

If one of these were implemented, it might be possible to eliminate the validate_plugins! from this PR. It's hard to know for sure until it's implemented. I'd definitely want to work on it as a separate PR.

since the current error at least mentions that the path does not exist while the new error mentions that the plugin "is not installed", which does not make a lot of sense

🤔 and here I am thinking "this is more clear, because regardless of if the gem exists, it may not be "installed" into the plugin index in current working directory as a Bundler plugin". But that comes from the perspective of someone familiar with how plugins work in Bundler. So yeah, I guess it could be more detailed and/or conditional: "The plugin is not installed" if the directory doesn't exist, or "The gem for plugin X is installed, but is not registered with Bundler; please run bundle install"?

Alright, that sounds good, let's give a more detailed message in that case and this is good to go from my side. We can work on improving the same error for standard gems later.

Very interested in this conversation. I have this exact problem with bundler-inject, because it also extends the DSL. I have to have the callers do a weird patching method, so a new hook for adding DSL methods would be amazing. Here's the details I wrote up if you're interested: https://github.com/ManageIQ/bundler-inject?tab=readme-ov-file#what-is-this-sorcery

(EDIT: I see you referenced bundler-override's line, which is actually the same line we use in bundler-inject...didn't realize they were using it too 😄)

@ccutrer Are you planning on providing the better error message you suggested? If you don't have time, I'm happy to give it a try myself, all these improvements have been delayed too long already :)

bundler/spec/other/major_deprecation_spec.rb

bundler/lib/bundler/plugin.rb

bundler/lib/bundler/plugin/installer.rb

deivid-rodriguez · 2024-06-26T15:03:39Z

Hei @ccutrer. I just merged a big PR that changes a lot of things in the test environment. It did not create a lot of conflicts here, but make sure to regenerate tmp/ once you rebase (rm -rf tmp && bin/rake spec:parallel_deps), because otherwise you'll get weird test failures. Sorry about that!

ccutrer · 2024-07-09T21:43:37Z

Rebased and all comments addressed. I ran most specs locally, so hopefully I got all the changes with the spec infrastructure changes.

deivid-rodriguez · 2024-07-10T10:34:26Z

Hi @ccutrer, thanks for your work. It seems some Bundler 3 specs are still not working. I'll be offline for a few days but I'll come to this next week.

ccutrer · 2024-07-10T21:23:39Z

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.

ccutrer · 2024-07-31T20:35:45Z

Ping @deivid-rodriguez

ccutrer · 2024-07-31T21:00:32Z

Rebased to resolve trivial merge conflict

deivid-rodriguez · 2024-08-01T14:53:06Z

Sorry @ccutrer, I'll get to this as soon as possible!

deivid-rodriguez

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.

deivid-rodriguez · 2024-08-02T16:32:01Z

bundler/lib/bundler/definition.rb

    def validate_runtime!
      validate_ruby!
      validate_platforms!
+      validate_plugins!


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?

bundler/lib/bundler/plugin.rb

ccutrer · 2024-09-23T14:29:00Z

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

ccutrer · 2025-04-09T16:34:56Z

@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.

ccutrer · 2025-05-28T16:57:22Z

@deivid-rodriguez are you waiting for anything else from me? Or just been busy with other priorities?

deivid-rodriguez · 2025-05-28T17:13:45Z

No, just busy, sorry about it, I'll try find some time for this PR.

ccutrer · 2025-06-30T19:32:35Z

Ping @deivid-rodriguez

deivid-rodriguez · 2025-07-01T04:31:22Z

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.

ccutrer mentioned this pull request Sep 14, 2023

Bundler plugins #3319

Open

pboling reviewed Sep 14, 2023

View reviewed changes

bundler/lib/bundler/plugin.rb Outdated Show resolved Hide resolved

ccutrer force-pushed the plugin-install branch 4 times, most recently from 024ff9c to 7122f47 Compare October 11, 2023 16:43

ccutrer force-pushed the plugin-install branch 2 times, most recently from 31d3050 to c014b7f Compare March 29, 2024 19:53

ccutrer force-pushed the plugin-install branch from c014b7f to 9041bb6 Compare March 29, 2024 20:06

deivid-rodriguez reviewed May 3, 2024

View reviewed changes

bundler/spec/plugins/install_spec.rb Show resolved Hide resolved

deivid-rodriguez mentioned this pull request May 3, 2024

[Plugin] Fix "Installing" bug when install plugin from Gemfile #6643

Closed

4 tasks

ccutrer force-pushed the plugin-install branch 4 times, most recently from e8f0d0a to 5acea42 Compare May 24, 2024 21:18

deivid-rodriguez reviewed Jun 12, 2024

View reviewed changes

bundler/lib/bundler/dsl.rb Outdated Show resolved Hide resolved

bundler/lib/bundler/definition.rb Outdated Show resolved Hide resolved

bundler/lib/bundler/plugin.rb Outdated Show resolved Hide resolved

deivid-rodriguez added the bundler: bug fix label Jun 21, 2024

deivid-rodriguez reviewed Jun 25, 2024

View reviewed changes

ccutrer force-pushed the plugin-install branch from c9aa3a2 to 5fe8eea Compare July 9, 2024 21:43

ccutrer force-pushed the plugin-install branch from 5fe8eea to b4da948 Compare July 10, 2024 20:50

ccutrer force-pushed the plugin-install branch from b4da948 to d5a55e1 Compare July 31, 2024 21:00

ccutrer force-pushed the plugin-install branch 3 times, most recently from 12da3f2 to 702f10e Compare July 31, 2024 22:09

deivid-rodriguez reviewed Aug 2, 2024

View reviewed changes

deivid-rodriguez mentioned this pull request Aug 2, 2024

Add useful error message for plugin load #7639

Merged

4 tasks

deivid-rodriguez mentioned this pull request Aug 23, 2024

plugin directive doesn't honor version constraint #7415

Open

deivid-rodriguez mentioned this pull request Feb 10, 2025

New plugin hooks around fetching gems and git sources #8162

Open

4 tasks

ccutrer and others added 4 commits April 9, 2025 08:59

Change how plugin deps are identified

2477e15

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.

remove validate_plugins!

2542e71

it's not core to this PR, and can be debated separately

ccutrer force-pushed the plugin-install branch from 702f10e to 2542e71 Compare April 9, 2025 16:32

Uh oh!

Fix plugin installation from gemfile #6957

Are you sure you want to change the base?

Fix plugin installation from gemfile #6957

Conversation

ccutrer commented Sep 13, 2023 • edited by deivid-rodriguez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

Uh oh!

ccutrer commented Sep 13, 2023

Uh oh!

pboling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slaughter550 commented Dec 8, 2023

Uh oh!

pboling commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Mar 29, 2024

Uh oh!

ccutrer commented Mar 29, 2024

Uh oh!

ccutrer commented Mar 29, 2024

Uh oh!

deivid-rodriguez commented Apr 29, 2024

Uh oh!

Uh oh!

ccutrer commented May 3, 2024

Uh oh!

deivid-rodriguez commented May 3, 2024

Uh oh!

ccutrer commented May 24, 2024

Uh oh!

deivid-rodriguez commented May 28, 2024

Uh oh!

ccutrer commented Jun 10, 2024

Uh oh!

deivid-rodriguez commented Jun 11, 2024

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ccutrer commented Jun 12, 2024

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deivid-rodriguez Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ccutrer commented Sep 13, 2023 •

edited by deivid-rodriguez

Loading

pboling commented Dec 8, 2023 •

edited

Loading

deivid-rodriguez Jun 26, 2024 •

edited

Loading

Fryguy Aug 28, 2024 •

edited

Loading