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! |
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. |
bundler/lib/bundler/definition.rb
Outdated
| def validate_runtime! | ||
| validate_ruby! | ||
| validate_platforms! | ||
| validate_plugins! |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Also, I wonder if #7639 will improve the situation?
There was a problem hiding this comment.
#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-multilockplugin
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
pluginmethod 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 possiblyLoadErrorandArgumentError), 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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄)
There was a problem hiding this comment.
@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 :)
|
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 |
|
Rebased and all comments addressed. I ran most specs locally, so hopefully I got all the changes with the spec infrastructure changes. |
|
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. |
|
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! |
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.
bundler/lib/bundler/definition.rb
Outdated
| 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 <[email protected]>
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. |
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