Skip to content
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

Add useful error message for plugin load #7639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sarahsehr
Copy link

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

When starting to work on my first issue, I found that rake setup failed for me. This was due to having previously installed a plugin, whose files had since been deleted (presumably when I uninstalled that version of Ruby).

#7636

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

Check to see whether each load path for the plugin is a directory. If it is not, notify the user in a friendly message which plugin in the problem and how to resolve the issue.

Make sure the following tasks are checked

If a plugin has previously been installed, but the path is no longer
valid, `rake setup` will fail with an unexpected error due to the file
not existing.

Instead, we want to present the user with what the issue is and how to
resolve the problem.
Copy link

welcome bot commented May 8, 2024

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

@simi
Copy link
Member

simi commented May 8, 2024

Hello @sarahsehr. Code looks good, but some specs are failing, which seems not really related to your change, since I can see the same failure on main branch. I'm looking into. 👀

EDIT: false alarm, main branch looks ok, it was outdated tmp during my initial test.


bundler plugin install #{name}
MESSAGE
raise PluginError.new(message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered just warn in here and skip plugin + continue? 🤔

raise PluginError.new(message)
end

Gem.add_to_load_path(paths)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarahsehr this seems to fails the test (and breaks some test gem installation at the same time). add_to_load_path is defined at

rubygems/lib/rubygems.rb

Lines 576 to 584 in c1933bf

##
# Add a list of paths to the $LOAD_PATH at the proper place.
def self.add_to_load_path(*paths)
@activated_gem_paths = activated_gem_paths + paths.size
# gem directories must come after -I and ENV['RUBYLIB']
$LOAD_PATH.insert(Gem.load_path_insert_index, *paths)
end
and it expects multiple arguments. To achieve this by passing paths array, you need to add splat (*) operator. Changing this to Gem.add_to_load_path(*paths) should fix this. 🙏

@deivid-rodriguez
Copy link
Member

Since this a bundle install run after all, should we automatically reinstall any missing plugins if necessary, instead of erroring out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants