Update build pack to upstream #4
Open
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
Update changelog
Bootstrap Ruby version of buildpack
``` bundle clean ``` Emits a "suggestion" indicating a more recent version of bundler is available, his is not needed and adds a network request that slows us down. # Conflicts: # lib/language_pack/ruby.rb
v4.0.12 introduced a fix for a deadlock in the reaper: heroku/hatchet@446c280 # Conflicts: # Gemfile.lock
Feature introduced in Shopify/ci-queue#99 This will output a failing test order if the suite fails.
Rev Bundler 2 to 2.0.2
Use the correct coloring for a warning via using the `warn` method. Also link directly to the known upgrade issues, as that is likely what people will want to read when they hit a problem with bundler.
Windows uses carriage return and newline which prevents the `$` regex from properly matching as currently placed. By opening the file in "text" mode i.e. `rt` the issue is avoided.
The current Rails 5 language pack work for Rails 6 applications but it could not be used because of the version constraints. This was making Rails 6 apps to be handled as generic rack apps and causing problems at deploy if no SECRET_KEY_BASE was set. By adding a new class we can update the constraint so new Rails 6 apps can work as in the pre-releases.
Rails 6 Support
Rails 6 release has a breaking change rails/rails#36542. While this is fixed on 6-0-stable that version is not yet released.
Fix tmp/pids folder creation for Rails 6 apps
clean old bundler binstubs on load
The yarn_installer_spec mutates the current environment. It doesn't test anything other than that the yarn lib was added to the path, which doesn't even tell us if it's a valid test. This test also had competing chdir calls which trigger a warning in Ruby (sometimes, but not always which is really surprising to me). This warning happens because the `LanguagePack::Ruby.new includes a `Dir.chdir` call in it. Instead of working around this warning, we can safely get rid of this test as yarn is exercised directly in other integration tests.
Log test order
If you don't know if code will mutate a process information, such as environment variables then run it in a fork to ensure it does not affect the parent process.
Currently when you run any `LanguagePack::<Klass>.use?` such as `LanguagePack::Rails6.use?` it will mutate the environment by loading in bundler and modifying the LOAD_PATH.
This happens because the `use?` method relies on bundler:
```
def self.use?
instrument "rails6.use" do
rails_version = bundler.gem_version('railties')
return false unless rails_version
is_rails = rails_version >= Gem::Version.new('6.x') &&
rails_version < Gem::Version.new('7.0.0')
return is_rails
end
end
```
Which loads and initializes this wrapper:
```
def self.bundler
@@bundler ||= LanguagePack::Helpers::BundlerWrapper.new.install
end
```
And the wrapper install method mutates globals:
```
def install
ENV['BUNDLE_GEMFILE'] = @gemfile_path.to_s
fetch_bundler
$LOAD_PATH << @path
require "bundler"
self
end
```
This is an issue because other tests such as the rake_runner_spec tests expect a specific version of Rake to be available however if the load path is mutated and bundler is required then only one specific version of rake will be available on the system.
While I _believe_ that this fixes some flappy tests, I'm not totally sure why it only fails sometimes. You would think that it would either always pass or always fail for a given order. This test order is taken from a failing test run:
```
$ cat << EOL > log/test_order_fails_sometimes.log
./spec/helpers/rake_runner_spec.rb[1:2]
./spec/helpers/shell_spec.rb[1:2:1]
./spec/helpers/jvm_installer_spec.rb[1:4]
./spec/installers/yarn_installer_spec.rb[1:1:1:1]
./spec/installers/heroku_ruby_installer_spec.rb[1:1:1]
./spec/helpers/rake_runner_spec.rb[1:4]
./spec/hatchet/stack_spec.rb[1:1]
./spec/hatchet/rails6_spec.rb[1:1]
./spec/hatchet/jvm_spec.rb[1:1]
./spec/helpers/rake_runner_spec.rb[1:1]
./spec/helpers/jvm_installer_spec.rb[1:6]
./spec/helpers/jvm_installer_spec.rb[1:1]
./spec/hatchet/bugs_spec.rb[1:3:1]
./spec/hatchet/bundler_spec.rb[1:1]
./spec/installers/heroku_ruby_installer_spec.rb[1:1:2:1]
./spec/hatchet/ruby_spec.rb[1:4:2:1]
./spec/hatchet/rubies_spec.rb[1:3]
./spec/hatchet/node_spec.rb[1:2]
./spec/hatchet/getting_started_spec.rb[1:1]
EOL
$ while bundle exec rspec-queue --queue log/test_order_fails_sometimes.log --build 1 --worker 2; do :; done
```
Test output link https://dashboard.heroku.com/pipelines/ac057663-170b-4bdd-99d0-87560eb3a570/tests/519
This PR possible to the feature added in heroku/hatchet#65
The binstub that Rails generates has a bug in it that prevents calling `yarn install` from any directory other than the app root. We were accidentally handling this when we install yarn (without the nodejs buildpack) since we prepend the yarn path https://github.com/heroku/heroku-buildpack-ruby/blob/af368418f2aef9f455afd7f0b77b3137aa8d38a5/lib/language_pack/ruby.rb#L696. However if the app is using the `heroku/nodejs` then the `bin/yarn` binstub will appear first in the buildpack after the changes in heroku@8413eec. To fix this, we are manually putting yarn first on the path at runtime and build time. While this is a fix to a regression on master, it has never been released. This fix preserves existing behavior with the deployed buildpack. (In which the `bin/yarn` binstub is not being used).
[close heroku#1001][changelog skip] Put Yarn first on the path
Previously when we gave people a warning that they were not setting their Ruby version we recommended that they set that version in their Gemfile. Since our default ruby versions are "sticky" it was common that we were recommending very old ruby versions:
```
###### WARNING:
You have not declared a Ruby version in your Gemfile.
To set your Ruby version add this line to your Gemfile:
ruby '2.4.1'
```
Instead of recommending their current version (Which is also available via the deploy output elsewhere) we are recommending the current default version. So in the previous case where the user was using 2.4.1, our recommended Gemfile in the warning would be `2.6.6`.
Previously when we gave people a warning that they were not setting their Ruby version we recommended that they set that version in their Gemfile. Since our default ruby versions are "sticky" it was common that we were recommending very old ruby versions:
```
###### WARNING:
You have not declared a Ruby version in your Gemfile.
To set your Ruby version add this line to your Gemfile:
ruby '2.4.1'
```
Instead of recommending their current version (Which is also available via the deploy output elsewhere) we are recommending the current default version. So in the previous case where the user was using 2.4.1, our recommended Gemfile in the warning would be `2.6.6`.
Warn on 2.6 old versions due to bundler binstub bug
…er-warning [changelog skip][close heroku#977] Recommend recent Ruby in warning
Spring causes erratic and unpredictable failures, most recently in ticket https://heroku.support/881089. We can avoid these class of problems entirely by disabling the library. Any benefits that spring provide come from repeatedly calling the same command, for example spring would speed up running `rails test` twice. However most commands on Heroku are only run once (`rails server` on boot for example) so using spring on the system would provide little to no realized practical benefit. I feel so strongly about the need to disable spring that I do not want to provide an "escape valve" to disable the behavior of disabling spring.
[close heroku#818] Disable spring
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: heroku#586 - Rolled back: heroku#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.
[close heroku#990] Warn on bad shebang line
This reverts commit aeed0ce, reversing changes made to 30184b6. Due to ticket https://heroku.support/885880 Error is: ``` ! ! invalid byte sequence in UTF-8 ! /app/tmp/buildpacks/dir/lib/language_pack/helpers/binstub_check.rb:27:in match?': invalid byte sequence in UTF-8 (ArgumentError) from /app/tmp/buildpacks ```
…g-check Revert "Merge pull request heroku#1014 from heroku/schneems/fu-binstubs"
* Revert "Merge pull request heroku#1020 from heroku/schneems/revert-bad-shebang-check" This reverts commit da07466, reversing changes made to aeed0ce. * Handle binary binstubs Logic was added to check the "shebang" lines of binstubs. This was then reverted due to a bug heroku#1019. This commit fixes this error by checking if a shebang line can be considered UTF-8 for purposes of comparison before we compare it. In the process I also refactored the code so that the logic around working with the binstub file is in BinstubWrapper. I added a test for the `BinstubWrapper#binary?` method as well as a higher level test that runs the whole check against the directory that contains the `which ruby` binary. * Document v216 rollback in git This deploy was rolled back due to heroku#1005
I've done something a bit funny with this release. The default branch is very far ahead of what was released in v215. There's some bugs preventing a release, but a release of stack support is blocking other work. There's two ways to release that work: - Revert everything on default branch back to v215 and then re-apply the patches we need for stack support, cut a release from master, then un-revert everything that was reverted. - Pros: The release will come from a point in time in the default branch. - Cons there's a lot of hoops to jump through. Mistakes might get made, it is messy. - Keep default branch separate from the code released in v217. All commits to the v217 branch are cherry-picked from master/main, the only difference is we're re-writing history to not have the rest of the features. - Pros: The default branch commits stay clean, there's no reverting and then un-reverting - Cons: The release v217 is being made from a branch which might be confusing in the future.
After v216 was released there were several failures reported in heroku#1029 and heroku#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 heroku#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 heroku#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: heroku#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 heroku#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 heroku#1005 and not required re-compiling dozens of binaries, but this bug was found and diagnosed after heroku#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.
v218 is released, these are no longer "unreleased".
* [close heroku#934] Skip rake task if it does not exist If the `assets:clean` task does not exist, don't call it. * Add test to assert `assets:clean` is called
(heroku#1037) * make gem_layer accessible to subsequent buildpacks * changelog * Test gem layer is exported Co-authored-by: Kamal Nasser <[email protected]>
ryanong
changed the title
|
Is this one still dependent on #9961? What's the status? |
jnoconor
reviewed
Jan 27, 2021
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.
Actual diff heroku/heroku-buildpack-ruby@master...vhx:update-to-upstream
If you look at the actual diff you can see that the only change this buildpack has is that of turbo-sprockets-rails3
This is a worthwhile change for us until we get to rails 4.2 where this feature is built in.
I originally explored this to upgrade to bundler 2.1.4 but rails 3.2 only support bundler 1.17.3
This update includes better node support, security checks, and a couple other features.