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

Patch bundler to allow different rubies #30

Merged
merged 5 commits into from
Jan 10, 2020

Conversation

jacobthemyth
Copy link
Contributor

@jacobthemyth jacobthemyth commented Dec 19, 2019

This allows using bootboot for dual booting different versions of Ruby.
For example:

source "https://rubygems.org"

plugin 'bootboot'
Plugin.send(:load_plugin, 'bootboot') if Plugin.installed?('bootboot')

if ENV['DEPENDENCIES_NEXT']
  enable_dual_booting if Plugin.installed?('bootboot')
  ruby '2.5.7'
else
  ruby '2.4.9'
end

I updated the env var from SKIP_BUNDLER_PATCH to BOOTBOOT_UPDATING_ALTERNATE_LOCKFILE because I thought it would be confusing if the RubyVersion patch took effect when that var was present, which is what needs to happen.

@jacobthemyth
Copy link
Contributor Author

I'm not sure why the build it failing, I think it must be something to do with the bundler version on Travis, but the tests all pass locally 🤔.

@jacobthemyth
Copy link
Contributor Author

@Edouard-chin I'm not sure if you monitor this repo regularly, so just want to ping you directly 😊.

@Edouard-chin
Copy link
Contributor

Hey, thanks for the PR ❤️ . I think this feature make sense. I don't have the bandwidth to do a proper review now, but I'll try to take some time next week to look.

@jacobthemyth
Copy link
Contributor Author

Sure, thanks! I'm okay if you end up deciding it's not a good fit, but I'm using it already so figured I'd upstream it if possible.

Copy link
Contributor

@Edouard-chin Edouard-chin left a comment

Choose a reason for hiding this comment

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

👍 Can you address my small comment as well as adding a section in the README for this new feature. Thanks !

@jacobthemyth
Copy link
Contributor Author

I think the additions made to README.md also address #28

@jacobthemyth
Copy link
Contributor Author

Instead of just adding an error message assertion, I actually reimplemented the patch to add Ruby to the Metadata source instead of overriding RubyVersion.system. The main reason is that when you asked for a test against the error message, I realized with the original implementation, the error message was pretty cryptic:

Could not find gem 'Ruby (~> x.x.x.x)' in the local ruby installation.
The source contains 'Ruby' at: x.x.x.x

Your Ruby version is x.x.x, but your Gemfile specified x.x.x is a better message, but required a different patch in order for the Bundler code to get that far. I tried to implement the feature using Bundler::Plugin::API::Source but as far as I could tell, Bundler has a hard coded "rule" that Ruby must come from the Metadata source, so I still had to use a patch instead of the Plugin API unfortunately.

@jacobthemyth
Copy link
Contributor Author

jacobthemyth commented Jan 9, 2020

After reviewing my previous commit, I realized that patching Metadata was too tightly coupled to the internals of Bundler. I reimplemented the Ruby dual booting again using Bundler::Plugin::API::Source instead. Even though it still requires patching Bundler::Definition, it seems far less coupled to the internals this way.

Also, overriding Bundler::RubyVersion.system turns out to be required because even though Metadata is used for resolution, system is used for writing the lock file. I've modified the tests to correctly inspect the locked Ruby version.

Copy link
Contributor

@Edouard-chin Edouard-chin left a comment

Choose a reason for hiding this comment

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

Thanks, this is great 👏

@Edouard-chin Edouard-chin merged commit fb502ad into Shopify:master Jan 10, 2020
@jacobthemyth
Copy link
Contributor Author

Thanks for taking the time to review and give feedback!

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.

2 participants