-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Load rubygems plugins from RUBYLIB during bundle install
and bundle update
#3534
Conversation
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
I'm so sorry for the delay getting to this. Wow, this has been broken for such a long time! Can you add a test to ensure we don't regress again in the future? |
a52d241
to
49c0061
Compare
I took a stab today, but I’m not familiar with the tests so I was
unsuccessful. I’ll work more on it soon, but I’m prepping for a move so
might be a few days.
…On Mon, Jul 13, 2020 at 12:37 PM David Rodríguez ***@***.***> wrote:
I'm so sorry for the delay getting to this. Wow, this has been broken for
such a long time! Can you add a test to ensure we don't regress again in
the future?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3534 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA5AF6R5QFXXSZMTV33P3DR3NO7HANCNFSM4MM2CFTA>
.
|
49c0061
to
7201382
Compare
Thanks, no rush! Another option would be to write a high level test in a workflow. Like install |
6e2185a
to
d5c54cf
Compare
@deivid-rodriguez Thanks for the pointer to workflows, but I think I managed to get a good lightweight test using the install command suite. It creates a simple plugin that emits a message to stdout and verifies that the bundle install command runs the plugin and the message appears in the output. I verified that the test does not pass before this PR and passes after. The only test that did not pass was |
77dd413
to
1632708
Compare
I figured it out, I needed to be more careful setting the |
Hi @deivid-rodriguez, hoping you've had some time to think about this and get this landed. It would be nice to get automatic reshimming working again for tools like rbenv and asdf. Hopefully any larger refactor of plugins would not need to block this relatively targeted fix. |
Hei @djmarcin! Thanks a lot for your patience here. I'm putting this PR on my review queue and plan to get to this in the next couple of weeks. The fix itself looks fine from what I remember, so it will land, don't worry :) Other than reviewing the fix itself, I also wanted to try out |
Thanks @deivid-rodriguez, appreciate your time.
Both version managers use similar "shim" strategies for making installed binaries available on your PATH. They both attempt to use the plugin mechanism to "reshim" (asdf) or "rehash" (rbenv) to pick up new binaries installed by new gems. When you use rbenv works around this by always running a rehash during asdf does not have a corresponding workaround, meaning that the issue persists until users run a reshim operation manually. With this PR, the need for a manual rehash/reshim goes away in both version managers as new gems always trigger the plugin mechanism, whether they come via |
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.
@djmarcin I'm really sorry for the delay. I had a look at this and looks great. Thanks for the great explanation of how this improves things for version managers too!
Could you rebase on top of the latest default branch so that CI runs one more time and I'll merge after it's green 👍. |
bundle install
and bundle update
1632708
to
2bb5a66
Compare
No worries, I rebased it for you, and also added a super tiny tweak to the spec just to make the style a bit more consistent with other specs manipulating RUBYLIB. |
Thanks @djmarcin!! 🎉 |
Load rubygems plugins from RUBYLIB during `bundle install` and `bundle update` (cherry picked from commit 5bb5583)
Load rubygems plugins from RUBYLIB during `bundle install` and `bundle update` (cherry picked from commit 5bb5583)
Load rubygems plugins from RUBYLIB during `bundle install` and `bundle update` (cherry picked from commit 5bb5583)
Load rubygems plugins from RUBYLIB during `bundle install` and `bundle update` (cherry picked from commit 5bb5583)
Description:
Restore load_env_plugins so that plugins stored in RUBYLIB are loaded by bundler.
What was the end-user or developer problem that led to this PR?
rubygems/bundler#4954 broke plugins that are loaded by setting RUBYLIB. The comments in the PR make clear that the intention was to augment the set of plugins being loaded, not remove ENV plugins and replace with only plugins in the standard location. This hook does work when loaded by
gem install
because ofrubygems/lib/rubygems/gem_runner.rb
Line 15 in 0ab446f
This explains why when rbenv attempts to hook into bundler like in the following link, it doesn't work -- this code is never actually executed during
bundle install
. https://github.com/rbenv/rbenv/blob/0843745be9267de0296de8725c5b7bfcbbb6bddf/rbenv.d/exec/gem-rehash/rubygems_plugin.rb#L14-L35What is your fix for the problem, implemented in this PR?
This PR restores the load from environment variables that was erroneously removed in rubygems/bundler#4954
Tasks:
I will abide by the code of conduct.