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

Use Bundler to manage vendor directory #4895

Merged
merged 1 commit into from Sep 14, 2018

Conversation

Projects
None yet
2 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Sep 13, 2018

Alternative title: how many Gemfiles does Homebrew need? Just one more...

Rather than having to manually keep track of what version each thing in here is and copy files around by hand on update let's use Bundler's standalone mode and careful use of .gitignore to help us do it.

This means a bundle update --standalone will allow us to update all gems in vendor.

We could consider vendoring other gems this way in future but I'd suggest only doing this for gems with no dependencies or at least gems with no native extensions. The only gem this applies to that we
currently use is ruby-prof and I'm not convinced it's widely used enough to warrant vendoring for everyone. Perhaps that's another criteria: it should be functionality that's used by non-developer commands and/or normal Homebrew usage.

Once this is merged we can look at updating our current gems in a follow-up PR.

This approach should also make it much easier to selectively include other backports (#4505) or parts of e.g. ActiveSupport.

We previously discussed a brew vendor-gem command in #4288. If desired,
brew vendor-gems could run the bundle install --standalone required here in the correct directory?

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@wafflebot wafflebot bot added the in progress label Sep 13, 2018

@MikeMcQuaid MikeMcQuaid referenced this pull request Sep 13, 2018

Closed

docs: Add notes on revendoring #4288

3 of 6 tasks complete

@MikeMcQuaid MikeMcQuaid requested review from reitermarkus , woodruffw and claui Sep 13, 2018

@@ -1,4 +1,3 @@
# Taken from https://github.com/marcandre/backports/blob/v3.8.0/lib/backports/2.4.0/string/match.rb

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 13, 2018

Member

This change is not present in the upstream file so was removed by Bundler.

require_relative 'plist/generator'
require_relative 'plist/parser'
require_relative 'plist/version'
require 'plist/generator'

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 13, 2018

Member

These changes are not present in the upstream file so were removed by Bundler.

@@ -5,3 +5,5 @@
unless $LOAD_PATH.include?(HOMEBREW_LIBRARY_PATH.to_s)
$LOAD_PATH.push(HOMEBREW_LIBRARY_PATH.to_s)
end

require "vendor/bundle-standalone/bundler/setup"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 13, 2018

Member

Check below if interested; this file is generated by bundler and sets up the load path for all the relevant gems. This means they can live in versioned directories but we never need to manually update the names of those directories.

!**/vendor/bundle-standalone/ruby/*/gems/*/lib

# Ignore backports gem (we don't need all files)
**/vendor/bundle-standalone/ruby/*/gems/backports-*/lib

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 13, 2018

Member

This requires git add -f to add additional backports files but I think it's the best approach to avoid making this .gitignore very messy and backports-specific.

@@ -1,5 +1,5 @@
# Contains backports from newer versions of Ruby

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 13, 2018

Member

This comment is perhaps no longer needed as it's a bit more obvious from the require what's going on here.

Use Bundler to manage vendor directory
Rather than having to manually keep track of what version each thing in
here is and copy files around by hand on update let's use Bundler's
standalone mode and careful use of `.gitignore` to help us do it.

This means a `bundle update --standalone` will allow us to update all
gems in vendor.

We could consider vendoring other gems this way in future but I'd
suggest only doing this for gems with no dependencies or at least gems
with no native extensions. The only gem this applies to that we
currently use is `ruby-prof` and I'm not convinced it's widely used
enough to warrant vendoring for everyone. Perhaps that's another
criteria: it should be functionality that's used by non-developer
commands and/or normal Homebrew usage.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:vendor-bundle-standalone branch from 8d2d915 to d7eca0b Sep 13, 2018

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Sep 13, 2018

I think it would be nicer to have either all in vendor/bundle or have these in vendor/bundle and the other ones used for tests/installed on-demand in another directory, since we don't actually vendor these.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 13, 2018

all in vendor/bundle

can't have two vendor/bundle sharing the same destination as things like bundle cleanup and bundle --standalone will produce incorrect results.

other ones used for tests/installed on-demand in another directory, since we don't actually vendor these.

The standard location for them tends to be vendor/bundle or vendor/ruby/bundle in projects, though, and I'm not sure it's worth the confusion of moving them all. Additionally, that would require moving them for all users which would likely invalidate all cached gems for all users.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 14, 2018

Let's try shipping this as-is and iterating on it further. Things like this have gone 💥 in the past so I'm merging the simplest thing we can now so it can be reverted if need be later today.

@MikeMcQuaid MikeMcQuaid merged commit 5772493 into Homebrew:master Sep 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Sep 14, 2018

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:vendor-bundle-standalone branch Sep 14, 2018

@lock lock bot added the outdated label Oct 14, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Oct 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.