-
Notifications
You must be signed in to change notification settings - Fork 35
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
[Fix] no spaces in files bug, remove deprecated code, remove host #38
Conversation
…ocop, remove host
…y/rules_ruby into graham/remove-host-fix-excludes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work Graham! Very impressive.
|
||
``` | ||
bundle lock --update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -122,28 +117,10 @@ def main(args) | |||
end | |||
end | |||
end | |||
exec(ruby_program, '--disable-gems', *rubyopt, main, *args) | |||
exec(ruby_program, *rubyopt, main, *args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also didn't understand why we need to disable ruby gems.
Speaking of ruby gems — when you install a brand new Ruby interpreter, remember to run gem update --system
— this only needs to run once because rubygems rarely get upgraded, but with Ruby 2.6 this will bring Bundler version 2 as part of RubyGems.
|
||
exclude_array = excludes[spec.name] || [] | ||
# We want to exclude files and folder with spaces in them | ||
exclude_array += ["**/* *.*", "**/* */*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we work with folder with spaces in them? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bazel does not like them because it creates a runfile target name out of the file path which breaks it.
This PR:
bundle_install
to be ruby based, making it easier to specify specific parts to includeis_host
from the ruby toolchain, as now the assumption is that ruby is built and running on the system, either using the local binary if the versions match, or installing a new version. This deprecates and remove some of @yugui's work in Run ruby_binary with the interpreter in a SDK again #22. We may later refer to this and reintroduce in this in a different manner.'--disable-gems',
from ruby_binary as many ruby binaries depend on ruby gems. I am unsure of why this was excluded. For an example whereGem
is required in a ruby_binary look at https://github.com/rubocop-hq/rubocop/blob/master/lib/rubocop/yaml_duplication_checker.rb@bundler//:libs
and:@bundler//:all
I think, like with go, we should encourage being specific about which gems are being depended on. This will help later when we optimize the installation of gems to only install gems that are needed for a specific build/target.