-
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
[close #1025] Remove default bin/rake binstub #1031
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After v216 was released there were several failures reported in #1029 and #1005 these are both related to `bin/` being put first on the path consistently in build and run time. After investigation it was uncovered that `bin/rake` was a file we're putting in that location if the customer does not check in a `bin/rake` binstub. This file is generated by compiling a Ruby binary: ``` $ curl -O https://heroku-buildpack-ruby.s3.amazonaws.com/heroku-18/ruby-2.7.1.tgz $ tar -xzf ruby-2.7.1.tgz bin/ $ cat bin/rake #!/bin/sh # -*- ruby -*- # This file was generated by RubyGems. # # The application 'rake' is installed as part of a gem, and # this file is here to facilitate running it. # _=_\ =begin bindir="${0%/*}" exec "$bindir/ruby" "-x" "$0" "$@" =end #!/usr/bin/env ruby require 'rubygems' version = ">= 0.a" str = ARGV.first if str str = str.b[/\A_(.*)_\z/, 1] if str and Gem::Version.correct?(str) version = str ARGV.shift end end if Gem.respond_to?(:activate_bin_path) load Gem.activate_bin_path('rake', 'rake', version) else gem "rake", version load Gem.bin_path("rake", "rake", version) end ``` v216 release caused two related issues due to having `bin/rake first on the PATH. ## Bad shebang lines Issue #1005 is related to having bad shebang lines: ``` $ cat bin/rake | head -n 1 #!/app/vendor/ruby-2.3.8/bin/ruby ``` This was fixed by modifying our compilation code in: heroku/docker-heroku-ruby-builder#5 ## Bundler not loaded Issue #1029 occurs because the code in `bin/rake` that is generated from compiling Ruby is not bundler aware. It does not load bundler. As a result: - Git based gems do not work with the Ruby compiled `bin/rake` - Referencing code from bundler like `Bundler.setup` does not work because it's not yet required This behavior is described in detail in this comment: #1025 (comment) The short version is: Without putting `bin/` first on the path at build time then the binstub generated by bundler in the location `vendor/bundle/bin/` takes precedence and this code (since it is generated by bundler) is bundler aware. When `bin/` was moved to be first in the path, this behavior broke. ## Fix implementation This fix for #1029 works by not putting the Ruby compiled `bin/rake` on in the customer's `bin/` folder. We keep `bin/` first in the path so if a customer is checking in a binstub their binstub will be used. If they are not checking in a binstub then the bundler generated version of `vendor/bundle/bin/` will be used. This fix also would have superseded the work to fix #1005 and not required re-compiling dozens of binaries, but this bug was found and diagnosed after #1005 was reported. ## Known risks The one risk with this approach is that anyone relying on running `rake assets:precompile` at build time but they're not using Rake in their Gemfile will no longer work. I believe this is not a common case, and it's best practice to have a specific version of Rake in the Gemfile.lock.
a2dd492
to
50ee521
Compare
edmorley
approved these changes
Jul 7, 2020
krisrang
added a commit
to skyltmax/heroku-buildpack-ruby
that referenced
this pull request
Oct 2, 2020
* upstream/master: [changelog skip] Bring back rake task (heroku#1038) CNB: make the gems layer accessible to subsequent buildpacks heroku#1033 (heroku#1037) [close heroku#934] Skip rake task if it does not exist (heroku#1036) [changelog skip] Move unreleased changelogs (heroku#1035) v218 for Monday release (heroku#1034) [close heroku#1029] Remove default bin/rake binstub (heroku#1031) Document v217 release in master (heroku#1030) [close heroku#1027][changelog skip] Fix frozen string error (heroku#1028) Handle binary binstubs (heroku#1021) v216 release for monday (heroku#1023) Revert "Merge pull request heroku#1014 from heroku/schneems/fu-binstubs" [close heroku#990] Warn on bad shebang line [close heroku#818] Disable spring [changelog skip][close heroku#977] Recommend recent Ruby in warning [changelog skip][close heroku#977] Recommend recent Ruby in warning [close heroku#1001] Put Yarn first on the path Update HEREDOC to 2.5 syntax to support indenting Switch to using /usr/bin/env bash [changelog skip] Fix CNB tests Allow Nolockfile to get to compile phase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After v216 was released there were several failures reported in #1025 and #1005 these are both related to
bin/
being put first on the path consistently in build and run time.After investigation, it was uncovered that
bin/rake
was a file we're putting in that location if the customer does not check in abin/rake
binstub. This file is generated by compiling a Ruby binary:v216 release caused two related issues due to having `bin/rake first on the PATH.
Bad shebang lines
Issue #1005 is related to having bad shebang lines:
This was fixed by modifying our compilation code in: heroku/docker-heroku-ruby-builder#5
Bundler not loaded
Issue #1029 occurs because the code in
bin/rake
that is generated from compiling Ruby is not bundler aware. It does not load bundler. As a result:bin/rake
Bundler.setup
does not work because it's not yet requiredThis behavior is described in detail in this comment: #1025 (comment)
The short version is: Without putting
bin/
first on the path at build time then the binstub generated by bundler in the locationvendor/bundle/bin/
takes precedence and this code (since it is generated by bundler) is bundler aware. Whenbin/
was moved to be first in the path, this behavior broke.Fix implementation
This fix for #1029 works by not putting the Ruby compiled
bin/rake
on in the customer'sbin/
folder. We keepbin/
first in the path so if a customer is checking in a binstub their binstub will be used. If they are not checking in a binstub then the bundler generated version ofvendor/bundle/bin/
will be used.This fix also would have superseded the work to fix #1005 and not required re-compiling dozens of binaries, but this bug was found and diagnosed after #1005 was reported.
Known risks
The one risk with this approach is that anyone relying on running
rake assets:precompile
at build time but they're not using Rake in their Gemfile will no longer work. I believe this is not a common case, and it's best practice to have a specific version of Rake in the Gemfile.lock.