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

[dfop02]

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

The problem was initially described here. When you install a plugin from a Gemfile (without git or path options), the bundle keep installing it every bundle install run.

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

There is 2 problems to solve.

The first problem is block an installed plugin to install again, it was fixed the by adding a condition here, where I confirm if there is any plugin not installed yet before continue to install dependences.

The second problem is when you already have an installed plugin, but add other into your Gemfile, then it keep installing the old one. For this case, I need go a bit far to check if the plugin is installed and if the plugin installed version is the same. For this, I create a cannot_install? method on Index class for this propose. You can check the code here.

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

[welcome]

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

[deivid-rodriguez]

Thanks for fixing this, I added a couple of comments.

Also, the previous CI timeout is unrelated to this PR and now fixed on master, so a rebased CI run should not run into that.

[deivid-rodriguez]

The cannot_install? makes me think of same problem when trying to install the gem. Maybe version_already_installed? or something like that would be better?

[dfop02]

Really sounds better, I changed the def name. Also want ask about the plugin version, do you have any suggest to how get it without using the regex, maybe a build-in way that I missed... or this way is fine? I'm talking about plugin index here.

[deivid-rodriguez]

I had the same thought but I could not find a better way that the regex based approach you used.

[deivid-rodriguez]

Thank you. Still the implementation and comment feels a bit confusing to me. I don't understand the !updating?(name, version) part. How about

def version_already_installed?(name, version)
  plugin_path = @plugin_paths[name]
  return false unless plugin_path

  version.to_s != plugin_path[/#{name}-(.*)$/, 1].to_s
end

Actually, would File.basename(plugin_path) == "#{name}-#{version}" work instead of the regex based approach?

[dfop02]

My initial objective was separate the methods to looks simple, because already exists the installed? so I just added the !updating?, maybe change to not_updating? or same_version? feels more readable. But since still sounds confuse, I can try your approach. Also, good idea about the "regex issue", I'll test :)

[deivid-rodriguez]

I see, from the options you give I think same_version? would be the best name for what we're checking here. I think better names would be something like some_version_installed? instead of installed? and same_version_installed? instead of !updating?. But since the second check gives really a subset of the first one, I think it's easier to inline the logic, hence my suggestion.

[dfop02]

While I was testing your approach about the "regex issue", I found few more things to fix, and now looks like *almost everything works fine. I notice that the index file was not updating the path correctly after change versions updating/downgrading so I needed fix it too.

Almost because before, the plugin was suppose to appears "Using foo x.y.z" when already installed, but now is suppressed by default (only for plugins), I'm not sure if is a big problem. That happens because when you cli bundle install, the installed_specs returns the plugin while bundle by gemfile doesnt and I couldnt understand why that happens.

[deivid-rodriguez]

Thanks for all the work, I'll loop back and have a look at that last issue.

[deivid-rodriguez]

@dfop02 Sorry I didn't loop back to this yet. Did you look into it? We recently removed unnecessary "Using..." messages at #6804, not sure if that changes anything here.

[dfop02]

Hello @deivid-rodriguez !

I take a look again today on currently bundler version, looks like the updates solve the "using..." messages that I told before, what is nice, but installing messages and index file issue continues. For now I create a separate matcher because I thought that was causing the failures on specs, let's see if solves it :)

[ccutrer]

you haven't done anything that prevents downloading metadata from rubygems if the plugin is already installed

[ccutrer]

This won't work for a plugin "my_plugin", path: "../my_plugin" in your Gemfile.

[dfop02]

Hello @ccutrer ! Indeed, you're right, I was focusing my attention on the "avoid unnecessary installs" problem and forgot about more things that I could fix. I saw your approach, better solving the problem at the root, congrats.

[deivid-rodriguez]

Since everyone seems to agree #6957 is a better approach, let's close this in favor of that other PR.