-
Notifications
You must be signed in to change notification settings - Fork 3
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
Warn if ruby/rails/bundle binstub shebangs are bad #586
Conversation
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.
I think we need to move the logic to a different place.
We could update one of the existing integration tests with a bad looking binstub that actually still works like
#!/usr/bin/env ruby #bad-binstub
And then assert the output on deploy in one of the existing Hatchet tests matched the string #bad-binstub
. That way we could be sure that the change was being tested.
@@ -22,5 +22,20 @@ def fetch_unpack(ruby_version, install_dir, build = false) | |||
@fetcher.fetch_untar(file) | |||
end | |||
end | |||
|
|||
def setup_binstubs(*) |
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 the right file to add this logic? We use this to installer to download the version of Ruby that the buildpack bootstraps with.
It gets called here
installer = LanguagePack::Installers::HerokuRubyInstaller.new(ENV['STACK'] || DEFAULT_STACK) |
and that gets called from this script
$bin_dir/support/download_ruby $heroku_buildpack_ruby_dir |
I don't think we're inside of the right directory either.
if File.file?(binstub) | ||
shebang = File.open(binstub, &:readline) | ||
if !shebang.match %r{\A#!/usr/bin/env ruby(.exe)?\z} | ||
warn("Binstub #{binstub} contains shebang #{shebang}. This may cause issues if the program specified is unavailable.", inline: true) |
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.
I would like a link to that devcenter article you wrote here. If you give me the link I can edit it and give some feedback as well.
This is the main flow for the compile action heroku-buildpack-ruby/lib/language_pack/ruby.rb Lines 91 to 114 in cebdf68
If we can't find a good place to shoe-horn in another check, one option is to add a new method like The trick is we want to do this before any errors could crop up that might arise from the thing we're warning about. In this case we can inspect the contents from disk any time, so perhaps doing it before we install ruby would be good because we use |
aa009ee
to
6720d13
Compare
Binstubs, which RubyGems and Bundler generate to prepare the environment before dispatching calls to original executables, can sometimes include shebangs that worked on developer machines, but won't work on Heroku. Often, this is caused by ruby binaries being named other than `ruby` (or `ruby.exe` on Windows). While it can happen for any binstub, we're mostly concerned about `rake`, `rails`, and `bundler` stubs. Check each of these to see if they are `!#/usr/bin/env ruby` or `!#/usr/bin/env ruby.exe` and issue a warning if this is not the case.
6720d13
to
ad7d764
Compare
Revert "Merge pull request #586 from heroku/check-binstubs"
* upstream/master: (213 commits) Revert "Merge pull request #586 from heroku/check-binstubs" Move binstub detection to main compile flow Warn if ruby/rails/bundle binstub shebangs are bad Added explicit case for JVM version 8 or 1.8 in system.properties v168 install node when using execjs OR webpacker fix typo in bin/support/ruby_compile Update comment in bin/compile Retry failed downloads for untar v167 Skip Bundler version check Use HTTPS for interactions with GitHub and S3 Changelog. Bundler 1.15.2 update changelog bump to v166 [CI Skip] heroku-buildpack-ruby v165 Add CHANGELOG v165 Update Node to 6.11.1 Set JAVA_HOME env var for JRuby apps ...
This article explains binstubs and "shebang" lines: https://devcenter.heroku.com/articles/bad-ruby-binstub-shebang. A relatively common problem when generating a binstub is for it to contain a bad shebang line. Here's an example: ``` #!/usr/bin/env ruby2.5 ``` If you've got a binstub with that shebang line in your project and are running on Heroku 18 then your app will be forced to use `ruby2.5` binary no matter what version of Ruby you've specified in the `Gemfile`. This causes very odd behavior and weird, difficult to debug bundler errors. This PR does not fix the problem, but will at least warn anyone with a version number in their ruby shebang line. This behavior was originally: - Introduced: #586 - Rolled back: #623 It was rolled back because it would falsely claim that `#!/usr/bin/env bash` was incorrect also it only protected against a limited set of binstubs.
Binstubs, which RubyGems and Bundler generate to prepare the environment
before dispatching calls to original executables, can sometimes include
shebangs that worked on developer machines, but won't work on Heroku.
Often, this is caused by ruby binaries being named other than
ruby
(orruby.exe
on Windows).While it can happen for any binstub, we're mostly concerned about
rake
,rails
, andbundler
stubs. Check each of these to see if theyare
!#/usr/bin/env ruby
or!#/usr/bin/env ruby.exe
and issue awarning if this is not the case.