Respect --no-install option for git: sources

[KJTsanaktsidis]

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

Currently, the --no-install option to bundle package is totally ignored for git sources. This can have very strange effects if you have:

• a git-sourced gem,
• with native extensions,
• whose extconf.rb script depends on another gem,
• which is installed from Rubygems in the gemfile.

In that circumstance, bundle package --no-install --all will download the Rubygems dependencies to vendor/cache but NOT install them. It will also check out the git gems to vendor/cache (good), and attempt to build their native extensions (bad!).

The native extension build will fail because the extconf.rb script crashes, since the dependency it needs is missing.

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

I implemented a fix for this in source/git.rb as an early return if Bundle.settings[:no_install], since this is analogous to what's happening in source/rubygems.rb.

I do admit though the whole thing is a little strange though - an "install" method that.... proceeds to look at a global flag to not install anything.

At this point, if you think this is the right solution for the problem, I can whip up a test and then get this merged. If you think there's a better way to factor this package functionality though I'm all ears.

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]

I think the underlying root cause of this is related to #3204?

[KJTsanaktsidis]

oof. That issue seems like a lot. Sorry you had to deal with that ❤️

So yeah, @thedanbob already noticed this: #3204 (comment)

I figured out why the test didn't fail: bundle cache --no-install actually does install git gems (and only git gems), and to the correct location of vendor/bundle/ruby/2.5.0/bundler/gems/. So there's no "Fetching' in the later bundle install because it's already installed!

I guess I should open another issue for bundle cache not respecting the --no-install flag for git gems

---

I think fixing the "the install path is the same as the cache path" is a necessary but not sufficient condition. If my reading of what the rubygems source is doing is correct, this is what I think the expected behaviour should be...

(apologies in advance for my wording here, I'm not too familiar with the correct Rubygems terminology)

For bundle install:

• Normally: Do a bare clone of the git repo from the remote to the system cache directory
• If --local is passed in: Assume there is a bare clone in the vendor/cache or system cache directory already
• git clone the gem from the bare clone to the install path (which will be either the system path or a path from the --path setting). Check out the correct ref.
• Build native extensions - this is done by cd'ing into the system gem path (or --path setting) and doing the ruby extconf.rb; make dance. (but see below!). Native extensions (that weren't precompiled) should never wind up in vendor/cache
• The .git directory is then deleted (not sure about this - but it seems like we could, because we know for sure the ref that was checked out from the directory name, and apparently from that issue I take it people don't want to copy large .git directories in situations where they're building gems on one machine and copying them to another)

For bundle exec:

• The installed path of the gem, either from the system directory of from the --path setting, should be added to $LOAD_PATH. The vendor/cache or system cache directory should NOT be added there.

For bundle cache:

• Nothing should happen unless the --all setting is present.
• The bare clone should be copied from the system cache dir to vendor/cache (or cloned there, if it wasn't already in the system cache dir).
• If --no-install is present, stop here
• Otherwise, proceed with the install steps - git clone the gem from vendor/cache to the --path setting or system gem dir, and build native extensions in that location as for bundle install.

Sound about right?

---

Question about native gems:

AFAICT native extensions installed by bundler wind up in two places - both in ruby/x.y.0/gems/$GEM_NAME/lib/ and ruby/x.y.0/extensions/$PLATFORM/x.y.0/$GEM_NAME/. Both seem to get added to $LOAD_PATH:

kj@kj-thinkpad p % find vendor/ -iname '*.so'
vendor/bundle/ruby/3.0.0/gems/puma-6.0.0/ext/puma_http11/puma_http11.so
vendor/bundle/ruby/3.0.0/gems/puma-6.0.0/lib/puma/puma_http11.so
vendor/bundle/ruby/3.0.0/gems/nio4r-2.5.8/ext/nio4r/nio4r_ext.so
vendor/bundle/ruby/3.0.0/gems/nio4r-2.5.8/lib/nio4r_ext.so
vendor/bundle/ruby/3.0.0/extensions/x86_64-linux/3.0.0/puma-6.0.0/puma/puma_http11.so
vendor/bundle/ruby/3.0.0/extensions/x86_64-linux/3.0.0/nio4r-2.5.8/nio4r_ext.so

irb(main):005:0> $LOAD_PATH
=>
["/home/kj/.rbenv/rbenv.d/exec/gem-rehash",
 "/home/kj/p/vendor/bundle/ruby/3.0.0/gems/puma-6.0.0/lib",
 "/home/kj/p/vendor/bundle/ruby/3.0.0/extensions/x86_64-linux/3.0.0/puma-6.0.0",
 "/home/kj/p/vendor/bundle/ruby/3.0.0/gems/nio4r-2.5.8/lib",
 "/home/kj/p/vendor/bundle/ruby/3.0.0/extensions/x86_64-linux/3.0.0/nio4r-2.5.8",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/gems/3.0.0/gems/bundler-2.2.33/lib",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/site_ruby/3.0.0",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/site_ruby/3.0.0/x86_64-linux",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/site_ruby",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/vendor_ruby/3.0.0",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/vendor_ruby/3.0.0/x86_64-linux",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/vendor_ruby",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/3.0.0",
 "/home/kj/.rbenv/versions/3.0.4/lib/ruby/3.0.0/x86_64-linux"]

Should they actually wind up in both places? Any idea what's going on here?

---

Question about --deployment:

Does the --deployment mode do anything other than imply --local --path vendor/bundle? That's my reading of the doc, but I'm not 100% sure (we don't use this at $DAYJOB because we ship docker images...)

[KJTsanaktsidis]

@deivid-rodriguez - any thoughts on the above? If I'm on the right track there I'll definitely try and contribute a patch.

[deivid-rodriguez]

@KJTsanaktsidis Sorry for the lack of feedback! Let's start with this patch, which seems correct and indeed a related but separate issue.

More issues with caching git sources should get fixed by #4469. Once we introduced the fix here and that PR we can reevaluate if there's still anything missing/incorrect.

Can you add a test for this patch? You can find related specs at bundler/spec/commands/cache_spec.rb.

[KJTsanaktsidis]

Hey @deivid-rodriguez, @Julzerator and I put our heads together in the new year and put together a test for this fix. PTAL - thanks!

[deivid-rodriguez]

Hei @KJTsanaktsidis, thanks for working on this.

That spec seems to also pass without the fix in this PR. Can you write it so that it fails without this change, and passes with it?

[deivid-rodriguez]

@KJTsanaktsidis still interested?

[KJTsanaktsidis]

Sorry @deivid-rodriguez - this totally fell off my radar. I think I wrote a much better test now, WDYT?

[deivid-rodriguez]

Looks great! Thanks for the thorough comments and test, hopefully we can remove the second comment block once #4469 is out!

[deivid-rodriguez]

Ooops, unfortunately the spec needs to be adapted to also work in "bundler 3" mode, where Bundler will install to .bundle by default, not to the default GEM_HOME. I think you can use the default_bundle_path helper for this.

[KJTsanaktsidis]

OK, think I got it working - should be good to go when you run the workflow. Thanks!

[deivid-rodriguez]

It seems good now. There were two lint issues but I fixed them myself to not delay this fix anymore!

[KJTsanaktsidis]

Great, thanks for your help, sorry it took me so long!

[deivid-rodriguez]

Thanks to you for the fix!