-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 absolute path bug in .profile.d/ruby.sh $PATH #973
Conversation
5313891
to
99d296e
Compare
58607d7
to
7100cce
Compare
In #888 a relative path was accidentally introduced into the `.profile.d/ruby.sh` script: ``` export PATH="bin:$HOME/vendor/bundle/bin:$HOME/vendor/bundle/ruby/2.6.0/bin:$PATH" ``` That first path `bin` is relative and should be `$HOME/bin`. Without it, then changing into a different directory means that the system version of ruby will be used ``` ~ $ which ruby bin/ruby ~ $ mkdir foo; cd foo ~/foo $ which ruby /usr/bin/ruby ``` This is a regression was not caught by tests previously because the version of ruby that the test `cd_ruby` uses is 2.5.1 and that happens to match system ruby. To fix for catching future regressions the test is now more explicit. Unfortunately there's a bug in hatchet preventing that specific test heroku/hatchet#71 so instead I'm at least asserting that it's an absolute path.
7100cce
to
0e52f71
Compare
Fix CNB case. In the CNB build process the `gem_layer_path` is not `$HOME` to ensure that `$HOME/bin` is always on the path, we can manually add it. If it gets added multiple times, we'll call `uniq` on the array before setting the override.
f7df9ad
to
203e9ea
Compare
@@ -211,7 +211,7 @@ def default_path(gem_layer_path = ".") | |||
# need to remove bin/ folder since it links | |||
# to the wrong --prefix ruby binstubs | |||
# breaking require. This only applies to Ruby 1.9.2 and 1.8.7. | |||
safe_binstubs = binstubs_relative_paths(gem_layer_path) - ["bin"] | |||
safe_binstubs = binstubs_relative_paths(gem_layer_path) - ["bin", "#{gem_layer_path}/bin"] |
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.
we don't need bin
here anymore right?
@@ -429,13 +429,14 @@ def setup_export(layer = nil) | |||
# sets up the profile.d script for this buildpack | |||
def setup_profiled(ruby_layer_path = "$HOME", gem_layer_path = "$HOME") | |||
instrument 'setup_profiled' do | |||
profiled_path = binstubs_relative_paths(gem_layer_path) | |||
profiled_path = ["$HOME/bin"] |
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.
is this redundant if $HOME
is passed in?
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.
Let's get this out now and re-review.
In #888 a relative path was accidentally introduced into the
.profile.d/ruby.sh
script:That first path
bin
is relative and should be$HOME/bin
.Without an absolute path, changing into a different directory means that the system version of ruby will be used
This is a regression was not caught by tests previously because the version of ruby that the test
cd_ruby
uses is 2.5.1 and that happens to match system ruby. To fix for catching future regressions the test is now more explicit.Unfortunately there's a bug in hatchet preventing that specific test heroku/hatchet#71 so instead I'm at least asserting that it's an absolute path.